-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Alloydb crr secondary instance #9203
Alloydb crr secondary instance #9203
Conversation
Hello! I am a robot. It looks like you are a: @ScottSuarez, 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. Terraform GA: Diff ( 7 files changed, 1408 insertions(+), 10 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 46 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccAlloydbCluster_secondaryClusterWithNetworkConfigAndAllocatedIPRange|TestAccAlloydbCluster_secondaryClusterUsingCMEK|TestAccAlloydbCluster_secondaryClusterAddAutomatedBackupPolicy|TestAccAlloydbCluster_secondaryClusterDefinedSecondaryConfigButClusterTypeIsPrimary|TestAccAlloydbCluster_secondaryClusterDefinedSecondaryConfigButMissingClusterTypeSecondary|TestAccAlloydbCluster_secondaryClusterMissingSecondaryConfig|TestAccAlloydbCluster_secondaryClusterMandatoryFields|TestAccAlloydbCluster_secondaryClusterUpdate|TestAccAlloydbCluster_secondaryClusterWithNetworkConfig|TestAccAlloydbInstance_alloydbSecondaryInstanceBasicExample|TestAccAlloydbCluster_alloydbSecondaryClusterBasicExample|TestAccAlloydbInstance_createInstanceWithMandatoryFields|TestAccAlloydbCluster_AutomatedBackupPolicyHandlesMidnight|TestAccAlloydbCluster_deleteAutomatedBackupPolicyAndInitialUser|TestAccAlloydbCluster_alloydbClusterFullExample|TestAccAlloydbCluster_withNetworkConfig|TestAccAlloydbInstance_createPrimaryAndReadPoolInstance|TestAccAlloydbInstance_createInstanceWithNetworkConfigAndAllocatedIPRange|TestAccAlloydbInstance_update|TestAccAlloydbCluster_continuousBackup_noChangeIfRemoved|TestAccAlloydbCluster_deleteTimeBasedRetentionPolicy|TestAccAlloydbUser_updatePassword_BuiltIn|TestAccAlloydbCluster_restore|TestAccAlloydbBackup_alloydbBackupBasicExample|TestAccAlloydbCluster_addAutomatedBackupPolicyAndInitialUser|TestAccAlloydbCluster_continuousBackup_update|TestAccAlloydbCluster_update|TestAccAlloydbCluster_missingWeeklySchedule|TestAccAlloydbUser_updateRoles_BuiltIn|TestAccAlloydbUser_alloydbUserBuiltinExample|TestAccAlloydbCluster_withNetworkConfigAndAllocatedIPRange|TestAccAlloydbCluster_continuousBackup_enabledByDefault|TestAccAlloydbCluster_alloydbClusterBasicExample|TestAccAlloydbBackup_alloydbBackupFullExample|TestAccAlloydbUser_alloydbUserIamExample|TestAccAlloydbCluster_continuousBackup_update_noChangeIfDefaultsSet|TestAccAlloydbUser_updateRoles_IAM|TestAccAlloydbInstance_alloydbInstanceBasicExample|TestAccAlloydbCluster_usingCMEK|TestAccAlloydbCluster_continuousBackup_CMEKIsUpdatable|TestAccAlloydbCluster_CMEKInAutomatedBackupIsUpdatable|TestAccAlloydbBackup_usingCMEK|TestAccAlloydbBackup_createBackupWithMandatoryFields|TestAccAlloydbBackup_update|TestAccAlloydbCluster_mandatoryFields|TestAccAlloydbCluster_maximumFields |
mmv1/templates/terraform/examples/alloydb_secondary_instance_basic.tf.erb
Show resolved
Hide resolved
mmv1/templates/terraform/examples/alloydb_secondary_instance_basic.tf.erb
Show resolved
Hide resolved
Rerun these tests in REPLAYING mode to catch issues
|
1e28c54
to
aa640e5
Compare
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 ( 7 files changed, 1386 insertions(+), 10 deletions(-)) |
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 ( 7 files changed, 1373 insertions(+), 10 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 11 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccAlloydbCluster_secondaryClusterUpdate|TestAccAlloydbInstance_alloydbSecondaryInstanceBasicExample|TestAccAlloydbCluster_addAutomatedBackupPolicyAndInitialUser|TestAccAlloydbCluster_continuousBackup_update|TestAccAlloydbCluster_update|TestAccAlloydbCluster_CMEKInAutomatedBackupIsUpdatable|TestAccAlloydbCluster_secondaryClusterAddAutomatedBackupPolicy|TestAccAlloydbCluster_deleteAutomatedBackupPolicyAndInitialUser|TestAccAlloydbCluster_AutomatedBackupPolicyHandlesMidnight|TestAccAlloydbCluster_continuousBackup_CMEKIsUpdatable|TestAccAlloydbCluster_restore |
Rerun these tests in REPLAYING mode to catch issues
|
72953b5
to
79d5d56
Compare
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 ( 4 files changed, 1136 insertions(+), 6 deletions(-)) |
Tests analyticsTotal tests:
|
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, 643 insertions(+), 4 deletions(-)) |
1 similar comment
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, 643 insertions(+), 4 deletions(-)) |
mmv1/third_party/terraform/services/alloydb/resource_alloydb_secondary_instance_test.go
Show resolved
Hide resolved
Tests analyticsTotal tests: Action takenFound 4 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccAlloydbInstance_secondaryInstanceWithReadPoolInstance|TestAccAlloydbCluster_secondaryInstanceWithNetworkConfigAndAllocatedIPRange|TestAccAlloydbInstance_secondaryInstanceUpdate|TestAccDataSourceGoogleServiceAccountJwt |
Tests analyticsTotal tests: Action takenFound 7 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccAlloydbCluster_secondaryInstanceWithNetworkConfigAndAllocatedIPRange|TestAccAlloydbInstance_secondaryInstanceWithReadPoolInstance|TestAccAlloydbInstance_secondaryInstanceUpdate|TestAccBigtableAppProfile_bigtableAppProfileMulticlusterExample|TestAccBigtableAppProfile_bigtableAppProfileSingleclusterExample|TestAccBigtableAppProfile_bigtableAppProfileAnyclusterExample|TestAccDataprocClusterIamPolicy |
Rerun these tests in REPLAYING mode to catch issues
|
Rerun these tests in REPLAYING mode to catch issues
|
Co-authored-by: Riley Karson <[email protected]>
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 testsTestAccAlloydbInstance_secondaryInstanceMaximumFields|TestAccAlloydbInstance_secondaryInstanceUpdateQueryInsightConfig|TestAccAlloydbInstance_secondaryInstanceWithReadPoolInstance|TestAccAlloydbInstance_secondaryInstanceUpdateDatabaseFlag|TestAccAlloydbCluster_secondaryInstanceWithNetworkConfigAndAllocatedIPRange|TestAccAlloydbInstance_secondaryInstanceUpdateMachineConfig|TestAccAlloydbInstance_alloydbSecondaryInstanceBasicTestExample|TestAccBigQueryDataTable_bigtable |
Rerun these tests in REPLAYING mode to catch issues
|
See errors inline in the results! |
Fixed TestAccAlloydbCluster_secondaryInstanceWithNetworkConfigAndAllocatedIPRange So moved few of the secondary cluster location to us-west1 in tests |
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, 865 insertions(+), 10 deletions(-)) |
As a followup can you confirm that we have no more than 5 clusters in a region other than us-central1 in tests? We bumped out quota there. You can do that after this PR- it'll just stop us from failing to re-record on changes that affect AlloyDB. Our retries need to be fixed to handle this situation correctly- @roaks3 did we file an issue for that? |
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 testsTestAccAlloydbInstance_secondaryInstanceUpdateQueryInsightConfig|TestAccAlloydbCluster_secondaryInstanceWithNetworkConfigAndAllocatedIPRange|TestAccAlloydbInstance_secondaryInstanceMaximumFields|TestAccAlloydbInstance_secondaryInstanceWithReadPoolInstance|TestAccComposerEnvironment_UpdateComposerV2|TestAccComposerEnvironment_ComposerV2|TestAccContainerAwsCluster_BetaBasicHandWritten|TestAccContainerAwsCluster_BetaBasicEnumHandWritten |
Rerun these tests in REPLAYING mode to catch issues
|
Yea, apologies for the delay, I was partway through writing it up: hashicorp/terraform-provider-google#16392. |
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.
Generally LGTM, barring one comment left to resolve inline- I feel reasonably good about this. I don't love the pre_update
we're using, but the reasoning behind it was reasonable- when a promotion happens, the instance will be promoted out of band and we don't want to destroy the now-primary instance. This resource doesn't have a deletion_protection
field which could be an alternate way to guard for those cases.
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, 859 insertions(+), 10 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccDataSourceGoogleServiceAccountJwt|TestAccDataSourceGoogleServiceAccountAccessToken_basic |
Rerun these tests in REPLAYING mode to catch issues
|
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!
Has this been released yet ? Looking to create a secondary instance in a secondary cluster, however can't find any documentation anywhere. |
Co-authored-by: Riley Karson <[email protected]> Co-authored-by: Shubham Sahu <[email protected]>
Co-authored-by: Riley Karson <[email protected]> Co-authored-by: Shubham Sahu <[email protected]>
Co-authored-by: Riley Karson <[email protected]> Co-authored-by: Shubham Sahu <[email protected]>
To Create a Secondary Instance inside a secondary cluster, set the instance_type to point to the cluster_type of the associated secondary cluster & set lifecycle.ignore_changes = [instance_type]
Avoid setting instance_type = "SECONDARY", as promoting a secondary cluster to a primary cluster also modifies the instance_type of the associated secondary instance
Release Note Template for Downstream PRs (will be copied)