Skip to content
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 deleting Firestore databases #9450

Merged
merged 1 commit into from
Nov 21, 2023

Conversation

rwhogg
Copy link
Contributor

@rwhogg rwhogg commented Nov 10, 2023

Due to backwards compatibility concerns, the default behavior remains to abandon the database upon destroy rather than to actually delete it.

To actually delete the database, you must set deletion_policy to DELETE, and apply if necessary, before running terraform destroy.

This also cleans up some related deletion-related docs and bugs:

  • Updates the delete protection docs
  • Either delete protection or deletion policy being set prevents destroy

Fixes hashicorp/terraform-provider-google#16488

Fixes hashicorp/terraform-provider-google#16404

Fixes hashicorp/terraform-provider-google#16325

Release Note Template for Downstream PRs (will be copied)

firestore: enabled database deletion upon destroy for `google_firestore_database`
firestore: added virtual field `deletion_policy` to `google_firestore_database`
firestore: prevent destruction if both `deletion_policy` is `DELETE` and `delete_protection_state` is `DELETE_PROTECTION_ENABLED`

Manual Testing notes:

delete_protection_state == enabled, policy == abandon, result: abandon
delete_protection_state == enabled, policy == delete, result == prevent
delete_protection_state == disabled, policy == abandon, result == abandon
delete_protection_state == disabled, policy == delete, result == delete

@modular-magician
Copy link
Collaborator

Hello! I am a robot. It looks like you are a: Community Contributor Googler Core Contributor. Tests will run automatically.

@slevenick, 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.

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 3 files changed, 165 insertions(+), 167 deletions(-))
Terraform Beta: Diff ( 3 files changed, 165 insertions(+), 167 deletions(-))

Copy link

@rxggg rxggg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3224
Passed tests 2892
Skipped tests: 329
Affected tests: 3

Action taken

Found 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccFirestoreDatabase_firestoreDatabaseExample|TestAccFirestoreDatabase_firestoreDatabaseInDatastoreModeExample|TestAccFirestoreDatabase_firestoreDefaultDatabaseExample

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccFirestoreDatabase_firestoreDatabaseExample[Debug log]
TestAccFirestoreDatabase_firestoreDatabaseInDatastoreModeExample[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$


$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccFirestoreDatabase_firestoreDefaultDatabaseExample[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

@rwhogg
Copy link
Contributor Author

rwhogg commented Nov 10, 2023

@slevenick would it be possible to delete the (default) database associated with the CI project? I can work with you next week to do that if necessary.

The ironic thing is that because database deletion is only possible now, the database from past CI runs is still lingering in the project, and we can't delete it via Terraform until this PR gets in.

@slevenick
Copy link
Contributor

@slevenick would it be possible to delete the (default) database associated with the CI project? I can work with you next week to do that if necessary.

The ironic thing is that because database deletion is only possible now, the database from past CI runs is still lingering in the project, and we can't delete it via Terraform until this PR gets in.

The database should be deleted now

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 3 files changed, 165 insertions(+), 167 deletions(-))
Terraform Beta: Diff ( 3 files changed, 165 insertions(+), 167 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3225
Passed tests 2893
Skipped tests: 329
Affected tests: 3

Action taken

Found 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccFirestoreDatabase_firestoreDefaultDatabaseExample|TestAccMemcacheInstance_update|TestAccSpannerDatabaseIamPolicy

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 3 files changed, 165 insertions(+), 167 deletions(-))
Terraform Beta: Diff ( 3 files changed, 165 insertions(+), 167 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3225
Passed tests 2895
Skipped tests: 329
Affected tests: 1

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccFirestoreDatabase_firestoreDefaultDatabaseExample

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccFirestoreDatabase_firestoreDefaultDatabaseExample[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$


$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

@rwhogg rwhogg marked this pull request as ready for review November 14, 2023 20:10
@rwhogg
Copy link
Contributor Author

rwhogg commented Nov 14, 2023

@slevenick thanks a bunch for deleting the database, it works now!

@rwhogg
Copy link
Contributor Author

rwhogg commented Nov 16, 2023

@slevenick this is ready for review

Due to backwards compatibility concerns, the default behavior remains to
abandon the database upon destroy rather than to actually delete it.

To actually delete the database, you must set deletion_policy to DELETE,
and apply if necessary, before running `terraform destroy`.

This also cleans up some related deletion-related docs and bugs:

* Updates the delete protection docs
* delete_protection_state being enabled with deletion_policy = DELETE fails the destroy

Fixes hashicorp/terraform-provider-google#16488
Fixes hashicorp/terraform-provider-google#16404
Fixes hashicorp/terraform-provider-google#16325
@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 3 files changed, 165 insertions(+), 167 deletions(-))
Terraform Beta: Diff ( 3 files changed, 165 insertions(+), 167 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3237
Passed tests 2905
Skipped tests: 330
Affected tests: 2

Action taken

Found 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccHealthcareDatasetIamPolicy|TestAccFolderIamPolicy_basic

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccHealthcareDatasetIamPolicy[Debug log]
TestAccFolderIamPolicy_basic[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$


$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

@slevenick slevenick merged commit 4829cc4 into GoogleCloudPlatform:main Nov 21, 2023
14 checks passed
BBBmau pushed a commit to BBBmau/magic-modules that referenced this pull request Nov 28, 2023
Due to backwards compatibility concerns, the default behavior remains to
abandon the database upon destroy rather than to actually delete it.

To actually delete the database, you must set deletion_policy to DELETE,
and apply if necessary, before running `terraform destroy`.

This also cleans up some related deletion-related docs and bugs:

* Updates the delete protection docs
* delete_protection_state being enabled with deletion_policy = DELETE fails the destroy

Fixes hashicorp/terraform-provider-google#16488
Fixes hashicorp/terraform-provider-google#16404
Fixes hashicorp/terraform-provider-google#16325
jialei-chen pushed a commit to jialei-chen/magic-modules that referenced this pull request Nov 29, 2023
Due to backwards compatibility concerns, the default behavior remains to
abandon the database upon destroy rather than to actually delete it.

To actually delete the database, you must set deletion_policy to DELETE,
and apply if necessary, before running `terraform destroy`.

This also cleans up some related deletion-related docs and bugs:

* Updates the delete protection docs
* delete_protection_state being enabled with deletion_policy = DELETE fails the destroy

Fixes hashicorp/terraform-provider-google#16488
Fixes hashicorp/terraform-provider-google#16404
Fixes hashicorp/terraform-provider-google#16325
trodge pushed a commit to trodge/magic-modules that referenced this pull request Dec 8, 2023
Due to backwards compatibility concerns, the default behavior remains to
abandon the database upon destroy rather than to actually delete it.

To actually delete the database, you must set deletion_policy to DELETE,
and apply if necessary, before running `terraform destroy`.

This also cleans up some related deletion-related docs and bugs:

* Updates the delete protection docs
* delete_protection_state being enabled with deletion_policy = DELETE fails the destroy

Fixes hashicorp/terraform-provider-google#16488
Fixes hashicorp/terraform-provider-google#16404
Fixes hashicorp/terraform-provider-google#16325
@rwhogg rwhogg deleted the db-delete branch January 2, 2024 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants