Change advertised_ip_ranges fields in google_compute_router_peer and google_compute_router resources to Sets

[Maarc-D]

Description

Linked to hashicorp/terraform-provider-google#9353

Release Note Template for Downstream PRs (will be copied)

compute: changed the `advertised_ip_ranges` fields in both `google_compute_router_peer` and `google_compute_router` resources to be sets. This will reduce diffs due to re-ordering.

ToDo

Need to test it to be sure their is no impacts with https://registry.terraform.io/providers/public-cloud-wl/google/5.17.4 from public-cloud-wl/terraform-provider-google@1a4d7e6

[github-actions]

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

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

[SarahFrench]

Just a heads up - this would be a breaking change for the field (see our guidance on this here) and would need to be included in an upcoming major release. We make exceptions if the 'breaking change' fixes something that is broken for all users but I don't think that is the case here.

[Maarc-D]

@SarahFrench In fact I would need to update the label because I tested It is not breaking change it will may say that it want to reorder everything so 1change but on apply everything is fine, tested and approved on my side, I will use my fork in the meaning time this one can be merge

What do you think I need to put, keep breaking-change or put something else like : bug ?

[SarahFrench]

@Maarc-D sorry for the delay, this PR slipped off my radar!

We'd need to double check what users will experience when upgrading from a previous version of the provider to a version including this change. Even if there's a new diff that's resolved with a single apply, navigating that diff might not work well for people with more complex configurations.

I'll trigger tests to run: /gcbrun

---

Edit: there is currently a build error occurring:

bundler: failed to load command: compiler.rb (compiler.rb)
/usr/local/lib/ruby/3.1.0/psych/class_loader.rb:99:in `find': Tried to load unspecified class: Api::Type::Set (Psych::DisallowedClass)

[Maarc-D]

@Maarc-D sorry for the delay, this PR slipped off my radar!

We'd need to double check what users will experience when upgrading from a previous version of the provider to a version including this change. Even if there's a new diff that's resolved with a single apply, navigating that diff might not work well for people with more complex configurations.

I'll trigger tests to run: /gcbrun

no problem I putted back as draft because we experienced few cases where this one does not resolve completely the idempotency issue :/ sometime still prompt more changes that it should. No working impact but it is on this cases still hard to make review before apply as it is now with list.

[LucaPrete]

Hello @SarahFrench this seems a huge pain for many of our large Google customers. Is there anything we can do to speed up the development/review? Happy to help!

BTW should we start to "undraft" this? :)

Thanks!

[SarahFrench]

Changing an Array to a Set is a breaking change and will need to be part of a major release. The 6.0.0 major release is being planned currently and this PRs changes could be included then. Should we choose this option?

Alternatively, you can return the field to being an Array/list and then implement a custom flattener function for the field that reorders the API response to match the users configuration and minimise unnecessary diffs. There is new guidance about how to do this in our contribution guide and this PR is an example of the new helper functions being used: #10429 . If you choose this option please mark the PR as ready for review once you want feedback!

[SarahFrench]

This is currently causing a build error - to specify a field as a Set in these yaml files you will need to revert it back to !ruby/object:Api::Type::Array and then add the property is_set: true to that field. However if you do that please refer to my previous review where making the change to a Set can only be in a major version.

[SarahFrench]

Hi @LucaPrete - My review comments above describes the two routes forward for this PR/the original issue. @Maarc-D hasn't had enough time to respond to the review yet, but if this PR was to become stale then someone could open another PR informed from the discussion here.

[LucaPrete]

Thank you @SarahFrench ! It makes absolutely sense to me.

I'd proceed with both paths in parallel:

• I opened a new PR that uses the flatten function. Waiting for the tests to complete. I'll add you in cc.

• @Maarc-D can you please address the comments of @SarahFrench. We can then think to merge this once we cut a new major? @SarahFrench do we have timelines for it (assuming the PR will be complete)?

[SarahFrench]

@Maarc-D The 6.0.0 major release is coming up later this year. Do you want to proceed with this PR for that release?

[LucaPrete]

@SarahFrench @Maarc-D I opened this one since I've seen this wasn't moving forward and I wasn't able to commit to it. Please let me know if you change your mind and you're somehow able to finish this instead. In that case I'll abandon it.

[SarahFrench]

Given the lack of response I'll close this PR as stale, and we can move forward with the replacement PR