Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code refactoring + Acceptance/Unit testing #111

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

abikouo
Copy link
Collaborator

@abikouo abikouo commented Apr 17, 2024

This pull request introduces the following changes:

  • Some code refactoring to aggregate the redundant operation into functions in utils.go. Creates a Parser struct to parse schema.ResourceData avoid repeating type assertion into all resource/data.
  • Remove tests/integration as this was drastically performing a file and using Terraform acceptance tests framework instead
  • Introduce unit tests for critical functions
  • bugfix: Remove call to Wait() function when using exec.Command as this is raising errors because the command is started using CombinedOutput()

Copy link
Member

@gravesm gravesm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most/nearly all of the refactoring in here seems to be to try and hide the ugliness of all the type assertions required by the SDK. If we were going to keep using the SDK it would probably be worth doing something like this. I think it's likely we'll upgrade to the framework soon, though. When we do that, this problem largely goes away, so I'm not sure it's worth it to do all this refactoring.

What I would really like to see is a PR that fixes the one bug you mentioned, and a PR that switches to using the test framework. The testing changes are sorely needed and unlikely to change much when switching to the framework.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants