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

Changed host ip lookup to IPv4 #17

Merged
merged 2 commits into from
Feb 7, 2024

Conversation

maxhardt
Copy link
Contributor

@maxhardt maxhardt commented Feb 4, 2024

No description provided.

Copy link
Contributor

@jimright jimright left a comment

Choose a reason for hiding this comment

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

Hi @maxhardt,

Thanks for handling this. See comments for one set of changes required to handle the case where the ingress_extra_cidrs_and_ports input variable is set.

Also it looks like your commit is missing DCO signoff (you had this on cloudera-labs/terraform-cdp-modules#53 so might just be a git config setting).

aws/main.tf Outdated Show resolved Hide resolved
azure/main.tf Outdated Show resolved Hide resolved
gcp/main.tf Outdated Show resolved Hide resolved
Signed-off-by: Maximilian Engelhardt <[email protected]>
Signed-off-by: Maximilian Engelhardt <[email protected]>
@maxhardt
Copy link
Contributor Author

maxhardt commented Feb 7, 2024

Thanks for catching these @jimright, followed your suggestion in the latest commits. Would be great to have some rudimentary (unit) testing to catch these when developing as well, happy to contribute! 🧪

@jimright
Copy link
Contributor

jimright commented Feb 7, 2024

Thanks for catching these @jimright, followed your suggestion in the latest commits. Would be great to have some rudimentary (unit) testing to catch these when developing as well, happy to contribute! 🧪

Yes, the addition of some testing is great suggestion and would definitely help to catch potential issues or regressions early.
We've done some terratest for another project but Terraform have also recently released a native terraform test framework that should help. Here's an overview - https://www.hashicorp.com/blog/testing-hashicorp-terraform. Let's review this and see what we can achieve.

Copy link
Contributor

@jimright jimright left a comment

Choose a reason for hiding this comment

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

Looks good. Approved.

@jimright jimright merged commit 0c7e9d5 into cloudera-labs:main Feb 7, 2024
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