magic-modules icon indicating copy to clipboard operation
magic-modules copied to clipboard

Change advertised_ip_ranges fields in google_compute_router_peer and google_compute_router resources to Sets

Open LucaPrete opened this issue 1 year ago • 46 comments

[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

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.

LucaPrete avatar May 31 '24 13:05 LucaPrete

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.

github-actions[bot] avatar May 31 '24 13:05 github-actions[bot]

@SarahFrench I opened this because I wasn't able to work on https://github.com/GoogleCloudPlatform/magic-modules/pull/10118. In case the @Maarc-D is able to move forward with the other I'm happy to abandon this one.

I applied the changes you recommended and it's well understood this is a breaking change that can be merged in 6.0.0.

LucaPrete avatar May 31 '24 13:05 LucaPrete

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 ( 2 files changed, 25 insertions(+), 18 deletions(-)) google-beta provider: Diff ( 2 files changed, 25 insertions(+), 18 deletions(-)) terraform-google-conversion: Diff ( 1 file changed, 1 insertion(+))

Errors

google provider:

  • The diff processor failed to build. This is usually due to the downstream provider failing to compile.

google-beta provider:

  • The diff processor failed to build. This is usually due to the downstream provider failing to compile.

modular-magician avatar May 31 '24 14:05 modular-magician

Tests analytics

Total tests: 0 Passed tests: 0 Skipped tests: 0 Affected tests: 0

Click here to see the affected service packages
  • compute

$\textcolor{red}{\textsf{Errors occurred during REPLAYING mode. Please fix them to complete your PR.}}$ View the build log

modular-magician avatar May 31 '24 14:05 modular-magician

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

SarahFrench avatar May 31 '24 14:05 SarahFrench

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 https://github.com/GoogleCloudPlatform/magic-modules/pull/10845#issuecomment-2142352486, or unsubscribe https://github.com/notifications/unsubscribe-auth/AARY7UABU6WZF7G46WHBOCLZFCCBZAVCNFSM6AAAAABIS3YMHGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNBSGM2TENBYGY . You are receiving this because you authored the thread.Message ID: @.***>

LucaPrete avatar May 31 '24 14:05 LucaPrete

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

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

LucaPrete avatar Jun 04 '24 06:06 LucaPrete

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 ( 3 files changed, 39 insertions(+), 36 deletions(-)) google-beta provider: Diff ( 3 files changed, 39 insertions(+), 36 deletions(-)) terraform-google-conversion: Diff ( 1 file changed, 1 insertion(+))

Errors

google provider:

  • The diff processor failed to build. This is usually due to the downstream provider failing to compile.

google-beta provider:

  • The diff processor failed to build. This is usually due to the downstream provider failing to compile.

modular-magician avatar Jun 04 '24 06:06 modular-magician

Tests analytics

Total tests: 0 Passed tests: 0 Skipped tests: 0 Affected tests: 0

Click here to see the affected service packages
  • compute

$\textcolor{red}{\textsf{Errors occurred during REPLAYING mode. Please fix them to complete your PR.}}$ View the build log

modular-magician avatar Jun 04 '24 06:06 modular-magician

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 ( 3 files changed, 39 insertions(+), 36 deletions(-)) google-beta provider: Diff ( 3 files changed, 39 insertions(+), 36 deletions(-)) terraform-google-conversion: Diff ( 1 file changed, 1 insertion(+))

Errors

google provider:

  • The diff processor failed to build. This is usually due to the downstream provider failing to compile.

google-beta provider:

  • The diff processor failed to build. This is usually due to the downstream provider failing to compile.

modular-magician avatar Jun 04 '24 08:06 modular-magician

Tests analytics

Total tests: 0 Passed tests: 0 Skipped tests: 0 Affected tests: 0

Click here to see the affected service packages
  • compute

$\textcolor{red}{\textsf{Errors occurred during REPLAYING mode. Please fix them to complete your PR.}}$ View the build log

modular-magician avatar Jun 04 '24 08:06 modular-magician

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

github-actions[bot] avatar Jun 04 '24 09:06 github-actions[bot]

@SarahFrench @roaks3 I see in my local tests that something is failing in the generated router code:

make testacc TEST=./google-beta/services/compute TESTARGS='-run=
TestAccComputeRouter_routerBasicExample'
==> Checking that code complies with gofmt requirements...
go vet
TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google-beta/services/compute -v -run=TestAccComputeRouter_routerBasicExample -timeout 240m -ldflags="-X=github.com/hashicorp/terraform-provider-google-beta/version.ProviderVersion=acc"
=== RUN   TestAccComputeRouter_routerBasicExample
=== PAUSE TestAccComputeRouter_routerBasicExample
=== CONT  TestAccComputeRouter_routerBasicExample
panic: interface conversion: interface {} is *schema.Set, not []interface {}

goroutine 307 [running]:
github.com/hashicorp/terraform-provider-google-beta/google-beta/services/compute.resourceComputeRouterCustomDiff({0x0?, 0x0?}, 0x0?, {0x0?, 0x0?})
	/Users/lucaprete/go/src/github.com/hashicorp/terraform-provider-google-beta/google-beta/services/compute/resource_compute_router.go:42 +0x1aa
github.com/hashicorp/terraform-provider-google-beta/google-beta/services/compute.ResourceComputeRouter.All.func1({0x56e9e20, 0xc00104a780}, 0x4de4eda?, {0x4d9f5c0, 0xc00026f000})
	/Users/lucaprete/go/pkg/mod/github.com/hashicorp/terraform-plugin-sdk/[email protected]/helper/customdiff/compose.go:52 +0xd2
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.schemaMap.Diff(0xc000981380, {0x56e9e20, 0xc00104a780}, 0xc0011b8d00, 0xc001ba1770, 0xc000480d40, {0x4d9f5c0, 0xc00026f000}, 0x0)
	/Users/lucaprete/go/pkg/mod/github.com/hashicorp/terraform-plugin-sdk/[email protected]/helper/schema/schema.go:698 +0x4b4
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*Resource).SimpleDiff(0x56ea088?, {0x56e9e20?, 0xc00104a780?}, 0xc0011b8d00, 0x47711c0?, {0x4d9f5c0?, 0xc00026f000?})
	/Users/lucaprete/go/pkg/mod/github.com/hashicorp/terraform-plugin-sdk/[email protected]/helper/schema/resource.go:962 +0xd5
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*GRPCProviderServer).PlanResourceChange(0xc000ff6e70, {0x56e9e20?, 0xc00104a6c0?}, 0xc001ba1630)
	/Users/lucaprete/go/pkg/mod/github.com/hashicorp/terraform-plugin-sdk/[email protected]/helper/schema/grpc_provider.go:798 +0x9b6
