feat!: allow null values for backup_window and retention_period

[morremeyer]

what

Sets the default for backup_window and retention_period to 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.

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:

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.

references

This is possible since https://github.com/hashicorp/terraform-provider-aws/issues/33465 was merged for aws_rds_cluster resources.

[morremeyer]

@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!

[hans-d]

/terratest

[morremeyer]

@hans-d Thanks! I see the error is

Error: creating EC2 VPC: operation error EC2: CreateVpc, https response error StatusCode: 400, RequestID: 9f342b56-35a2-405b-b548-b2f29f700952, api error VpcLimitExceeded: The maximum number of VPCs has been reached.

Which I can do nothing about. Does anyone need to be notified about the need for cleanup here?

[hans-d]

@hans-d Thanks! I see the error is

Error: creating EC2 VPC: operation error EC2: CreateVpc, https response error StatusCode: 400, RequestID: 9f342b56-35a2-405b-b548-b2f29f700952, api error VpcLimitExceeded: The maximum number of VPCs has been reached.

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.

[mergify[bot]]

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 #pr-reviews channel.

[morremeyer]

/terratest

[Nuru]

Sorry it took so long, @morremeyer

I'm inclined to not accept the PR because

• It is a breaking change
• Your statement "a default can't be overridden with null" is incorrect

I generally disapprove of breaking changes that have easy non-breaking workarounds, and this one fits that category.

[morremeyer]

@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!