-
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
Change advertised_ip_ranges fields in google_compute_router_peer and google_compute_router resources to Sets #10845
Conversation
Hello! I am a robot. Tests will require approval from a repository maintainer to run. @roaks3, 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. |
48ab804
to
e9d0374
Compare
@SarahFrench I opened this because I wasn't able to work on I applied the changes you recommended and it's well understood this is a breaking change that can be merged in 6.0.0. |
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.
Errors
|
Tests analyticsTotal tests: Click here to see the affected service packages
|
I'll leave review to @roaks3. If this PR addresses ordering diffs on this field by converting it to a Set in 6.0.0 then this PR will also need to remove the ordering logic added in https://github.com/GoogleCloudPlatform/magic-modules/pull/10572/files |
Ah yes! Good point. I’ll do that.
Il giorno ven 31 mag 2024 alle 16:26 Sarah French ***@***.***>
ha scritto:
… I'll leave review to @roaks3 <https://github.com/roaks3>.
If this PR addresses ordering diffs on this field by converting it to a
Set in 6.0.0 then this PR will also need to remove the ordering logic added
in https://github.com/GoogleCloudPlatform/magic-modules/pull/10572/files
—
Reply to this email directly, view it on GitHub
<#10845 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AARY7UABU6WZF7G46WHBOCLZFCCBZAVCNFSM6AAAAABIS3YMHGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNBSGM2TENBYGY>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
@SarahFrench done! Please, check if it looks good to you. @roaks3 this should be ready (I know you won't be able to merge until the release...) |
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.
Errors
|
Tests analyticsTotal tests: Click here to see the affected service packages
|
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.
Errors
|
Tests analyticsTotal tests: Click here to see the affected service packages
|
This PR has been waiting for review for 2 weekdays. Please take a look! Use the label |
eb26506
to
86d7eba
Compare
@SarahFrench @roaks3 I see in my local tests that something is failing in the generated router code:
On the other hand, here I see some test failures that seem completely unrelated to this change. I'll try to re-trigger them and see if at least I get something consistent with what I get on my device. Any idea what I am missing, given it's just a one line modification? |
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.
Breaking Change(s) DetectedThe following breaking change(s) were detected within your pull request.
If you believe this detection to be incorrect please raise the concern with your reviewer. |
Tests analyticsTotal tests: Click here to see the affected service packages
Action takenFound 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccComputeInstanceNetworkIntefaceWithSecurityPolicy|TestAccComputeRegionAutoscaler_update |
|
86d7eba
to
86a3457
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.
Breaking Change(s) DetectedThe following breaking change(s) were detected within your pull request.
If you believe this detection to be incorrect please raise the concern with your reviewer. |
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 testsTestAccComputeInstanceNetworkIntefaceWithSecurityPolicy |
86a3457
to
1011234
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.
Breaking Change(s) DetectedThe following breaking change(s) were detected within your pull request.
If you believe this detection to be incorrect please raise the concern with your reviewer. |
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 testsTestAccComputeInstanceNetworkIntefaceWithSecurityPolicy |
|
|
Thanks @roaks3 I just rebased, hopefully on the correct branch. Please, let me know if there's anything else I should do. |
I was also hoping the rebase would have re-triggered the tests but it doesn't seem the case. Can you please help me re-running them? I remember I wasn't able to trigger them on my own... |
/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.
Breaking Change(s) DetectedThe following breaking change(s) were detected within your pull request.
If you believe this detection to be incorrect please raise the concern with your reviewer. |
Tests analyticsTotal tests: 79 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 tests
|
|
I can see there's a panic affecting the TGC unit tests, FYI @roaks3 |
Yea, I had missed it before but it looks like the panic has always been present. I'm seeing it occur from a nil pointer on this line https://github.com/modular-magician/terraform-provider-google-beta/compare/auto-pr-10845-old..auto-pr-10845#diff-d2eb6a6a2712aaced589de3439e521d9a17895670b7118da797a5e90681285b6R42. @LucaPrete it's not immediately obvious to me what is wrong, so you may need to step through the code and see where the nil could be coming from with this change. I would suspect there is an edge case that was simply missed, but it could also be related to the set behaving differently in some way. |
@GoogleCloudPlatform/terraform-team @roaks3 This PR has been waiting for review for 3 weeks. Please take a look! Use the label |
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.
Quiet review notification
@LucaPrete, this PR is waiting for action from you. Please address any comments or change requests, or re-request review from a core reviewer if no action is required. If no action is taken, this PR will be closed in 28 days. This notification can be disabled with the |
@LucaPrete, this PR is waiting for action from you. Please address any comments or change requests, or re-request review from a core reviewer if no action is required. If no action is taken, this PR will be closed in 14 days. This notification can be disabled with the |
@LucaPrete @c2thorn I'm noticing this a bit late, but this PR was targeting 6.0.0, which I think is about to be closed (if it isn't already). The last message was mentioning a panic that needs to be fixed, so I know the change isn't ready in its current state, but wanted to raise awareness: if we can't get this in now, the long-term solution proposed here might not be available until 7.0.0 next year. |
The window is closed unfortunately, we need time to sync the branches and test |
Hello folks,
It seems the panic comes from something deeper into the code. It’s
something I haven’t
time to look into….on the other hand, I want to underline how this change
is crucial for many of our customers. Happy to contribute and make thing
happen in no time, but I’ll definitely need help from somebody from the
core dev team as well.
Please, let me know how you’d like to proceed.
Thanks!
Il giorno mar 20 ago 2024 alle 18:42 Cameron Thornton <
***@***.***> ha scritto:
… @LucaPrete <https://github.com/LucaPrete> @c2thorn
<https://github.com/c2thorn> I'm noticing this a bit late, but this PR
was targeting 6.0.0, which I think is about to be closed (if it isn't
already). The last message was mentioning a panic that needs to be fixed,
so I know the change isn't ready in its current state, but wanted to raise
awareness: if we can't get this in now, the long-term solution proposed
here might not be available until 7.0.0 next year.
The window is closed unfortunately, we need time to sync the branches and
test
—
Reply to this email directly, view it on GitHub
<#10845 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AARY7UB5MV4VMWV77EABSMTZSNWXZAVCNFSM6AAAAABIS3YMHGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOJZGI4TQMBQGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
There is simply no time left, the contribution window for 6.0 closed Friday, and the 6.0 branch is effectively frozen so the major release as a whole can be completed in time and tested thoroughly. I've already declined other changes that missed the window that were in a more ready state than this is. If there is significant enough prioritization, we can discuss internally after 6.0 is released. |
@roaks3 This PR has been waiting for review for 3 weekdays. Please take a look! Use the label |
@GoogleCloudPlatform/terraform-team @roaks3 This PR has been waiting for review for 1 week. Please take a look! Use the label |
@GoogleCloudPlatform/terraform-team @roaks3 This PR has been waiting for review for 2 weeks. Please take a look! Use the label |
@GoogleCloudPlatform/terraform-team @roaks3 This PR has been waiting for review for 3 weeks. Please take a look! Use the label |
@GoogleCloudPlatform/terraform-team @roaks3 This PR has been waiting for review for 4 weeks. Please take a look! Use the label |
@GoogleCloudPlatform/terraform-team @roaks3 This PR has been waiting for review for 5 weeks. Please take a look! Use the label |
@GoogleCloudPlatform/terraform-team @roaks3 This PR has been waiting for review for 6 weeks. Please take a look! Use the label |
@GoogleCloudPlatform/terraform-team @roaks3 This PR has been waiting for review for 7 weeks. Please take a look! Use the label |
@GoogleCloudPlatform/terraform-team @roaks3 This PR has been waiting for review for 8 weeks. Please take a look! Use the label |
[breaking change for 6.0.0 major release]
Change advertised_ip_ranges fields in google_compute_router_peer and google_compute_router resources to Sets.
Fixes hashicorp/terraform-provider-google/issues/9353