-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Fix permadiff that reorders stateful_external_ip
blocks on google_compute_instance_group_manager
and google_compute_region_instance_group_manager
resources
#9577
Conversation
@rileykarson - I requested you as reviewer as you provided initial feedback on the issue this PR relates to about how the solution would be to re-order information from the API to match the user's config. I feel a bit weird about this logic being in the flattener, but on the other hand the flattener was already performing a data transformation. If you think this reordering should be performed in a function separate to the flattener (or should be pulled into a separate function that's used in there, etc) please let me know! Having said that, we're lucky that whatever we land on would be removed when the 'true' fix for this issue, changing the field to a Set, is added in a future major release. Also, should I extend this logic to also affect the stateful_internal_ip field? |
Odd error in the generate diffs build:
I'll try again /gcbrun |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 4 files changed, 369 insertions(+), 6 deletions(-)) |
mmv1/third_party/terraform/services/compute/resource_compute_instance_group_manager.go.erb
Outdated
Show resolved
Hide resolved
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 4 files changed, 370 insertions(+), 6 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccRegionInstanceGroupManager_APISideListRecordering |
Rerun these tests in REPLAYING mode to catch issues
|
7099d6a
to
6578582
Compare
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 4 files changed, 388 insertions(+), 7 deletions(-)) |
Tests analyticsTotal tests:
|
@rileykarson could you please take a look at this when you have a chance? Thanks! |
mmv1/third_party/terraform/services/compute/resource_compute_instance_group_manager.go.erb
Show resolved
Hide resolved
…ernal_ip` fields from API to match user's config
…tefulExternalIps`
…` blocks vs 0 or 1+ in API data
…ring and data transformation occur in same loop
…e, update code to pass
6578582
to
db91a96
Compare
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 4 files changed, 429 insertions(+), 7 deletions(-)) |
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.
Small note on deadcode in the handwritten test, but otherwise LGTM. Going through some snippets in the parent comments:
The root of this problem is that the API returns those blocks in a sorted order that doesn't match the user's config.
Just to make sure I'm not missing something, it returns them in an order that doesn't match the user's config, but not a sorted order, i.e. the ordering on the API side does not matter, right? That's my understanding based on hashicorp/terraform-provider-google#13430 (comment)
I feel a bit weird about this logic being in the flattener...
Seems right to me. I'd only really consider breaking out of a flattener if we were accessing multiple same-level fields. We could use a decoder to handle both the external and internal versions in the same place, although flatteners are fine since their individual handlers are still field-scoped. (wrt. child fields in when I'd use a flattener vs an alternative, a parent can access its children in its flattener)
Also, should I extend this logic to also affect the stateful_internal_ip field?
I think so, unless there's a reason this issue couldn't happen there!
..._party/terraform/services/compute/resource_compute_region_instance_group_manager_test.go.erb
Outdated
Show resolved
Hide resolved
...arty/terraform/services/compute/resource_compute_instance_group_manager_internal_test.go.erb
Outdated
Show resolved
Hide resolved
Tests analyticsTotal tests: Click here to see the affected service packages
|
Thanks for the review!
Yeah you're right that the API side ordering doesn't appear to matter. It's not clear if the reordering is due to the API explicitly choosing to do that or because the Compute Go client library storing the data in a map results in losing the old ordering. Below's an example of the map where the keys correspond to
I'll update this PR to pull the sorting code into a reusable function and I'll update the |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 4 files changed, 373 insertions(+), 21 deletions(-)) |
Tests analyticsTotal tests: Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccRegionInstanceGroupManager_APISideListRecordering |
Rerun these tests in REPLAYING mode to catch issues
|
Re-requesting review as I've made big-ish changes since the last approval and don't want to abuse the fact the approval isn't dismissed by new commits |
…compute_instance_group_manager` and `google_compute_region_instance_group_manager` resources (GoogleCloudPlatform#9577) * Make `flattenStatefulPolicyStatefulExternalIps` reorder `stateful_external_ip` fields from API to match user's config * Add unit test for new ordering behaviour of `flattenStatefulPolicyStatefulExternalIps` * Whitespace fix * Update test file to be templated to use GA or Beta version of compute client library * Add new test cases where the config contains no `stateful_external_ip` blocks vs 0 or 1+ in API data * Refactor `flattenStatefulPolicyStatefulExternalIps` to make data ordering and data transformation occur in same loop * Add test case for when nic in TF config is missing in the API response, update code to pass * Remove unused test step * Refactor IP sorting code to be reusable * Update `flattenStatefulPolicyStatefulInternalIps` to reorder IPs * Update unit and acceptance tests following refactor
…compute_instance_group_manager` and `google_compute_region_instance_group_manager` resources (GoogleCloudPlatform#9577) * Make `flattenStatefulPolicyStatefulExternalIps` reorder `stateful_external_ip` fields from API to match user's config * Add unit test for new ordering behaviour of `flattenStatefulPolicyStatefulExternalIps` * Whitespace fix * Update test file to be templated to use GA or Beta version of compute client library * Add new test cases where the config contains no `stateful_external_ip` blocks vs 0 or 1+ in API data * Refactor `flattenStatefulPolicyStatefulExternalIps` to make data ordering and data transformation occur in same loop * Add test case for when nic in TF config is missing in the API response, update code to pass * Remove unused test step * Refactor IP sorting code to be reusable * Update `flattenStatefulPolicyStatefulInternalIps` to reorder IPs * Update unit and acceptance tests following refactor
…compute_instance_group_manager` and `google_compute_region_instance_group_manager` resources (GoogleCloudPlatform#9577) * Make `flattenStatefulPolicyStatefulExternalIps` reorder `stateful_external_ip` fields from API to match user's config * Add unit test for new ordering behaviour of `flattenStatefulPolicyStatefulExternalIps` * Whitespace fix * Update test file to be templated to use GA or Beta version of compute client library * Add new test cases where the config contains no `stateful_external_ip` blocks vs 0 or 1+ in API data * Refactor `flattenStatefulPolicyStatefulExternalIps` to make data ordering and data transformation occur in same loop * Add test case for when nic in TF config is missing in the API response, update code to pass * Remove unused test step * Refactor IP sorting code to be reusable * Update `flattenStatefulPolicyStatefulInternalIps` to reorder IPs * Update unit and acceptance tests following refactor
…compute_instance_group_manager` and `google_compute_region_instance_group_manager` resources (GoogleCloudPlatform#9577) * Make `flattenStatefulPolicyStatefulExternalIps` reorder `stateful_external_ip` fields from API to match user's config * Add unit test for new ordering behaviour of `flattenStatefulPolicyStatefulExternalIps` * Whitespace fix * Update test file to be templated to use GA or Beta version of compute client library * Add new test cases where the config contains no `stateful_external_ip` blocks vs 0 or 1+ in API data * Refactor `flattenStatefulPolicyStatefulExternalIps` to make data ordering and data transformation occur in same loop * Add test case for when nic in TF config is missing in the API response, update code to pass * Remove unused test step * Refactor IP sorting code to be reusable * Update `flattenStatefulPolicyStatefulInternalIps` to reorder IPs * Update unit and acceptance tests following refactor
Relates to (but doesn't close!) this issue : hashicorp/terraform-provider-google#13430
This PR updates code related to
stateful_external_ip
blocks in thegoogle_compute_instance_group_manager
&google_compute_region_instance_group_manager
resources to address a permadiff that proposes re-ordering those blocks. The root of this problem is that the API returns those blocks in a sorted order that doesn't match the user's config. The state will contain data returned from the API, so there's always a plan trying to return to the config-defined order.Prior to this PR, the flattener function for
stateful_external_ip
blocks performs a conversion from a map (how the data from the API is presented by the compute Go client library) to an array (how the data is stored in state).This PR updates the flattener function to also re-order data returned by the API so it matches the user's Terraform configuration. This is achieved by passing the
schema.ResourceData
data for the resource into the flattener, to act as a source of information about the order of those fields in the user's config.Release Note Template for Downstream PRs (will be copied)