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

bug(plugin): Fix incorrect use of WithIpAddrs in Repository.Endpoints #1849

Merged
merged 1 commit into from
Feb 10, 2022

Conversation

justenwalker
Copy link
Contributor

When creating the options for the preferencer, this used WithIpAddrs for both the host addresses and also DNS names. This causes errors when a host contains DNS names that are not IP addresses (ie: whenever dns names are present).

This change will:

  • Replace WithIpAddrs with WithDNSNames option for DNS Names when constructing options for the preferencer.
  • Add a test which includes a DNS name to prevent regressions

Fixes #1848

Copy link
Contributor

@talanknight talanknight left a comment

Choose a reason for hiding this comment

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

This is great! Thank you so much for identifying and fixing this bug as well as adding regression testing. I have a suggestion for a very minor tweak regarding the test case.

internal/host/plugin/repository_host_set_test.go Outdated Show resolved Hide resolved
When creating the options for the preferencer, this used WithIpAddrs for both the host addresses and also DNS names. This causes errors when a host contains DNS names that are not IP addresses (ie: whenever dns names are present).

This change will:
- Replace WithIpAddrs with WithDNSNames option for DNS Names when constructing options for the preferencer.
- Add a test which includes a DNS name to prevent regressions

Fixes hashicorp#1848
@justenwalker
Copy link
Contributor Author

This is great! Thank you so much for identifying and fixing this bug as well as adding regression testing. I have a suggestion for a very minor tweak regarding the test case.

I've applied your suggestions to this PR. Please have another look and let me know if there is anything else you think I should do.

Copy link
Contributor

@talanknight talanknight left a comment

Choose a reason for hiding this comment

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

This looks great and ready to merge! Thank you again for the issue and pull request.

@talanknight talanknight merged commit 6166d22 into hashicorp:main Feb 10, 2022
@justenwalker justenwalker deleted the justen/fix-1848 branch February 11, 2022 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] incorrect use of WithIpAddrs in plugin.Repository.Endpoints
2 participants