Marked customer_name field as optional for interconnect resource

[abhijeetkjha-google]

compute: marked `customer_name` and `location` fields as optional in `google_compute_interconnect` resource to support cross cloud interconnect

Bug Fix: hashicorp/terraform-provider-google#18264
The proto which defines the interconnect resource has customer_name field as an optional, so we should be providing this same parity between the terraform and the provided api. For Cross cloud interconnect, We should not be providing customer_name, else error is being thrown.

[github-actions]

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]

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

[modular-magician]

Tests analytics

Total tests: 993
Passed tests: 915
Skipped tests: 78
Affected tests: 0

Click here to see the affected service packages

• compute

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

View the build log

[shuyama1]

Looks like this example is skipped from test generation. It will update the examples in the web documentation though. Would you mind also removing the field from this example to have a test coverage on this. Plus, can you also remove this line https://github.com/abhijeetkjha-google/magic-modules/blob/bugFix-361094483/mmv1/products/compute/Interconnect.yaml#L52, given the field is removed from the example. Thanks

[abhijeetkjha-google]

Would you mind also removing the field from this example to have a test coverage on this.

Since we are marking a field as optional wouldn't it be a good idea to have one test example with the field and another without the field. The compute_interconnect_basic.tf.erb contains the required field only and compute_interconnect_basic_test.tf.erb includes the optional field as well.

can you also remove this line https://github.com/abhijeetkjha-google/magic-modules/blob/bugFix-361094483/mmv1/products/compute/Interconnect.yaml#L52, given the field is removed from the example.

Sure, Done.

[shuyama1]

Would you mind also removing the field from this example to have a test coverage on this.

Since we are marking a field as optional wouldn't it be a good idea to have one test example with the field and another without the field. The compute_interconnect_basic.tf.erb contains the required field only and compute_interconnect_basic_test.tf.erb includes the optional field as well.

Oh, right, it makes sense to have the field included in at least one test. But it looks like compute_interconnect_basic.tf.erb is only used to generate the doc instead of tests, as indicated by the flag skip_test for the example. Are you able to test it locally and make sure the field is not required for resource creation?

[abhijeetkjha-google]

Yes yes, I have verified that customer_name is an optional field and is not required for resource creation.

[harshithpatte-g]

@abhijeetkjha-google Please update the release note.
Otherwise, LGTM.

[abhijeetkjha-google]

@abhijeetkjha-google Please update the release note. Otherwise, LGTM.

Done.

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

[shuyama1]

I've changed the change type from bug to enhancement in the release note as I think it's more accurate.

[modular-magician]

Tests analytics

Total tests: 997
Passed tests: 921
Skipped tests: 76
Affected tests: 0

Click here to see the affected service packages

• compute

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

View the build log

[shuyama1]

I've removed customer_name from the test case and run tests in a test PR #11668. Looks like the test failed with API error googleapi: Error 400: Required field 'resource.customerName' not specified, required. I wonder if the field is still required?

[abhijeetkjha-google]

I've removed customer_name from the test case and run tests in a test PR #11668. Looks like the test failed with API error googleapi: Error 400: Required field 'resource.customerName' not specified, required. I wonder if the field is still required?

The error is rightly thrown, So I investigated this. So this is the issue.

We have three interconnects: Dedicated, Partner and Cross cloud.
So out of these three, Dedicated and Partner Interconnect requires customer_name as a field to create interconnect resource. And for Cross cloud interconnect we do no pass the customer_name else we will throw an error, incase a customer_name is detected.

without this change, If a person creates an cross cloud interconnect resource, They have to pass the customer_name field as its marked "required" for now but this will throw an error stating ""Customer name should not be present for Cross Cloud Interconnect".

So the test which are failing are for dedicated interconnect, which was a miss. So thats why we are getting this error.
So what I propose is that we should mark "customer_name" field as optional and not make any changes in these existing test. Instead I will try to write some test for "cross cloud interconnect" scenario.

[github-actions]

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

[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, 54 insertions(+), 12 deletions(-))
google-beta provider: Diff ( 3 files changed, 54 insertions(+), 12 deletions(-))
Open in Cloud Shell: Diff ( 4 files changed, 113 insertions(+))

[modular-magician]

Tests analytics

Total tests: 1005
Passed tests: 929
Skipped tests: 74
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

• TestAccComputeInstance_confidentialHyperDiskBootDisk
• TestAccComputeInterconnect_computeInterconnectCrossCloudBasicExample

Get to know how VCR tests work

[modular-magician]

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

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

View the build log or the debug log for each test

[shuyama1]

TestAccComputeInterconnect_computeInterconnectCrossCloudBasicExample failed with

resource_compute_interconnect_generated_test.go:85: Step 1/2 error: Error running pre-apply refresh: exit status 1
        
        Error: Missing required argument
        
          on terraform_plugin_test.tf line 4, in resource "google_compute_interconnect" "example-interconnect":
           4: resource "google_compute_interconnect" "example-interconnect" {
        
        The argument "location" is required, but no definition was found.

[abhijeetkjha-google]

Hi @shuyama1,
I tried adding/updating create tests, But it keeps failing as we can't use z2z location which are the locations available for testing. Also we need to add remote_location field which identifies this request as a cross_cloud instance. But as per the implementation we can't have location field in such cases so marking this field as optional as well. Since I am not able to add any function test around this scenario till we get some sort of test remote_location. I have updated this pr just with the field change. Let me know if anything can be done around this. Thanks.

[shuyama1]

Would you be able to set up a local environment and test it locally? We may rely on local testing in this case if our testing environment is not compatible with feature. Thank you! https://googlecloudplatform.github.io/magic-modules/develop/test/run-tests/#run-tests

[shuyama1]

Is making location optional in this resource intended? Sorry, I'm not fully understand what remote_location field is. Is location field not needed for resource creation in some cases?

[abhijeetkjha-google]

Yes, Incase of cross_cloud interconnect we don't need to specify location field as location field implies for google owned location, Incase of cross_cloud interconnect we have to replace it with remote_location field.

[shuyama1]

Got it. Would you mind also clarifying it in the description of the field? Thanks!

[abhijeetkjha-google]

Done

[shuyama1]

Could you please update the description of this field to clarify when this field is required and when it is not? Thanks

[abhijeetkjha-google]

Done.

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

[modular-magician]

Tests analytics

Total tests: 1011
Passed tests: 937
Skipped tests: 74
Affected tests: 0

Click here to see the affected service packages

• compute

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

View the build log

[shuyama1]

left a small comment in #11640 (comment) regarding a documentation improvement. Thanks!

[abhijeetkjha-google]

left a small comment in #11640 (comment) regarding a documentation improvement. Thanks!

Updated the description. Thanks.

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

[modular-magician]

Tests analytics

Total tests: 1015
Passed tests: 941
Skipped tests: 74
Affected tests: 0

Click here to see the affected service packages

• compute

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

View the build log

[shuyama1]

The contributor has confirmed the changes work as expected with local testing.