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

Revert "Revert "Fix permadiff that reorders stateful_external_ip blocks on google_compute_instance_group_manager and google_compute_region_instance_group_manager resources"" #9758

Conversation

SarahFrench
Copy link
Collaborator

@SarahFrench SarahFrench commented Jan 5, 2024

Reverts #9754

I'm re-introducing the original #9577 PR but changing the test case that's vulnerable to the reordering problem.

compute: fixed a permadiff that reordered `stateful_external_ip` and `stateful_internal_ip` blocks on `google_compute_instance_group_manager` and `google_compute_region_instance_group_manager` resources

…ocks on `google_compute_instance_group_manager` and `google_compute_region_instance_group_manager` resources" (#9754)"

This reverts commit 24ff156.
@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 4 files changed, 373 insertions(+), 21 deletions(-))
Terraform Beta: Diff ( 4 files changed, 373 insertions(+), 21 deletions(-))

},
},
},
"Two IPs (nic0, nic1). None stored in config and both stored in API data": {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This test failed before because the data in the client library has uncertain order due to being a map's k:v pairs yet the provider converts that data into a slice and tries to preserve an order. The test case has 2 interfaces that aren't present in the config and the new sorting logic only acts to sort interfaces that match those in the config; interfaces not in the config are added in what ever order they come from the client library. If the client library has >1 of those then "the order they come from the client library" can vary due to randomness, which means tests cannot assert an expected order.

Other test cases in this unit test are fine because there's only ever 1 'unexpected' interface appended to the end of the slice.

One solution is to update this test case to only consider 1 interface, or another solution is to order the unexpected interfaces stored in unexpectedIps

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll add ordering of the unexpectedIps slice (that contains interfaces returned from the API that aren't in the config). This will allow this unit test to assert what data returned from the client library will look like without being affected by the randomness of converting maps to slices.

In a real world scenario, if a user has >1 interfaces that aren't present in their Terraform config they will see a diff in their plan and the order of those new fields might change in repeated plans. If I implemented ordering of unexpectedIps users will benefit by those new fields being displayed in a consistent order.

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 861
Passed tests 796
Skipped tests: 65
Affected tests: 0

Click here to see the affected service packages
  • compute

$\textcolor{green}{\textsf{All tests passed in REPLAYING mode.}}$
View the build log

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 4 files changed, 399 insertions(+), 21 deletions(-))
Terraform Beta: Diff ( 4 files changed, 399 insertions(+), 21 deletions(-))

@SarahFrench SarahFrench marked this pull request as ready for review January 5, 2024 12:12
@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 861
Passed tests 796
Skipped tests: 65
Affected tests: 0

Click here to see the affected service packages
  • compute

$\textcolor{green}{\textsf{All tests passed in REPLAYING mode.}}$
View the build log

@SarahFrench SarahFrench merged commit fabeca9 into main Jan 5, 2024
11 checks passed
bskaplan pushed a commit to bskaplan/magic-modules that referenced this pull request Jan 17, 2024
…ocks on `google_compute_instance_group_manager` and `google_compute_region_instance_group_manager` resources"" (GoogleCloudPlatform#9758)

* Revert "Revert "Fix permadiff that reorders  `stateful_external_ip` blocks on `google_compute_instance_group_manager` and `google_compute_region_instance_group_manager` resources" (GoogleCloudPlatform#9754)"

This reverts commit 24ff156.

* Sort unexpected IPs not present in TF config, update unit test
kylase pushed a commit to yuanchuankee/magic-modules that referenced this pull request Jan 21, 2024
…ocks on `google_compute_instance_group_manager` and `google_compute_region_instance_group_manager` resources"" (GoogleCloudPlatform#9758)

* Revert "Revert "Fix permadiff that reorders  `stateful_external_ip` blocks on `google_compute_instance_group_manager` and `google_compute_region_instance_group_manager` resources" (GoogleCloudPlatform#9754)"

This reverts commit 24ff156.

* Sort unexpected IPs not present in TF config, update unit test
@SarahFrench SarahFrench deleted the revert-9754-revert-9577-instance-group-manager-interface-reordering branch February 6, 2024 11:01
balanaguharsha pushed a commit to balanaguharsha/magic-modules that referenced this pull request May 2, 2024
…ocks on `google_compute_instance_group_manager` and `google_compute_region_instance_group_manager` resources"" (GoogleCloudPlatform#9758)

* Revert "Revert "Fix permadiff that reorders  `stateful_external_ip` blocks on `google_compute_instance_group_manager` and `google_compute_region_instance_group_manager` resources" (GoogleCloudPlatform#9754)"

This reverts commit 24ff156.

* Sort unexpected IPs not present in TF config, update unit test
pengq-google pushed a commit to pengq-google/magic-modules that referenced this pull request May 21, 2024
…ocks on `google_compute_instance_group_manager` and `google_compute_region_instance_group_manager` resources"" (GoogleCloudPlatform#9758)

* Revert "Revert "Fix permadiff that reorders  `stateful_external_ip` blocks on `google_compute_instance_group_manager` and `google_compute_region_instance_group_manager` resources" (GoogleCloudPlatform#9754)"

This reverts commit 24ff156.

* Sort unexpected IPs not present in TF config, update unit test
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.

3 participants