-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 firewall source range DSF #5309
Add firewall source range DSF #5309
Conversation
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 1 file changed, 34 insertions(+), 3 deletions(-)) |
/gcbrun |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 1 file changed, 34 insertions(+), 3 deletions(-)) |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccServiceNetworkingPeeredDNSDomain_basic|TestAccTags You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=210240 |
} | ||
|
||
// Diff suppress only should suppress removing the default range | ||
if oldInt == 1 && newInt == 1 { |
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.
Would newInt be 1
here? I thought it'd be 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.
So, for reasons I'm not super sure about we end up calling this DSF twice for the normal ["0.0.0.0/0"] -> [] diff when the API returns the default.
Once it is telling us that the length of the set is changing, and the key is "source_ranges.#" and the values are 1 and 0, and once telling us that "0.0.0.0/0" is being removed, but in this case it doesn't really say it is being removed, just that it is being replaced by the empty string. DSFs on sets are mysterious to me
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.
It gets called once on the length, and once on each field I think, so that's expected! Yeah, I don't love DSFs on non primitives
Not sure if that answers the 0/1 question?
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.
Ah- didn't notice the review request too, just the chat message. LGTM pending an answer to the newInt
question.
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 1 file changed, 37 insertions(+), 3 deletions(-)) |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDatasourceGoogleServiceNetworkingPeeredDnsDomain_basic|TestAccContainerNodePool_withGPU|TestAccSqlUser_postgresIAM|TestAccTags You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=211340 |
ba3b455
to
cfd4687
Compare
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 1 file changed, 37 insertions(+), 3 deletions(-)) |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDatasourceGoogleServiceNetworkingPeeredDnsDomain_basic|TestAccCloudbuildWorkerPool_withNetwork|TestAccCloudFunctionsFunction_vpcConnector|TestAccComputeFirewallPolicyRule_update|TestAccComputeForwardingRule_update|TestAccComputeInstanceFromMachineImage_basic|TestAccComputeInstanceFromMachineImage_overrideMetadataDotStartupScript|TestAccComputeInstanceFromMachineImage_diffProject|TestAccComputeOrganizationSecurityPolicyAssociation_organizationSecurityPolicyAssociationBasicExample|TestAccComputeOrganizationSecurityPolicy_organizationSecurityPolicyUpdateExample|TestAccComputeOrganizationSecurityPolicyRule_organizationSecurityPolicyRuleUpdateExample|TestAccComputeRegionNetworkEndpointGroup_regionNetworkEndpointGroupAppengineExample|TestAccContainerAnalysisOccurrence_multipleSignatures|TestAccContainerNodePool_withGPU|TestAccContainerNodePool_ephemeralStorageConfig|TestAccServiceNetworkingPeeredDNSDomain_basic|TestAccSqlUser_postgresIAM You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=213630 |
Fixes: hashicorp/terraform-provider-google#8076
If this PR is for Terraform, I acknowledge that I have:
make test
andmake lint
to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)