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

Change stateful_internal|external_ips to Set from List #11441

Draft
wants to merge 2 commits into
base: FEATURE-BRANCH-major-release-6.0.0
Choose a base branch
from

Conversation

askubis
Copy link
Member

@askubis askubis commented Aug 14, 2024

Change stateful_internal|external_ips to Set from List. This solves all issues with ordering. All reordering code removed from IGMs.
fixes: hashicorp/terraform-provider-google#13430

Release Note Template for Downstream PRs (will be copied)

Change resource_compute_instance_group_manager and resource_compute_region_instance_gorup_manager fields: stateful_internal_ips and stateful_external_ips from TypeList to TypeSet. This fixes the ordering issues.

@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.

google provider: Diff ( 4 files changed, 33 insertions(+), 359 deletions(-))
google-beta provider: Diff ( 4 files changed, 33 insertions(+), 359 deletions(-))

Breaking Change(s) Detected

The following breaking change(s) were detected within your pull request.

  • Field stateful_external_ip changed from TypeList to TypeSet on google_compute_instance_group_manager - reference
  • Field stateful_external_ip changed from TypeList to TypeSet on google_compute_region_instance_group_manager - reference
  • Field stateful_internal_ip changed from TypeList to TypeSet on google_compute_instance_group_manager - reference
  • Field stateful_internal_ip changed from TypeList to TypeSet on google_compute_region_instance_group_manager - reference

If you believe this detection to be incorrect please raise the concern with your reviewer.
If you intend to make this change you will need to wait for a major release window.
An override-breaking-change label can be added to allow merging.

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 978
Passed tests: 904
Skipped tests: 72
Affected tests: 2

Click here to see the affected service packages
  • compute

Action taken

Found 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
  • TestAccComputeDisk_storagePoolSpecified
  • TestAccComputeInstance_bootDisk_storagePoolSpecified

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccComputeDisk_storagePoolSpecified[Debug log]
TestAccComputeInstance_bootDisk_storagePoolSpecified[Debug log]

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$


$\textcolor{green}{\textsf{All tests passed!}}$

View the build log or the debug log for each test

@askubis askubis marked this pull request as ready for review August 14, 2024 08:41
@github-actions github-actions bot requested a review from shuyama1 August 14, 2024 08:42
Copy link

Hello! I am a robot. Tests will require approval from a repository maintainer to run.

@shuyama1, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look.

You can help make sure that review is quick by doing a self-review and by running impacted tests locally.

@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.

google provider: Diff ( 4 files changed, 33 insertions(+), 359 deletions(-))
google-beta provider: Diff ( 4 files changed, 33 insertions(+), 359 deletions(-))

Breaking Change(s) Detected

The following breaking change(s) were detected within your pull request.

  • Field stateful_external_ip changed from TypeList to TypeSet on google_compute_instance_group_manager - reference
  • Field stateful_external_ip changed from TypeList to TypeSet on google_compute_region_instance_group_manager - reference
  • Field stateful_internal_ip changed from TypeList to TypeSet on google_compute_instance_group_manager - reference
  • Field stateful_internal_ip changed from TypeList to TypeSet on google_compute_region_instance_group_manager - reference

If you believe this detection to be incorrect please raise the concern with your reviewer.
If you intend to make this change you will need to wait for a major release window.
An override-breaking-change label can be added to allow merging.

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 978
Passed tests: 906
Skipped tests: 72
Affected tests: 0

Click here to see the affected service packages
  • compute

$\textcolor{green}{\textsf{All tests passed!}}$

View the build log

Copy link
Member

@shuyama1 shuyama1 left a comment

Choose a reason for hiding this comment

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

@@ -414,25 +414,6 @@ func TestAccRegionInstanceGroupManager_stoppedSuspendedTargetSize(t *testing.T)
}
<% end -%>

func TestAccRegionInstanceGroupManager_APISideListRecordering(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we want to keep this test to ensure the change from list to set works, like it also wouldn't cause permadiff for multiple stateful_internal_ip/stateful_external_ip attributes configurations.

func flattenStatefulPolicyStatefulInternalIps(d *schema.ResourceData, statefulPolicy *compute.StatefulPolicy) []map[string]interface{} {
if statefulPolicy == nil || statefulPolicy.PreservedState == nil || statefulPolicy.PreservedState.InternalIPs == nil {
return make([]map[string]interface{}, 0, 0)
func flattenStatefulPolicyStatefulInternalIps(statefulPolicy *compute.StatefulPolicy) []map[string]interface{} {
Copy link
Member

Choose a reason for hiding this comment

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

I suspect if this flattener will work as intended. Let's bring back the test first and see it works

Copy link
Member Author

Choose a reason for hiding this comment

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

I did run the tests before removing it.

@shuyama1 shuyama1 added the override-breaking-change Allows a potential breaking change to be merged label Aug 14, 2024
@c2thorn
Copy link
Member

c2thorn commented Aug 16, 2024

looks like this is going to miss the contribution cutoff date for 6.0 (today)

@askubis
Copy link
Member Author

askubis commented Aug 20, 2024

Would you mind also adding an entry in the upgrade guide following step 4 in https://googlecloudplatform.github.io/magic-modules/develop/breaking-changes/make-a-breaking-change/#make-the-change-on-feature-branch-major-release-600.

Thanks!

I do not really understand how it should work for this case - there is nothing to do wrt. upgrade - for the customers the change is invisible (the same configurations that worked will continue to work).

Sorry about missing the cutoff (luckily this change is really not important), I got sick and couldn't work on it.

@github-actions github-actions bot requested a review from shuyama1 August 20, 2024 06:33
Copy link

@shuyama1 This PR has been waiting for review for 3 weekdays. Please take a look! Use the label disable-review-reminders to disable these notifications.

@shuyama1 shuyama1 removed the override-breaking-change Allows a potential breaking change to be merged label Aug 22, 2024
@shuyama1
Copy link
Member

The 6.0 release cut is complete, so I'll move this PR to draft for now. Please feel free to reach out if you have any questions regarding this change. If you decide to proceed with this update, it will need to wait until the next major release, but I’m happy to assist along the way.

@shuyama1 shuyama1 marked this pull request as draft August 22, 2024 17:28
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.

5 participants