feat!: allow null values for backup_window and retention_period #184
Conversation
|
@srhopkins @dotCipher Can you share an estimate for when you'll get to review this PR? That would be very useful for my planning. Thank you! |
|
/terratest |
|
@hans-d Thanks! I see the error is
Which I can do nothing about. Does anyone need to be notified about the need for cleanup here? |
I'm already following up on this one - something I cannot fix myself as well unfortunately. |
|
Thanks @morremeyer for creating this pull request! A maintainer will review your changes shortly. Please don't be discouraged if it takes a while. While you wait, make sure to review our contributor guidelines. Tip Need help or want to ask for a PR review to be expedited?Join us on Slack in the |
morremeyer
force-pushed
the
feat-allow-null-values
branch
from
0be7fef
to
2f74876
Compare
This sets the default for backup_window and retention_period to `null`. When AWS backup is used, the settings either have to match the AWS backup settings exactly or they have to be unset. Trying to apply any diverging settings will result in an error: ``` InvalidParameterValue: RDS cluster $name is associated with the following AwsBackupRecoveryPointArn: arn:aws:backup:$region:$account:recovery-point:$mode:cluster-$id. The BackupRetentionPeriod can be blank, or you can use the current value, $current_value. For more details, see the AWS Backup documentation ``` Since matching these settings manually defeats the purpose of having a single source of truth, this change updates the default values to be `null`. I would prefer to make this a non-breaking change, but without adding another variable, this seems to not be possible since a default can't be overridden with `null`. BREAKING CHANGE: backup_window and retention_period are now `null` by default. If you want to keep the old default configuration, set `backup_window = "07:00-09:00"` and `retention_period = 5`.
morremeyer
force-pushed
the
feat-allow-null-values
branch
from
2f74876
to
8c0f79d
Compare
|
/terratest |
|
Sorry it took so long, @morremeyer I'm inclined to not accept the PR because
I generally disapprove of breaking changes that have easy non-breaking workarounds, and this one fits that category. |
|
@Nuru I agree with that, I always prefer non-breaking changes to breaking ones. However, when I implemented this, I did not see a way to make a non-breaking change. Let me re-examine how I came to this conclusion so that I can document it better - or ideally find out I was wrong! |
what
Sets the default for backup_window and retention_period to
null.BREAKING CHANGE: backup_window and retention_period are now
nullby default. If you want to keep the olddefault configuration, set
backup_window = "07:00-09:00"andretention_period = 5.why
When AWS backup is used, the settings either have to match the AWS backup settings exactly or they
have to be unset.
Trying to apply any diverging settings will result in an error:
Since matching these settings manually defeats the purpose of having a single source of truth,
this change updates the default values to be
null.I would prefer to make this a non-breaking change, but without adding another variable, this seems to not be possible
since a default can't be overridden with
null.references
This is possible since hashicorp/terraform-provider-aws#33465 was merged for
aws_rds_clusterresources.