-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Add Beta support & Beta feature ip_version to google_compute_global_address #250
Merged
Merged
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
37f0ec3
Make google_compute_global_address a versioned resource with Beta sup…
rileykarson 0e4698e
Added Beta support for ip_version in google_compute_global_address.
rileykarson d63606f
Move checks to TestCheckFuncs, add a regression test for IPV4 on v1 r…
rileykarson 07a88f4
Consolidated TestCheckFuncs to a single function.
rileykarson b183c5a
Add missing return statement.
rileykarson dea7db7
Fix IPV4 test
rileykarson a38bd13
Clarified comment.
rileykarson File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider asserting on the format of the computed address to make sure an IPv6 is used.
Additionally, to prevent regression, make sure the ipv4 address follows the ipv4 pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I agree that we need to validate that the API isn't lying to us when it says an IP address is at some version; especially when we would need to use a regex or vendor a new package to verify it.
Added a test for IPV4 on
_basic
, and moved checks to a TestCheckFunc.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use https://golang.org/pkg/net/#ParseIP
ip := ParseIP(address)
ip.To16 != nil // means it is a IPv6 address.
ip.To4 != nil // means this is a IPv4 address.
Checking if the IpVersion match what we expect is probably fine for the IPv6 case. My main goal was not to test if the API "lies" to us but rather make sure we don't create an ipv6 address when we don't use the beta feature (and since we don't have the ip version field in that case, the only way to check is looking at address itself to validate it is ipv4).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, ok - we can actually just read the resource using a Beta API in this test and see the
IpVersion
; that's what the TestCheckFunc does.