-
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
Add version retention period to Spanner database resource #6141
Add version retention period to Spanner database resource #6141
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
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. @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. |
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. |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 3 files changed, 191 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFirebaserulesRelease_BasicRelease|TestAccContainerCluster_withConfidentialNodes|TestAccContainerCluster_withAddons|TestAccSpannerDatabase_basic|TestAccSpannerDatabaseIamPolicy|TestAccSpannerDatabaseIamMember|TestAccSpannerDatabaseIamBinding |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
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. |
1 similar comment
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. |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 6 files changed, 212 insertions(+), 20 deletions(-)) |
Regex previously didn't allow time periods of less than a day, but times of >1hour are valid in the API
afbeaf3
to
17266d8
Compare
Squashed a couple of my commits together to clean up the PR and accidentally removed the link between commits & modular-magician output - sorry about that! 😅 When I first made the PR I wasn't sure to handle how the new field cannot be set when creating a new Postgres DB. In this PR I've included ways that side-step this restriction in the API, but as the same isn't done for |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 7 files changed, 215 insertions(+), 20 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFirebaserulesRelease_BasicRelease|TestAccContainerCluster_withConfidentialNodes|TestAccContainerCluster_withAddons|TestAccSpannerDatabase_postgres |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 7 files changed, 215 insertions(+), 20 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFirebaserulesRelease_BasicRelease|TestAccContainerCluster_withConfidentialNodes|TestAccContainerCluster_withAddons|TestAccSpannerDatabase_basic|TestAccSpannerDatabaseIamPolicy|TestAccSpannerDatabaseIamMember|TestAccSpannerDatabaseIamBinding |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
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.
only minor asks, looks great overall. I'll address the open questions in another comment
and 7 days, and can be specified in days, hours, minutes, or seconds. For example, | ||
the values 1d, 24h, 1440m, and 86400s are equivalent. Default value is 1h. | ||
If this property is used, you must avoid adding new DDL statements to `ddl` that | ||
update the database's version_retention_period. |
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 we add a note similar to what database_dialect has for the double-apply limitation?
obj["extraStatements"] = extraStatements | ||
} | ||
if retentionPeriodOk && dialect == "POSTGRESQL" { | ||
// DDL statements other than <CREATE DATABASE> are not allowed in database creation request for PostgreSQL-enabled databases |
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 think it's possible to have a post-create function that waits for the resource to be created and then calls an update to set the retention period and dialect fields. This would mean two API requests in one apply, and should be a better UX unless I am missing something.
Looking at this earlier conversation it seems like this limitation was acknowledged and may be addressed in the future. Based on precedent, I think it is fine to leave as-is for 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.
Thanks for this comment - I'm new to magic-modules and wasn't aware of things like post_create code blocks that I could set. I had a chat with @megan07 and decided to try out moving all the DDL (originating from ddl
or version_retention_period
) into a post_create block that calls the updateDdl
endpoint after the DB is first made via the create
endpoint.
This means that this PR has also refactored how the ddl
field is handled, but I think that it's preferable to have the control flow the same regardless of database dialect and only use dialect == "POSTGRESQL"
to control the SQL syntax when statements are prepared.
It also means that Postgres DBs can be added in a Terraform config with DDL set and that config doesn't need to be applied multiple times to get to the end result. I've updated tests to reflect this, and removed warnings from the documentation.
(Added in commit 3b78b94)
We would ignore the field when verifying the create step like here: https://github.com/GoogleCloudPlatform/magic-modules/pull/5799/files#diff-bc14ae2ed214f27590f8fe2a8106235c9dee0fca59f31608041d97e025b257c8R143
Yep, but isn't too rare. Also see #6141 (comment)
It's hard for me to tell if it's better to try to warn users up front vs the convenience of not having to edit the config twice. Many users run Terraform effectively in a
That feels like a minor enough use case to not warrant the effort + maintenance, but let me know if you think otherwise. If they wanted to resume setting it via |
Co-authored-by: Cameron Thornton <[email protected]>
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 7 files changed, 215 insertions(+), 20 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFirebaserulesRelease_BasicRelease|TestAccContainerCluster_withConfidentialNodes|TestAccContainerCluster_withAddons |
Instead of DB dialect-specific logic, move all setting of DDL to a post-create, using updateDdl endpoint. Remove notes in documentation mentioning re-applying Terraform
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 7 files changed, 266 insertions(+), 32 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_withConfidentialNodes|TestAccContainerCluster_withAddons|TestAccSpannerDatabase_basic|TestAccSpannerDatabase_postgres |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 7 files changed, 416 insertions(+), 32 deletions(-)) |
Hi @c2thorn, thanks for your review. After I learned about the ability to add post_create blocks I changed approach for this PR - covered here in this comment - which goes against the precedent set by #5799 but I agree results in better UX. I believe I've updated the necessary tests & documentation to describe this and no longer warn about needing to apply the config 2 times. Also, re this from your last comment:
I added a new acceptance test in commit e302e6d that documents the expected behaviour of how the two fields interact - it's possible to swap back and forth between the two without any major downsides (other than out-of-date SQL in |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFirebaserulesRelease_BasicRelease|TestAccContainerCluster_withConfidentialNodes|TestAccContainerCluster_withAddons|TestAccSpannerDatabase_versionRetentionPeriod |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
// - changes the value (from 2h to 8h) | ||
// - is unstable; non-empty plan afterwards due to conflict | ||
Config: testAccSpannerDatabase_versionRetentionPeriodUpdate3(instanceName, databaseName), | ||
ExpectNonEmptyPlan: true, // is unstable |
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 expected behavior, for posterity
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.
Thanks for the improvements to ddl
as well. This LGTM
Description
Fixes hashicorp/terraform-provider-google#8994
This PR lets Terraform users set the retention period of historical versions of a Spanner database's data and schema for point in time recovery (PITR).
The retention period of the data used for PITR can be changed by running a DDL statement to update the database's options:
See usage notes section here & docs about equivalent DDL for Postgres
These DDL statements can be sent through the REST API and executed at either:
create
endpointextraStatements
array in the request bodyextraStatements
in the create request for a Postgres database, you get this error:DDL statements other than <CREATE DATABASE> are not allowed in database creation request for PostgreSQL-enabled databases
updateDdl
endpointApproach
I've used the value of the
version_retention_period
field to create an ALTER DDL statement like above and add it to the array of statements in the request bodies of requests sent to thecreate
orupdateDdl
endpointsEdit 2022-06-27 : I refactored the code for creating databases so that DDL statements (both originating from
ddl
andversion_retention_period
fields) are executed in the database via a separate API call. After the database is made using thecreate
endpoint the provider runs DDL against the new database using theupdateDdl
endpoint, all within the creation process in Terraform.Postgres issues
The DDL restriction when making Postgres databases has been met by the initial version of this PR by skipping setting the retention time during creation, even if theversion_retention_period
is set in the Terraform configuration. When a plan is generated a 2nd time from the Terraform configuration it will plan to update the retention time on the Postgres DB.This might be confusing to practitioners, but seems like the need to re-apply plans for Postgres databases isn't new and is mentioned in the docs.Edit 2022-06-27 : By separating all execution of DDL into an API call separate to the initial
create
API call this means Postgres's limitations in Spanner don't require 2nd apply stepsOpen questions
version_retention_period
being set before making the Postgres DB be better? That would avoid the confusion of needing to apply a plan twice to reach the desired goalALTER
statement in theddl
parameter of the Terraform provider? The only issue would be if someone addedversion_retention_period
to their config but then resumed setting it viaddl
(version_retention_period
would always re-assert itself on the next plan&apply)PR steps
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)