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
Fix sql_database_instance to include backup_configuration in create request #6836
Conversation
…ial create request
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. |
@@ -472,7 +472,12 @@ func TestAccSqlDatabaseInstance_replica(t *testing.T) { | |||
Steps: []resource.TestStep{ | |||
resource.TestStep{ | |||
Config: fmt.Sprintf( | |||
testGoogleSqlDatabaseInstance_replica, databaseID, databaseID, databaseID), | |||
testGoogleSqlDatabaseInstance_replica, databaseID, databaseID, databaseID, "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.
Confirms that backup_configuration.enabled = true
fails for a replica
}, | ||
resource.TestStep{ | ||
Config: fmt.Sprintf( | ||
testGoogleSqlDatabaseInstance_replica, databaseID, databaseID, databaseID, "false"), |
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.
Confirms that backup_configuration.enabled = false
is allowed for a replica
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, 18 insertions(+), 9 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccSqlDatabaseInstance_replica|TestAccComputeForwardingRule_update|TestAccFirebaserulesRelease_BasicRelease |
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.
Definitely an oversight on the previous implementation. Thanks for the fix!
Fixes b/257534640 (TF plan allows google_sql_database_instance.replica to define a backup configuration)
SQL database instances do not allow replicas to enable backups. For example, the following config will fail:
Problem
Previously, config like this would correctly result in an error, but the replica would still be created with the
backup_configuration
omitted. This caused issues with consecutiveterraform apply
calls where the first succeeded, but the second failed with no changes to the config.Root cause
We were using some custom behavior for
backup_configuration.binary_log_enabled
that removed it from the initial create request, and then added it back to the instance with a subsequent patch request. This behavior had a bug that caused the entirebackup_configuration
to be removed and not get patched ifbinary_log_enabled
wasfalse
for a replica.Resolution
This PR proposes fixing the issue by leaving the
backup_configuration
on the create request, so that the server can validate the full configuration before creating any entities. The one exception isbinary_log_enabled
, which cannot be set totrue
until after a replica is created. In that case, we make the initial create request withbinary_log_enabled = false
, and then use the subsequent patch call to set it totrue
like before.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)