github.com/hashicorp/terraform-plugin-mux/tf5muxserver.(*muxServer).PlanResourceChange(0xc00015f570, {0x56e9e20?, 0xc00104a420?}, 0xc001ba1630)
	/Users/lucaprete/go/pkg/mod/github.com/hashicorp/[email protected]/tf5muxserver/mux_server_PlanResourceChange.go:73 +0x2ad
github.com/hashicorp/terraform-plugin-go/tfprotov5/tf5server.(*server).PlanResourceChange(0xc0004a8dc0, {0x56e9e20?, 0xc001c7fbf0?}, 0xc000e609a0)
	/Users/lucaprete/go/pkg/mod/github.com/hashicorp/[email protected]/tfprotov5/tf5server/server.go:811 +0x3d0
github.com/hashicorp/terraform-plugin-go/tfprotov5/internal/tfplugin5._Provider_PlanResourceChange_Handler({0x4d45080?, 0xc0004a8dc0}, {0x56e9e20, 0xc001c7fbf0}, 0xc002709c80, 0x0)
	/Users/lucaprete/go/pkg/mod/github.com/hashicorp/[email protected]/tfprotov5/internal/tfplugin5/tfplugin5_grpc.pb.go:500 +0x169
google.golang.org/grpc.(*Server).processUnaryRPC(0xc001020600, {0x56e9e20, 0xc001c7fb60}, {0x56f5aa8, 0xc00053de00}, 0xc001aea360, 0xc001068840, 0x754c080, 0x0)
	/Users/lucaprete/go/pkg/mod/google.golang.org/[email protected]/server.go:1369 +0xe23
google.golang.org/grpc.(*Server).handleStream(0xc001020600, {0x56f5aa8, 0xc00053de00}, 0xc001aea360)
	/Users/lucaprete/go/pkg/mod/google.golang.org/[email protected]/server.go:1780 +0x1016
google.golang.org/grpc.(*Server).serveStreams.func2.1()
	/Users/lucaprete/go/pkg/mod/google.golang.org/[email protected]/server.go:1019 +0x8b
created by google.golang.org/grpc.(*Server).serveStreams.func2 in goroutine 245
	/Users/lucaprete/go/pkg/mod/google.golang.org/[email protected]/server.go:1030 +0x135
FAIL	github.com/hashicorp/terraform-provider-google-beta/google-beta/services/compute	4.509s
FAIL
make: *** [testacc] Error 1

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?

LucaPrete avatar Jun 04 '24 13:06 LucaPrete

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 ( 3 files changed, 61 insertions(+), 59 deletions(-)) google-beta provider: Diff ( 3 files changed, 61 insertions(+), 59 deletions(-)) terraform-google-conversion: Diff ( 1 file changed, 1 insertion(+))

