[#9353] Make cloud router sort advertised_ip_ranges in state by config order #10572

LucaPrete · 2024-05-01T11:31:06Z

Use the new function introduced in #10410 to order advertised_ip_ranges properties according to the order in config.

Pre-6.0.0 fix for hashicorp/terraform-provider-google/issues/9353.

Release Note Template for Downstream PRs (will be copied)

compute: fixed permadiff in ordering of `advertised_ip_ranges.range` field on `google_compute_router` resource.

…s in state by config order

github-actions · 2024-05-01T11:31:40Z

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.

modular-magician · 2024-05-01T11:41:41Z

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, 16 insertions(+), 14 deletions(-))
google-beta provider: Diff ( 2 files changed, 16 insertions(+), 14 deletions(-))

LucaPrete · 2024-05-01T11:46:50Z

@SarahFrench I can't understand from our CloudBuild logs what's failing. Do you find any useful log anywhere?

modular-magician · 2024-05-01T11:50:21Z

Tests analytics

Total tests: 78
Passed tests: 20
Skipped tests: 58
Affected tests: 0

Click here to see the affected service packages

compute

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

LucaPrete · 2024-05-01T11:53:36Z

Once this gets passes the tests and gets reviewed, I can also put (here or through another PR) the same flatten function for advertised_ip_ranges in google_compute_router_peer here.

SarahFrench

I'll look in more depth tomorrow, but in case it helps unblock you the error is:

Error compiling file: /.../GoogleCloudPlatform/magic-modules/mmv1/templates/terraform/custom_flatten/compute_router_range.go.erb
Error compiling file: /.../GoogleCloudPlatform/magic-modules/mmv1/templates/terraform/flatten_property_method.erb
Error compiling file: /.../GoogleCloudPlatform/magic-modules/mmv1/templates/terraform/flatten_property_method.erb
Error compiling file: /.../GoogleCloudPlatform/magic-modules/mmv1//templates/terraform/resource.erb
bundler: failed to load command: compiler.rb (compiler.rb)
(erb):2:in `compile_template': undefined local variable or method `range' for #<Provider::Terraform:0x0000000104c75fb0 @api=#<Api::Product:0x0000000104e257e8 @name="Compute", @display_name="Compute Engine", @version

LucaPrete · 2024-05-02T05:55:34Z

I'll look in more depth tomorrow, but in case it helps unblock you the error is:


Error compiling file: /.../GoogleCloudPlatform/magic-modules/mmv1/templates/terraform/custom_flatten/compute_router_range.go.erb

Error compiling file: /.../GoogleCloudPlatform/magic-modules/mmv1/templates/terraform/flatten_property_method.erb

Error compiling file: /.../GoogleCloudPlatform/magic-modules/mmv1/templates/terraform/flatten_property_method.erb

Error compiling file: /.../GoogleCloudPlatform/magic-modules/mmv1//templates/terraform/resource.erb

bundler: failed to load command: compiler.rb (compiler.rb)

(erb):2:in `compile_template': undefined local variable or method `range' for #<Provider::Terraform:0x0000000104c75fb0 @api=#<Api::Product:0x0000000104e257e8 @name="Compute", @display_name="Compute Engine", @version

@SarahFrench thank you! With the message you provided is all clear but I'm wondering where you found it. I do have access to CB logs but I still can't find any.

SarahFrench · 2024-05-02T12:54:37Z

I'm wondering where you found it. I do have access to CB logs but I still can't find any.

I think the logs in Cloud Build omitted a bunch of log lines. The logs show ... omitting 1869281 bytes ... in the place where that error would be shown.

I investigating the error by generating the provider from your PR's branch on my laptop, following this guidance: https://googlecloudplatform.github.io/magic-modules/get-started/generate-providers/

mmv1/templates/terraform/custom_flatten/compute_router_range.go.erb

modular-magician · 2024-05-02T14:27:13Z

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, 28 insertions(+), 3 deletions(-))
google-beta provider: Diff ( 2 files changed, 28 insertions(+), 3 deletions(-))

modular-magician · 2024-05-02T14:42:49Z

Tests analytics

Total tests: 685
Passed tests: 613
Skipped tests: 72
Affected tests: 0

Click here to see the affected service packages

compute

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

SarahFrench · 2024-05-02T15:26:26Z

mmv1/templates/terraform/custom_flatten/compute_router_range.go.erb

