-
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
Make google_notebooks_instance metadata field updatable #7945
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
…e changes are required to make the tests pass
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 ( 1 file changed, 150 insertions(+)) |
Tests analyticsTotal tests: Action takenFound 8 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccNotebooksInstance_update|TestAccAlloydbBackup_missingLocation|TestAccComputeFirewallPolicyRule_multipleRules|TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample|TestAccAlloydbCluster_missingLocation|TestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example|TestAccDataSourceAlloydbLocations_basic|TestAccDataSourceGoogleFirebaseAndroidAppConfig |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
…nly these changes are required to make the tests pass" This reverts commit f22a9e4.
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 ( 2 files changed, 204 insertions(+), 1 deletion(-)) |
Tests analyticsTotal tests: Action takenFound 8 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccNotebooksInstance_update|TestAccDataSourceGoogleFirebaseAndroidAppConfig|TestAccComputeFirewallPolicyRule_multipleRules|TestAccDataSourceAlloydbLocations_basic|TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample|TestAccAlloydbCluster_missingLocation|TestAccAlloydbBackup_missingLocation|TestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example |
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.
Overall LGTM. I had a question regarding the tests, but I suspect that there is either a verification step I missed, or adding one would be more effort than it's worth.
}, | ||
{ | ||
ResourceName: "google_notebooks_instance.instance", | ||
ImportState: true, | ||
ImportStateVerify: true, | ||
ImportStateVerifyIgnore: []string{"vm_image"}, | ||
ImportStateVerifyIgnore: []string{"vm_image", "metadata"}, |
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.
Am I understanding this correctly that we aren't ever checking that the metadata is updated on the server?
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.
yes. The field is marked as ignore_read
on the resource, so we never check what the value on the server is. I had to add it here because I introduced it as a new field in this test.
I suspect that we would not use ignore_read like this if implementing the resource today - but fixing that wouldn't be trivial and is outside of the scope of the ticket. In the future I think we'd want to do something similar to hashicorp/terraform-provider-google#14293.
…latform#7945) * Added update tests proving that metadata is not updatable * Test fixes * Made notebooks instance metadata field updatable * Added metadata to ImportStateVerifyIgnore for impacted tests * Removed non-updatable fields from update test * Temporarily made metadata field not updatable to prove that only these changes are required to make the tests pass * Revert "Temporarily made metadata field not updatable to prove that only these changes are required to make the tests pass" This reverts commit f22a9e4.
…latform#7945) * Added update tests proving that metadata is not updatable * Test fixes * Made notebooks instance metadata field updatable * Added metadata to ImportStateVerifyIgnore for impacted tests * Removed non-updatable fields from update test * Temporarily made metadata field not updatable to prove that only these changes are required to make the tests pass * Revert "Temporarily made metadata field not updatable to prove that only these changes are required to make the tests pass" This reverts commit f22a9e4.
…latform#7945) * Added update tests proving that metadata is not updatable * Test fixes * Made notebooks instance metadata field updatable * Added metadata to ImportStateVerifyIgnore for impacted tests * Removed non-updatable fields from update test * Temporarily made metadata field not updatable to prove that only these changes are required to make the tests pass * Revert "Temporarily made metadata field not updatable to prove that only these changes are required to make the tests pass" This reverts commit f22a9e4.
Fixes hashicorp/terraform-provider-google#14403.
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)