Breaking Change(s) Detected

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

  • Field advertised_ip_ranges changed from TypeList to TypeSet on google_compute_router_peer - reference
  • Field bgp.advertised_ip_ranges changed from TypeList to TypeSet on google_compute_router - 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 avatar Jun 04 '24 13:06 modular-magician

Tests analytics

Total tests: 78 Passed tests: 18 Skipped tests: 58 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
TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy|TestAccComputeRegionAutoscaler_update

Get to know how VCR tests work

modular-magician avatar Jun 04 '24 14:06 modular-magician

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$ TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy[Error message] [Debug log] TestAccComputeRegionAutoscaler_update[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$ View the build log or the debug log for each test

modular-magician avatar Jun 04 '24 14:06 modular-magician

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 ( 3 files changed, 61 insertions(+), 59 deletions(-)) google-beta provider: Diff ( 3 files changed, 61 insertions(+), 59 deletions(-)) terraform-google-conversion: Diff ( 1 file changed, 1 insertion(+))

Breaking Change(s) Detected

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

  • Field advertised_ip_ranges changed from TypeList to TypeSet on google_compute_router_peer - reference
  • Field bgp.advertised_ip_ranges changed from TypeList to TypeSet on google_compute_router - 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 avatar Jun 05 '24 12:06 modular-magician

Tests analytics

Total tests: 80 Passed tests: 19 Skipped tests: 60 Affected tests: 1

Click here to see the affected service packages
  • compute

Action taken

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

Get to know how VCR tests work

modular-magician avatar Jun 05 '24 12:06 modular-magician

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 ( 3 files changed, 59 insertions(+), 57 deletions(-)) google-beta provider: Diff ( 3 files changed, 59 insertions(+), 57 deletions(-)) terraform-google-conversion: Diff ( 1 file changed, 1 insertion(+))

Breaking Change(s) Detected

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

  • Field advertised_ip_ranges changed from TypeList to TypeSet on google_compute_router_peer - reference
  • Field bgp.advertised_ip_ranges changed from TypeList to TypeSet on google_compute_router - 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 avatar Jun 05 '24 12:06 modular-magician

Tests analytics

Total tests: 80 Passed tests: 19 Skipped tests: 60 Affected tests: 1

Click here to see the affected service packages
  • compute

Action taken

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

Get to know how VCR tests work

modular-magician avatar Jun 05 '24 12:06 modular-magician

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$ TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$ View the build log or the debug log for each test

modular-magician avatar Jun 05 '24 12:06 modular-magician

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$ TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$ View the build log or the debug log for each test

modular-magician avatar Jun 05 '24 13:06 modular-magician

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 ( 3 files changed, 59 insertions(+), 57 deletions(-)) google-beta provider: Diff ( 3 files changed, 59 insertions(+), 57 deletions(-)) terraform-google-conversion: Diff ( 1 file changed, 1 insertion(+))

Breaking Change(s) Detected

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

  • Field advertised_ip_ranges changed from TypeList to TypeSet on google_compute_router_peer - reference
  • Field bgp.advertised_ip_ranges changed from TypeList to TypeSet on google_compute_router - 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 avatar Jun 07 '24 08:06 modular-magician

Tests analytics

Total tests: 80 Passed tests: 19 Skipped tests: 60 Affected tests: 1

Click here to see the affected service packages
  • compute

Action taken

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

Get to know how VCR tests work

modular-magician avatar Jun 07 '24 09:06 modular-magician

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$ TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$ View the build log or the debug log for each test

modular-magician avatar Jun 07 '24 09:06 modular-magician

@GoogleCloudPlatform/terraform-team This PR has been waiting for review for 1 week. Please take a look! Use the label disable-review-reminders to disable these notifications.

github-actions[bot] avatar Jun 07 '24 09:06 github-actions[bot]

@GoogleCloudPlatform/terraform-team @roaks3 This PR has been waiting for review for 2 weeks. Please take a look! Use the label disable-review-reminders to disable these notifications.

github-actions[bot] avatar Jun 14 '24 09:06 github-actions[bot]

This PR is approved and has been waiting for merge for 1 week. Is it ready to merge? Use the label disable-review-reminders to disable these notifications.

github-actions[bot] avatar Jun 27 '24 09:06 github-actions[bot]

Follow-up note: this should be changed to point to the feature branch since it is a breaking change (see https://googlecloudplatform.github.io/magic-modules/develop/breaking-changes/make-a-breaking-change/)

roaks3 avatar Jun 27 '24 13:06 roaks3

Follow-up note: this should be changed to point to the feature branch since it is a breaking change (see https://googlecloudplatform.github.io/magic-modules/develop/breaking-changes/make-a-breaking-change/)

Thanks @roaks3 I just rebased, hopefully on the correct branch. Please, let me know if there's anything else I should do.

LucaPrete avatar Jun 28 '24 16:06 LucaPrete