-
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
Add enableDropProtection support to spanner database #8011
Add enableDropProtection support to spanner database #8011
Conversation
Oops! It looks like you're using an unknown release-note type in your changelog entries:
Please only use the types listed in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/.ci/RELEASE_NOTES_GUIDE.md. |
Hello! I am a robot who works on Magic Modules PRs. I've detected that you're a community contributor. @shuyama1, 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 ( 2 files changed, 88 insertions(+)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: Please add acceptance tests which include these fields. |
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 testsTestAccComputeFirewallPolicyRule_multipleRules|TestAccAlloydbCluster_missingLocation|TestAccAlloydbBackup_missingLocation|TestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example|TestAccDataSourceAlloydbLocations_basic|TestAccDataSourceGoogleFirebaseAndroidAppConfig|TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
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, 121 insertions(+)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: Please add acceptance tests which include these fields. |
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 testsTestAccComputeRegionInstanceTemplate_with18TbScratchDisk|TestAccComputeRegionInstanceTemplate_withScratchDisk|TestAccComputeRegionInstanceTemplate_minCpuPlatform|TestAccComputeRegionInstanceTemplate_guestAcceleratorSkip|TestAccComputeRegionInstanceTemplate_guestAccelerator|TestAccComputeRegionInstanceTemplate_secondaryAliasIpRange|TestAccComputeRegionInstanceTemplate_primaryAliasIpRange|TestAccComputeRegionInstanceTemplate_metadata_startup_script|TestAccComputeRegionInstanceTemplate_subnet_custom|TestAccComputeRegionInstanceTemplate_subnet_auto|TestAccComputeInstanceTemplate_minCpuPlatform|TestAccComputeRegionInstanceTemplate_regionDisks|TestAccComputeInstanceFromTemplate_overrideAttachedDisk|TestAccComputeInstanceTemplate_guestAcceleratorSkip|TestAccComputeInstanceFromTemplate_overrideBootDisk|TestAccComputeInstanceTemplate_NetworkAttachment|TestAccComputeInstanceFromRegionTemplate_basic|TestAccContainerAwsNodePool_BetaBasicHandWritten|TestAccComputeInstanceTemplate_secondaryAliasIpRange|TestAccComputeInstanceFromTemplate_self_link_unique|TestAccContainerAwsNodePool_BasicHandWritten|TestAccComputeInstanceFromTemplate_basic|TestAccContainerAwsCluster_BetaBasicHandWritten|TestAccComputeInstanceTemplate_primaryAliasIpRange|TestAccContainerAwsCluster_BasicHandWritten|TestAccComputeInstanceTemplate_metadata_startup_script|TestAccComputeRegionAutoscaler_regionAutoscalerBasicExample|TestAccComputeInstanceTemplate_subnet_custom|TestAccComputeInstanceTemplate_subnet_auto|TestAccComputeInstanceTemplate_regionDisks|TestAccComputeInstanceTemplate_disks|TestAccComputeInstanceTemplate_guestAccelerator|TestAccComputeInstanceTemplate_with18TbScratchDisk|TestAccComputeInstanceTemplate_withScratchDisk|TestAccCloudbuildv2Connection_GleOldConnection|TestAccCloudbuildv2Connection_GleConnection|TestAccComputeFirewallPolicyRule_multipleRules|TestAccCloudbuildv2Connection_GitlabConnection|TestAccClouddeployDeliveryPipeline_CanaryrunDeliveryPipeline|TestAccClouddeployDeliveryPipeline_CanaryServiceNetworkingDeliveryPipeline|TestAccComputeInstanceFromTemplate_012_removableFields|TestAccComputeInstanceFromTemplate_overrideScratchDisk|TestAccClouddeployDeliveryPipeline_CanaryDeliveryPipeline|TestAccCloudbuildv2Connection_GlePrivUpdateConnection|TestAccClouddeployDeliveryPipeline_DeliveryPipeline|TestAccCloudbuildv2Connection_GlePrivConnection |
tests got timed out. rerun now |
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, 121 insertions(+)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: Please add acceptance tests which include these fields. |
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.
Can you add a test on it? We require that every field have at least one test coverage if feasible. You can modify an existing one in https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/third_party/terraform/tests/resource_spanner_database_test.go.erb.
Plus, since this field support in-place update, can you also include this in an update step?
More details on https://googlecloudplatform.github.io/magic-modules/develop/add-handwritten-test/#update-tests
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 testsTestAccContainerAwsNodePool_BetaBasicHandWritten|TestAccComputeFirewallPolicyRule_multipleRules |
Tests failed during RECORDING mode: Please fix these to complete your PR |
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 ( 3 files changed, 135 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 testsTestAccSpannerDatabase_enableDropProtection|TestAccComputeTargetHttpsProxy_targetHttpsProxyHttpKeepAliveTimeoutExample|TestAccComputeTargetHttpProxy_targetHttpProxyHttpKeepAliveTimeoutExample|TestAccContainerAwsNodePool_BetaBasicHandWritten|TestAccComputeFirewallPolicyRule_multipleRules |
Rerun these tests in REPLAYING mode to catch issues
|
enableDropProtection, enableDropProtectionOk := d.GetOk("enable_drop_protection") | ||
dropProtection := enableDropProtection.(bool) | ||
if enableDropProtectionOk && dropProtection { | ||
updateMask := []string{"enableDropProtection"} | ||
url, err := tpgresource.ReplaceVars(d, config, "{{SpannerBasePath}}projects/{{project}}/instances/{{instance}}/databases/{{name}}") | ||
if err != nil { | ||
return err | ||
} | ||
// updateMask is a URL parameter but not present in the schema, so ReplaceVars | ||
// won't set it | ||
url, err = transport_tpg.AddQueryParams(url, map[string]string{"updateMask": strings.Join(updateMask, ",")}) | ||
if err != nil { | ||
return err | ||
} | ||
res, err = transport_tpg.SendRequest(transport_tpg.SendRequestOptions{ | ||
Config: config, | ||
Method: "PATCH", | ||
Project: billingProject, | ||
RawURL: url, | ||
UserAgent: userAgent, | ||
Body: obj, | ||
Timeout: d.Timeout(schema.TimeoutUpdate), | ||
}) | ||
if err != nil { | ||
return fmt.Errorf("Error updating enableDropDatabaseProtection on Database: %s", err) | ||
} else { | ||
log.Printf("[DEBUG] Finished updating enableDropDatabaseProtection %q: %#v", d.Id(), res) | ||
} |
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.
qq: why do we need it for creation?
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.
The create database API does not support setting enableDropProtection, so we need to make separate request using this https://cloud.google.com/spanner/docs/reference/rest/v1/projects.instances.databases/patch
/gcbrun |
@rahul2393 would you mind rebasing your PR? |
@rahul2393 small question regarding the field: I can't find API documentation of the field, but my assumption is that if we set this field to true, we will prevent the database from deletion? We've already have a Terraform side |
Yes, this is for server side protection and since we don't want it to be breaking change so we will support both the fields for now, and plan is to put deprecation message/comment on existing virtual field |
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 ( 3 files changed, 207 insertions(+)) |
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 ( 3 files changed, 212 insertions(+)) |
/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 ( 3 files changed, 218 insertions(+)) |
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 ( 3 files changed, 267 insertions(+), 30 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 testsTestAccProjectIamPolicy_invalidMembers|TestAccContainerAwsNodePool_BetaBasicHandWritten|TestAccSpannerDatabase_basic |
Rerun these tests in REPLAYING mode to catch issues
|
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 ( 3 files changed, 264 insertions(+), 30 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 testsTestAccContainerAwsNodePool_BetaBasicHandWritten|TestAccFolderIamPolicy_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! Thank you!
Add support for setting enable_drop_protection on google_spanner_database
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)