-
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
Adding noautomatednszone
in the resource forwardingrule
#8102
Adding noautomatednszone
in the resource forwardingrule
#8102
Conversation
…ith test step and the plan was not empty
Hello! I am a robot who works on Magic Modules PRs. I've detected that you're a community contributor. @SarahFrench, a repository maintainer, has been assigned to assist you and help review your changes. ❓ First time contributing? Click here for more detailsYour assigned reviewer will help review your code by:
You can help make sure that review is quick by running local tests and ensuring they're passing in between each push you make to your PR's branch. Also, try to leave a comment with each push you make, as pushes generally don't generate emails. If your reviewer doesn't get back to you within a week after your most recent change, please feel free to leave a comment on the issue asking them to take a look! In the absence of a dedicated review dashboard most maintainers manage their pending reviews through email, and those will sometimes get lost in their inbox. |
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. Terraform GA: Diff ( 5 files changed, 57 insertions(+)) |
Tests analyticsTotal tests: Action takenFound 9 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccComputeGlobalForwardingRule_privateServiceConnectGoogleApisExample|TestAccComputeForwardingRule_forwardingRuleVpcPscExample|TestAccComputeFirewallPolicyRule_multipleRules|TestAccBigQueryDataTable_bigtable|TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample|TestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example|TestAccComputeForwardingRule_forwardingRuleVpcPscExampleUpdate|TestAccDataSourceGoogleFirebaseAndroidAppConfig|TestAccDataSourceAlloydbLocations_basic |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
Just an FYI in case you're not able to run the tests on your machine: the acceptance tests currently show some failures because Terraform has a non-empty plan after the acceptance tests finish the defined test steps. Terraform wants to change
It might be worth running acceptance tests with |
Hi @SarahFrench , thanks for the info. Yes, I can run the tests locally, great I'll use and save the logs with env_var. |
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. Terraform GA: Diff ( 5 files changed, 326 insertions(+)) |
Tests analyticsTotal tests: Action takenFound 5 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccComputeNetworkEndpoints_networkEndpointsBasic|TestAccComputeGlobalForwardingRule_privateServiceConnectGoogleApisNoAutomateDnsExample|TestAccComputeForwardingRule_forwardingRuleVpcPscNoAutomateDnsExample|TestAccComputeFirewallPolicyRule_multipleRules|TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
Hi @SarahFrench, I've made some updates in the field for both resources, tested the scenarios for beta and ga, both passed. |
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. Terraform GA: Diff ( 6 files changed, 319 insertions(+), 8 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccComputeForwardingRule_forwardingRuleVpcPscNoAutomateDnsExample|TestAccComputeFirewallPolicyRule_multipleRules|TestAccComputeNetworkEndpoints_networkEndpointsBasic |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
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.
Sorry for the wait! I've left a comment as I'm unfamiliar with how this field behaves, and something about comments in the example file.
This is used in PSC consumer ForwardingRule to control whether it should try to auto-generate a DNS zone or not. | ||
Non-PSC forwarding rules do not use this field. | ||
send_empty_value: true | ||
immutable: 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.
I was looking at the docs for updating global forwarding rules and didn't see a note about being unable to update this field. What made you choose to set it as immutable here (and for the regional version of the resource)?
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.
@SarahFrench about this one, I see it as immutable because I can't update the field after it's created, the API show us it's possible to add the field in the body, but when trying to update I receive an error "Invalid value for field 'resource.noAutomateDnsZone': 'false'. Invalid field set in Private Service Connect Forwarding Rule. This field should not be set."
Another point, this field doesn't return on the GET, the reason I'm adding the ignore_read, another point in the doc of gcloud for forwarding rules update, there's no flag to update the field
https://cloud.google.com/sdk/gcloud/reference/compute/forwarding-rules/update.
But, I'm double checking with some Googler that works with this resource and will let you know.
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's a shame that the docs don't reflect this field being unchangeable, though from what you described the decision to make this field immutable and ignored on read sounds correct.
I'm happy to approve this PR, though should we wait on what the Googler says?
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.
Hi @SarahFrench , thanks for the answers and the help.
While I was writing an answer to you I just received a message.
So the field can't be patched because it's just on creation time and unfortunately we don't have this information on the documentation, just figuring it out.
So I see we can move forward with the review
Thanks a lot
mmv1/templates/terraform/examples/private_service_connect_google_apis_no_automate_dns.tf.erb
Outdated
Show resolved
Hide resolved
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. Terraform GA: Diff ( 6 files changed, 310 insertions(+), 8 deletions(-)) |
Tests analyticsTotal tests: 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 testsTestAccComputeFirewallPolicyRule_multipleRules |
Tests failed during RECORDING mode: Please fix these to complete your PR |
…oudPlatform#8102) * adding the new fields noAutomateDnsZone, need to validate the error with test step and the plan was not empty * adding the tests scenarios, need to fix the test for global * adding more tests, but requires a test with a no_autome_dns with true value * added to the field, immutability and ignoring read from api, tests are passing * removing comments from tests
…oudPlatform#8102) * adding the new fields noAutomateDnsZone, need to validate the error with test step and the plan was not empty * adding the tests scenarios, need to fix the test for global * adding more tests, but requires a test with a no_autome_dns with true value * added to the field, immutability and ignoring read from api, tests are passing * removing comments from tests
Solves the issue: hashicorp/terraform-provider-google#14833
If this PR is for Terraform, I acknowledge that I have:
make test
andmake lint
in the generated providers to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)