+		})
+	}
+	configData := []map[string]interface{}{}
+	for _, item := range d.Get("advertisedIpRanges.0.range").([]interface{}) {


The error mentioned here is due to the problem below, and I think the test that first encountered the error was TestAccComputeRouter_noRegion

panic: interface conversion: interface {} is string, not []interface {} goroutine 4529793 [running]: github.com/hashicorp/terraform-provider-google-beta/google-beta/services/compute.flattenComputeRouterBgpAdvertisedIpRangesRange({0x3e85f60?, 0xc016c89f40?}, 0x4763f12?, 0x5?) /go/src/github.com/modular-magician/terraform-provider-google-beta/google-beta/services/compute/resource_compute_router.go:654 +0x4e9

I attached my comment to the wrong line - L654 mentioned in the panic is here

@SarahFrench apologies if I disappeared but I'm in and out working at other customers.

I'm not able to debug this code now as

I can't run the test on my computer. Both the GA and the beta provider seem to be failing in this exact moment

The lines reported in the build log and in the diff don't seem to correspond

I'd probably need to log some things here and there to be able to say more. As soon as I'll find again some time I'll try again and let you know. In the meantime, if you see anything obvious, please let me know.

Thanks!

The lines reported in the build log and in the diff don't seem to correspond

The panic output I posted above refers to the code in the provider that's built from the Magic Modules repo. If you click the links on Diff Report posts on the PR it'll show the resulting provider code from your PR and the lines should correspond. A useful bit of context is that the tests that run on PRs use the google-beta code generated from the PR, so if there are differences in a file between the GA and Beta versions of the provider that could result in some code line confusion too.

No worries about juggling other tasks, that's totally understandable!

@SarahFrench there's something I'm clearly not understanding here :)

If I build the GA provider on my computer and run TestAccComputeRouter_noRegion I never get an error.
I guess the first thing I wish is being able to reproduce it in my local environment, so I can debug it and understand what's going on.

You said you got the error during your local build. May I ask you how you got to the error exactly?

FYI my commands:

make provider VERSION=ga OUTPUT_PATH="$GOPATH/src/github.com/hashicorp/terraform-provider-google" PRODUCT=compute cd $GOPATH/src/github.com/hashicorp/terraform-provider-google make testacc TEST=./google/services/compute TESTARGS='-run=TestAccComputeRouter_noRegion'

What am I missing? Also, are you a Googler? If you are, can you please reach me in chat? Thanks

👋 I'm a HashiCorp employee so I'm afraid this PR is the best place for us to talk.

If I build the GA provider on my computer and run TestAccComputeRouter_noRegion I never get an error.

I'll have a go running the Compute tests against your PR, the output where the panic was reported may have been misleading about which test triggered the problem. I'll report back!

You said you got the error during your local build. May I ask you how you got to the error exactly?

Do you mean when I helped with the build error here? That build error is separate to the panic that we're now seeing when the tests are running using the built provider code.

modular-magician · 2024-05-06T09:16:08Z

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, 16 insertions(+), 14 deletions(-))
google-beta provider: Diff ( 2 files changed, 16 insertions(+), 14 deletions(-))

modular-magician · 2024-05-06T09:26:07Z

Tests analytics

Total tests: 135
Passed tests: 77
Skipped tests: 58
Affected tests: 0

Click here to see the affected service packages

compute

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

SarahFrench

Reviewing again as GHA automation re-requested review from me, but see previous discussion for next steps

github-actions · 2024-05-21T09:39:59Z

@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 disable-automatic-closure label.

SarahFrench · 2024-05-21T10:38:47Z

I'm running individual tests in TeamCity to test this PR (see #10572 (comment)) but I'll re-trigger tests on this PR directly too:

/gcbrun

modular-magician · 2024-05-21T10:48:54Z

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, 16 insertions(+), 14 deletions(-))
google-beta provider: Diff ( 2 files changed, 16 insertions(+), 14 deletions(-))

modular-magician · 2024-05-21T10:58:03Z

Tests analytics

Total tests: 78
Passed tests: 20
Skipped tests: 58
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

mmv1/templates/terraform/custom_flatten/compute_router_range.go.erb

SarahFrench · 2024-05-22T10:53:54Z

Tests that triggered the panic in our testing environment when I ran all Compute tests against this PR:

TestAccComputeRouter_addAndUpdateIdentifierRangeBgp
TestAccComputeRouter_full
TestAccComputeRouter_routerBasicExample
TestAccComputeRouter_update

All the panics point to google-beta/services/compute/resource_compute_router.go:649.

The data coming from the API before the panic is a GET request to get the resource's data:

Expand me to see the API interaction

