-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add the subnetLength field to the InterconnectAttachment resource #10722
Add the subnetLength field to the InterconnectAttachment resource #10722
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. |
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.
|
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 testsTestAccComputeInterconnectAttachment_interconnectAttachmentBasicExample |
|
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.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_compute_interconnect_attachment" "primary" {
subnet_length = # value needed
}
|
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.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_compute_interconnect_attachment" "primary" {
stack_type = # value needed
}
|
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.
|
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 testsTestAccComputeInterconnectAttachment_interconnectAttachmentDedicatedExample |
|
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 testsTestAccComputeInterconnectAttachment_interconnectAttachmentDedicatedExample |
|
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.
Overall LGTM, but just checking if the field config is correct, and I'm not sure the test is ideal.
interconnect = google_compute_interconnect.foobar.id | ||
router = google_compute_router.foobar.id | ||
mtu = 1500 | ||
subnet_length = 0 |
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.
I'm a little confused about this test. The description for the field says that 29 and 30 are valid values, but here we are testing 0. Because of how TF handles zero-values, this might not even be sent to the server (ie. the equivalent of not setting the field). Is it not possible to test with the suggested values of 29 or 30?
Also, is the default value set on the server? If so, you may need to add default_from_api: true
.
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.
Added the default_from_api
field.
Followed up about the 0 value offline.
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.
Ok, confirmed that this value is not being sent:
---[ REQUEST ]---------------------------------------
POST /compute/beta/projects/ci-test-project-188019/regions/us-east4/interconnectAttachments?alt=json HTTP/1.1
Host: compute.googleapis.com
User-Agent: Terraform/1.2.5 (+https://www.terraform.io) Terraform-Plugin-SDK/2.33.0 terraform-provider-google-beta/acc
Content-Length: 429
Content-Type: application/json
Accept-Encoding: gzip
{
"adminEnabled": true,
"encryption": "NONE",
"interconnect": "projects/ci-test-project-188019/global/interconnects/tf-test-interconenct-18x24pgb1wd",
"mtu": "1500",
"name": "tf-test-on-prem-attachment8x24pgb1wd",
"region": "projects/ci-test-project-188019/global/regions/us-east4",
"router": "projects/ci-test-project-188019/regions/us-east4/routers/tf-test-router-18x24pgb1wd",
"stackType": "IPV4_ONLY",
"type": "DEDICATED",
"vlanTag8021q": 1000
}
If the 0 truly holds value here (I'm still not 100% clear), you may want to use the config send_empty_value: true
to allow it to be sent. Otherwise, I believe there isn't any value in the test, and we should just remove it. I know there are limitations with this particular resource, and it is only a single field that is not being tested. It would be nice to have some sort of future-looking plan to test this, if possible, but not blocking.
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.
Ok, that plan sounds good to me. I will file a bug for our team to go back and add a test for this field in the future. Thanks!
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.
|
Tests analyticsTotal tests: Click here to see the affected service packages
|
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.
LGTM, just waiting on checks to complete before merge
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.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_compute_interconnect_attachment" "primary" {
stack_type = # value needed
subnet_length = # value needed
}
|
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.
|
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 testsTestAccComputeInterconnectAttachment_interconnectAttachmentDedicatedExample |
|
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.
|
Tests analyticsTotal tests: Click here to see the affected service packages
|
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.
LGTM, will merge on Monday morning (which will be well ahead of the release cut on Tuesday evening)
…ogleCloudPlatform#10722) Co-authored-by: Tommy Lang <tommylang@google.com>
…ogleCloudPlatform#10722) Co-authored-by: Tommy Lang <tommylang@google.com>
…ogleCloudPlatform#10722) Co-authored-by: Tommy Lang <tommylang@google.com>
…ogleCloudPlatform#10722) Co-authored-by: Tommy Lang <tommylang@google.com>
Add the subnetLength field to the InterconnectAttachment resource