-
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
Implement Cloud SQL deletion protection #6441
Conversation
Hello! I am a robot who works on Magic Modules PRs. I've detected that you're a community contributor. @c2thorn, 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. |
The provider crashed while running the VCR tests in REPLAYING mode |
@c2thorn |
mmv1/third_party/terraform/resources/resource_sql_database_instance.go.erb
Outdated
Show resolved
Hide resolved
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 1 file changed, 119 insertions(+), 109 deletions(-)) |
Tests analyticsTotal tests: Errors occurred during REPLAYING mode. Please fix them to complete your PR |
a62d399
to
c6e3c25
Compare
You can keep @c2thorn as the main reviewer, I don't have any power here :) |
I removed @c2thorn by mistake and it looks like I cannot add him back. |
The provider crashed while running the VCR tests in REPLAYING mode |
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.
Hey @ShubhamAvasthi,
I discussed with my team, and we are not sure merging the TF-only deletion_protection_enabled
field with settings.deletion_protection
is the right approach.
While similar to the user, they have different behaviors behind the scenes. Also, merging would introduce immediate drift since the API default is false correct?
@c2thorn, I will send you an email internally, which will address your concerns and explain our idea behind this change. |
@c2thorn, can you also please help me understand why the provider is crashing while running VCR tests? It does not look like it's caused by my changes. |
Any progress on this change? Can I help in any ways? |
@multani, there are a few different ways of implementing the feature in Terraform, each with its pros and cons. Thanks for offering to help but we will handle it! |
8d515d7
to
d1baab6
Compare
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 3 files changed, 94 insertions(+), 31 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccComputeInstance_soleTenantNodeAffinities|TestAccSqlDatabaseInstance_settings_deletionProtectionEnabled|TestAccFirebaserulesRelease_BasicRelease|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
Wanted to double-check the status of the PR here. I've been holding further review under the assumption that the failed All that should be left is to modify the destroy producer to accept the
error, or turn off the deletion protection at the end of the test so it can be deleted. |
@c2thorn, we found a minor issue in the backend, which is currently blocking this PR. I have implemented a fix for that, but we will have to wait for it to roll out before submitting this 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, 100 insertions(+), 34 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccLoggingBucketConfigProject_cmekSettings|TestAccContainerCluster_withInvalidGatewayApiConfigChannel|TestAccIapWebIamPolicyGenerated|TestAccSqlDatabaseInstance_settings_deletionProtectionEnabled |
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 ( 3 files changed, 100 insertions(+), 34 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccContainerCluster_withInvalidGatewayApiConfigChannel|TestAccLoggingBucketConfigProject_cmekSettings|TestAccSqlDatabaseInstance_settings_deletionProtectionEnabled |
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 ( 3 files changed, 99 insertions(+), 34 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccLoggingBucketConfigProject_cmekSettings|TestAccComputeRouterNat_withNatRules|TestAccComputeRouterNat_withPortAllocationMethods|TestAccComputeRouterNat_withNatIpsAndDrainNatIps|TestAccComputeRouterNat_removeLogConfig|TestAccComputeRouterNat_update|TestAccContainerCluster_withInvalidGatewayApiConfigChannel|TestAccSqlDatabaseInstance_settings_deletionProtectionEnabled |
Tests passed during RECORDING mode: All tests passed |
@c2thorn, I believe this PR is ready to submit after your review. The API fix has been rolled-out, so we are good to go now. |
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. I've updated the release note as well. Thanks for your effort and patience with this feature
I acknowledge that I have:
make test
andmake lint
to ensure it passes unit and linter tests.Release Note for Downstream PRs