---[ REQUEST ]---------------------------------------
GET /compute/beta/projects/PROJECT_ID/regions/us-central1/routers/tf-test-router-21774xuy3t?alt=json HTTP/1.1
Host: compute.googleapis.com
User-Agent: Terraform/1.8.3 (+https://www.terraform.io) Terraform-Plugin-SDK/2.33.0 terraform-provider-google-beta/dev
Content-Type: application/json
Accept-Encoding: gzip


-----------------------------------------------------
2024/05/21 15:58:25 [DEBUG] Google API Response Details:
---[ RESPONSE ]--------------------------------------
HTTP/2.0 200 OK
Alt-Svc: h3=":443"; ma=2592000,h3-29=":443"; ma=2592000
Cache-Control: private
Content-Type: application/json; charset=UTF-8
Date: Tue, 21 May 2024 15:58:25 GMT
Etag: c3IxE2a5zjtzwTTfPRfkArR717Q=/89RsJ8v3AuKyB9cl514K1xCfAH0=
Server: ESF
Vary: Origin
Vary: X-Origin
Vary: Referer
X-Content-Type-Options: nosniff
X-Frame-Options: SAMEORIGIN
X-Xss-Protection: 0

{
  "kind": "compute#router",
  "id": "6599853179148532362",
  "creationTimestamp": "2024-05-21T08:57:25.107-07:00",
  "name": "tf-test-router-21774xuy3t",
  "description": "",
  "region": "https://www.googleapis.com/compute/beta/projects/PROJECT_ID/regions/us-central1",
  "network": "https://www.googleapis.com/compute/beta/projects/PROJECT_ID/global/networks/tf-test-router-21774xuy3t-net",
  "bgp": {
    "asn": 64514,
    "advertiseMode": "CUSTOM",
    "advertisedGroups": [
"ALL_SUBNETS"
    ],
    "advertisedIpRanges": [
      {
        "range": "1.2.3.4",
        "description": ""
      },
      {
        "range": "6.7.0.0/16",
        "description": ""
      }
    ],
    "keepaliveInterval": 25
  },
  "selfLink": "https://www.googleapis.com/compute/beta/projects/PROJECT_ID/regions/us-central1/routers/tf-test-router-21774xuy3t",
  "encryptedInterconnectRouter": false
}

SarahFrench

Could you please take a look at my suggested code change, and if you're able could you please try running the TestAccComputeRouter_full test locally?

mmv1/templates/terraform/custom_flatten/compute_router_range.go.erb

…o.erb Co-authored-by: Sarah French <15078782+SarahFrench@users.noreply.github.com>

LucaPrete · 2024-05-23T11:14:56Z

@SarahFrench thanks a lot! I committed your proposed change. Your help has been key! Let's see if we have more tests complaining now...

modular-magician · 2024-05-23T11:24:58Z

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, 18 insertions(+), 14 deletions(-))
google-beta provider: Diff ( 2 files changed, 18 insertions(+), 14 deletions(-))

modular-magician · 2024-05-23T11:46:29Z

Tests analytics

Total tests: 933
Passed tests: 861
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

SarahFrench

I reproduced the original issue from hashicorp/terraform-provider-google#9353 locally with the latest google provider release and saw the reordering diff, and with the provider built from this PR the diff isn't there anymore! 🎉

Given the existing tests are passing ok and my manual test I'm happy to merge

SarahFrench · 2024-05-23T16:33:06Z

Thanks for your work on this @LucaPrete!

