-
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
Support Cloud SQL Address Range Picker for Clones and Read replicas #5664
Support Cloud SQL Address Range Picker for Clones and Read replicas #5664
Conversation
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 5 files changed, 222 insertions(+), 3 deletions(-)) |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDatasourceGoogleServiceNetworkingPeeredDnsDomain_basic|TestAccApigeeEnvironmentIamBindingGenerated|TestAccApigeeEnvironmentIamMemberGenerated|TestAccApigeeEnvironmentIamPolicyGenerated|TestAccCloudbuildWorkerPool_basic|TestAccComputeGlobalForwardingRule_externalCndLbWithBackendBucketExample|TestAccContainerCluster_withAddons|TestAccServiceNetworkingPeeredDNSDomain_basic|TestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRange_replica|TestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRange_clone You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=249753 |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 5 files changed, 223 insertions(+), 5 deletions(-)) |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccApigeeEnvironmentIamBindingGenerated|TestAccApigeeEnvironmentIamMemberGenerated|TestAccApigeeEnvironmentIamPolicyGenerated|TestAccCloudbuildWorkerPool_basic|TestAccComputeGlobalForwardingRule_externalCndLbWithBackendBucketExample|TestAccContainerCluster_withAddons|TestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRangeReplica|TestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRangeClone|TestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRange You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=250521 |
/gcbrun |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 5 files changed, 223 insertions(+), 5 deletions(-)) |
@@ -311,7 +311,6 @@ settings.backup_configuration.binary_log_enabled are both set to true.`, | |||
"allocated_ip_range": { | |||
Type: schema.TypeString, | |||
Optional: true, | |||
ForceNew: 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.
Is the update case for this field something we can add a test for, or is it too hard to orchestrate?
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.
We can create a test on updating this field with a new private network, as I mentioned in the PR description, but just not within the same VPC. I will add one on this.
func TestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRangeClone(t *testing.T) { | ||
t.Parallel() | ||
|
||
databaseName := "tf-test-" + randString(t, 10) |
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.
Noting here that we cannot use the bootstrap db instance since this uses a private network. If more than one private network clone/backup test is needed, we should create a new bootstrap instance.
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.
Agree! Looks like this is the only one now. We will probably need to create a bootstrap instance with private networks if more tests need it.
resource "google_sql_database_instance" "instance" { | ||
depends_on = [google_service_networking_connection.foobar] | ||
name = "%s" | ||
region = "europe-west4" |
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'm assuming this region is one of the limited regions available for this feature?
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, correct
(also note: we should hold merging on confirmation of full rollout) |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDatasourceGoogleServiceNetworkingPeeredDnsDomain_basic|TestAccApigeeEnvironmentIamBindingGenerated|TestAccApigeeEnvironmentIamMemberGenerated|TestAccApigeeEnvironmentIamPolicyGenerated|TestAccComputeBackendServiceIamBindingGenerated|TestAccComputeBackendServiceIamMemberGenerated|TestAccComputeBackendServiceIamPolicyGenerated|TestAccComputeBackendServiceIamBindingGenerated_withCondition|TestAccComputeBackendServiceIamMemberGenerated_withCondition|TestAccComputeBackendServiceIamPolicyGenerated_withCondition|TestAccCloudbuildWorkerPool_basic|TestAccComputeGlobalForwardingRule_externalCndLbWithBackendBucketExample|TestAccComputeSecurityPolicy_withRateLimitOptions|TestAccContainerCluster_withAddons|TestAccContainerCluster_nodeAutoprovisioning|TestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRangeReplica|TestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRangeClone|TestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRange You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=251051 |
7d7f84e
to
65d5376
Compare
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 5 files changed, 236 insertions(+), 5 deletions(-)) |
1 similar comment
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 5 files changed, 236 insertions(+), 5 deletions(-)) |
1b1786a
to
363aa17
Compare
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 5 files changed, 233 insertions(+), 5 deletions(-)) |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 5 files changed, 234 insertions(+), 5 deletions(-)) |
Created separate private networks for each test (primary, read replica and clone). Should be fine to run tests in parallel now. As the networks are created as bootstrap (basically two extra bootstrap Networks are added), it should not hit the quota issue. |
2696036
to
366d4cb
Compare
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 5 files changed, 214 insertions(+), 5 deletions(-)) |
366d4cb
to
2260e35
Compare
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 5 files changed, 214 insertions(+), 5 deletions(-)) |
Fixes hashicorp/terraform-provider-google#11000
Also removed the
ForceNew
forallocated_ip_range
field for primary instance update, as currently allocated IP range can not be updated only within a VPC - the field can be updated in place if we also update the private network. Therefore, recreation functionality should be added to this field in a general case. We could add a CustomizeDiff to only addForceNew
ifallocated_ip_range
is changed within a VPC, but I am not sure if that's something we want to add, as the API may change to include the update feature anytime, plus the allocated ip range does not support updating after creation using UI and I also wonder if users would appreciate recreation for sql instances in general.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)