-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Deleted immutable attributes for initialUser and weeklySchedule so th… #7552
Conversation
…at deleting/adding these properties doesn't force new cluster creation
/gcbrun |
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, 10 insertions(+), 2 deletions(-)) |
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 testsTestAccFirebaserulesRelease_BasicRelease|TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample|TestAccFrameworkProviderBasePath_setBasePath|TestAccFrameworkProviderMeta_setModuleName|TestAccDataSourceGoogleFirebaseAppleAppConfig|TestAccDataSourceDNSKeys_basic|TestAccDataSourceDnsRecordSet_basic|TestAccDataSourceDnsManagedZone_basic|TestAccDataSourceDNSKeys_noDnsSec |
Tests passed during RECORDING mode: All tests passed |
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.
This looks fairly straightforward and it looks like the downstream code for in-place updates is getting generated. However, it would be good to add additional update tests to make sure that all the fields we think are updatable can actually be updated. There is already an update test for alloydb, which can be extended to exercise a larger number of fields, for example to ensure that this issue is actually resolved and prevent it from recurring. See
/* Validate that read replica is enabled on the instance without recreate |
Hey Stephen, Once the current code is stabilised and doesn't have any glaring bugs (max 1-2 weeks), we will switch focus to writing sufficient automated tests to ensure these issues are caught early in development. |
responded elsewhere. |
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, 160 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 16 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccFrameworkProviderMeta_setModuleName|TestAccFrameworkProviderBasePath_setBasePath|TestAccFirebaserulesRelease_BasicRelease|TestAccFirebaseWebApp_firebaseWebAppBasicExample|TestAccFirebaseWebApp_firebaseWebAppFull|TestAccDataSourceGoogleFirebaseAppleAppConfig|TestAccComputeForwardingRule_update|TestAccAlloydbCluster_deleteAutomatedBackupPolicyAndInitialUser|TestAccAlloydbCluster_addAutomatedBackupPolicyAndInitialUser|TestAccApigeeEnvKeystoreAliasPkcs12_apigeeEnvKeystoreAliasPkcs12Example|TestAccDataSourceDNSKeys_basic|TestAccDataSourceDnsRecordSet_basic|TestAccDataSourceDNSKeys_noDnsSec|TestAccDataSourceDnsManagedZone_basic|TestAccComposerEnvironment_withEncryptionConfigComposer2|TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample |
Tests passed during RECORDING mode: All tests 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 ( 2 files changed, 160 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 13 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccFirebaserulesRelease_BasicRelease|TestAccFirebaseWebApp_firebaseWebAppFull|TestAccFirebaseWebApp_firebaseWebAppBasicExample|TestAccComposerEnvironment_withEncryptionConfigComposer2|TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample|TestAccApigeeEnvKeystoreAliasPkcs12_apigeeEnvKeystoreAliasPkcs12Example|TestAccDataSourceGoogleFirebaseAppleAppConfig|TestAccFrameworkProviderMeta_setModuleName|TestAccDataSourceDNSKeys_basic|TestAccFrameworkProviderBasePath_setBasePath|TestAccDataSourceDnsRecordSet_basic|TestAccDataSourceDnsManagedZone_basic|TestAccDataSourceDNSKeys_noDnsSec |
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.
LGTM - thanks for adding tests!
Failing test seems unrelated. |
GoogleCloudPlatform#7552) * Deleted immutable attributes for initialUser and weeklySchedule so that deleting/adding these properties doesn't force new cluster creation * Automated tests for initialUser and automatedBackupPolicy * Add ImportStateVerifyIgnore for non-changing fields * gofmt fixes --------- Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>
@melinath: Please make sure you review the changelog note in the future! I've adjusted this to:
|
GoogleCloudPlatform#7552) * Deleted immutable attributes for initialUser and weeklySchedule so that deleting/adding these properties doesn't force new cluster creation * Automated tests for initialUser and automatedBackupPolicy * Add ImportStateVerifyIgnore for non-changing fields * gofmt fixes --------- Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>
GoogleCloudPlatform#7552) * Deleted immutable attributes for initialUser and weeklySchedule so that deleting/adding these properties doesn't force new cluster creation * Automated tests for initialUser and automatedBackupPolicy * Add ImportStateVerifyIgnore for non-changing fields * gofmt fixes --------- Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>
GoogleCloudPlatform#7552) * Deleted immutable attributes for initialUser and weeklySchedule so that deleting/adding these properties doesn't force new cluster creation * Automated tests for initialUser and automatedBackupPolicy * Add ImportStateVerifyIgnore for non-changing fields * gofmt fixes --------- Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>
…at deleting/adding these properties doesn't force new cluster creation
Creating a cluster with the minimum fields works fine the first time. On re-running the same .tf file, it deletes the existing cluster and creates a new one.
We get the default values for automated backup policy from the API. This includes many fields. One of them is the weekly schedule. This field is marked as forceNew. The first time when we create the cluster without providing any automated backup policy, we get the default from the API. This default value gets persisted in the .tfstate file. On subsequent runs of terraform, Terraform sees that the automated backup policy is present in the state file but not in the .tf file. Since this field is marked as forceNew, the old cluster is deleted and a new one is created. This is highly undesirable.
Similarly, initialUser is marked as immutable while it is a mutable field. Addition/Deletion to this field causes existing cluster deletion and creation of a new one which is highly undesirable.
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)