…s in state by config order (GoogleCloudPlatform#10572) Co-authored-by: Luca Prete <lucaprete@google.com> Co-authored-by: Sarah French <15078782+SarahFrench@users.noreply.github.com>

[GoogleCloudPlatform#9353] Make cloud router sort advertised_ip_range…

680b916

…s in state by config order

github-actions bot requested a review from SarahFrench May 1, 2024 11:31

LucaPrete mentioned this pull request May 1, 2024

Change advertised_ip_ranges fields in google_compute_router_peer and google_compute_router resources to Sets #10118

Closed

Luca Prete added 2 commits May 1, 2024 13:34

Fix naming

bf86f7d

Fix

e9ce198

modular-magician added the service/compute-router label May 1, 2024

SarahFrench reviewed May 1, 2024

View reviewed changes

Fix

d75e595

github-actions bot requested a review from SarahFrench May 2, 2024 12:46

SarahFrench reviewed May 2, 2024

View reviewed changes

mmv1/templates/terraform/custom_flatten/compute_router_range.go.erb Outdated Show resolved Hide resolved

r/titlize_property(range)/titlize_property(property)

5320b76

github-actions bot requested a review from SarahFrench May 2, 2024 14:17

SarahFrench reviewed May 2, 2024

View reviewed changes

SarahFrench mentioned this pull request May 2, 2024

google_compute_router: dynamic block leads to undesired diff in plan hashicorp/terraform-provider-google#9353

Open

Moving flatten function call under array

90c28e2

github-actions bot requested a review from SarahFrench May 6, 2024 09:05

SarahFrench reviewed May 13, 2024

View reviewed changes

SarahFrench reviewed May 21, 2024

View reviewed changes

mmv1/templates/terraform/custom_flatten/compute_router_range.go.erb Outdated Show resolved Hide resolved

SarahFrench requested changes May 22, 2024

View reviewed changes

mmv1/templates/terraform/custom_flatten/compute_router_range.go.erb Outdated Show resolved Hide resolved

Update mmv1/templates/terraform/custom_flatten/compute_router_range.g…

0550d5a

…o.erb Co-authored-by: Sarah French <15078782+SarahFrench@users.noreply.github.com>

github-actions bot requested a review from SarahFrench May 23, 2024 11:14

SarahFrench approved these changes May 23, 2024

View reviewed changes

SarahFrench merged commit 43a016f into GoogleCloudPlatform:main May 23, 2024
15 checks passed

This was referenced May 23, 2024

[#9353] Make cloud router sort advertised_ip_ranges in state by config order hashicorp/terraform-provider-google-beta#7411

Merged

[#9353] Make cloud router sort advertised_ip_ranges in state by config order hashicorp/terraform-provider-google#18228

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#9353] Make cloud router sort advertised_ip_ranges in state by config order #10572

[#9353] Make cloud router sort advertised_ip_ranges in state by config order #10572

LucaPrete commented May 1, 2024 •

edited by SarahFrench

Loading

github-actions bot commented May 1, 2024

modular-magician commented May 1, 2024

LucaPrete commented May 1, 2024

modular-magician commented May 1, 2024

LucaPrete commented May 1, 2024 •

edited

Loading

SarahFrench left a comment •

edited

Loading

LucaPrete commented May 2, 2024

SarahFrench commented May 2, 2024 •

edited

Loading

modular-magician commented May 2, 2024

modular-magician commented May 2, 2024

SarahFrench May 2, 2024 •

edited

Loading

SarahFrench May 2, 2024

LucaPrete May 6, 2024

SarahFrench May 7, 2024

LucaPrete May 20, 2024

SarahFrench May 21, 2024

modular-magician commented May 6, 2024

modular-magician commented May 6, 2024

SarahFrench left a comment

github-actions bot commented May 21, 2024

SarahFrench commented May 21, 2024

modular-magician commented May 21, 2024

modular-magician commented May 21, 2024

SarahFrench commented May 22, 2024 •

edited

Loading

SarahFrench left a comment

LucaPrete commented May 23, 2024

modular-magician commented May 23, 2024

modular-magician commented May 23, 2024

SarahFrench left a comment

SarahFrench commented May 23, 2024

[#9353] Make cloud router sort advertised_ip_ranges in state by config order #10572

[#9353] Make cloud router sort advertised_ip_ranges in state by config order #10572

Conversation

LucaPrete commented May 1, 2024 • edited by SarahFrench Loading

github-actions bot commented May 1, 2024

modular-magician commented May 1, 2024

Diff report

LucaPrete commented May 1, 2024

modular-magician commented May 1, 2024

Tests analytics

LucaPrete commented May 1, 2024 • edited Loading

SarahFrench left a comment • edited Loading

Choose a reason for hiding this comment

LucaPrete commented May 2, 2024

SarahFrench commented May 2, 2024 • edited Loading

modular-magician commented May 2, 2024

Diff report

modular-magician commented May 2, 2024

Tests analytics

SarahFrench May 2, 2024 • edited Loading

Choose a reason for hiding this comment

SarahFrench May 2, 2024

Choose a reason for hiding this comment

LucaPrete May 6, 2024

Choose a reason for hiding this comment

SarahFrench May 7, 2024

Choose a reason for hiding this comment

LucaPrete May 20, 2024

Choose a reason for hiding this comment

SarahFrench May 21, 2024

Choose a reason for hiding this comment

modular-magician commented May 6, 2024

Diff report

modular-magician commented May 6, 2024

Tests analytics

SarahFrench left a comment

Choose a reason for hiding this comment

github-actions bot commented May 21, 2024

SarahFrench commented May 21, 2024

modular-magician commented May 21, 2024

Diff report

modular-magician commented May 21, 2024

Tests analytics

SarahFrench commented May 22, 2024 • edited Loading

SarahFrench left a comment

Choose a reason for hiding this comment

LucaPrete commented May 23, 2024

modular-magician commented May 23, 2024

Diff report

modular-magician commented May 23, 2024

Tests analytics

SarahFrench left a comment

Choose a reason for hiding this comment

SarahFrench commented May 23, 2024

LucaPrete commented May 1, 2024 •

edited by SarahFrench

Loading

LucaPrete commented May 1, 2024 •

edited

Loading

SarahFrench left a comment •

edited

Loading

SarahFrench commented May 2, 2024 •

edited

Loading

SarahFrench May 2, 2024 •

edited

Loading

SarahFrench commented May 22, 2024 •

edited

Loading