-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 instance_redistribution_type to region IGM #2212
Add instance_redistribution_type to region IGM #2212
Conversation
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesNo diff detected in terraform-google-conversion. New Pull RequestsI built this PR into one or more new PRs on other repositories, and when those are closed, this PR will also be merged and closed. |
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.
LGTM- just some minor thoughts/concerns on my part, neither are blockers.
@@ -16,6 +16,13 @@ import ( | |||
"google.golang.org/api/compute/v1" | |||
) | |||
|
|||
const ( | |||
// Some fields are only supported by region IGM - these flags are used to |
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 a little 👎 on using the same method as the resources diverge. While it's fine now while there's one field that's different, I'm afraid of carrying the pattern forward if more fields like this are added.
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.
yeah, i could go either way on splitting right now honestly
const ( | ||
// Some fields are only supported by region IGM - these flags are used to | ||
// manage non-shared fields in shared expanders/flatteners. | ||
includeMultiZoneFields = true |
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.
includeMultiZoneFields = true | |
includeRegionalOnlyFields = true |
Given that there's a precedent for a "multi-zonal" cluster that's distinct from a regional cluster in GKE, I'm a little hesitant to call this MultiZone
instead of Regional
in case multi-zonal evolves as a distinct concept in IGM. (It's pretty unlikely, admittedly)
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.
can switch, i used multizonal because that's what the error message said and I had to do the "that means regional" switch in my head
9d033e7
to
be8e476
Compare
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
Tracked submodules are build/terraform-beta build/terraform-mapper build/terraform build/ansible build/inspec.
Release Note for Downstream PRs (will be copied)