feat!: Add DevX Backups support to RDS instance construct #2276
Conversation
🦋 Changeset detectedLatest commit: 69ec42c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
jacobwinch
requested review from
adamnfish,
michaelwmcnamara,
AshCorr and
JuliaBrigitte
BREAKING CHANGE: Users of the GuDatabaseInstance class now need to explicitly opt-in/out of DevX Backups via the devXBackups prop.
jacobwinch
force-pushed
the
jw-rds-construct
branch
from
f4bbfa2 to
dc450f0
Compare
|
|
||
| export interface GuDatabaseInstanceProps | ||
| extends Omit<DatabaseInstanceProps, "instanceType" | "backupRetention" | "preferredBackupWindow">, | ||
| AppIdentity { | ||
| instanceType: string; |
There was a problem hiding this comment.
It seems a shame that we are removing the InstanceType type from instanceType and making it a string instead - I'd like to change this but it'd be a breaking change and is not linked to the PR, so have left it as is for now.
There was a problem hiding this comment.
Agree on both counts.
jacobwinch
changed the title
| Ref: Match.stringLikeRegexp("DatabaseInstanceTestingParameterGroup[A-Z0-9]+"), | ||
| }, | ||
| console.log(JSON.stringify(stack.tags.tagValues())); | ||
| GuTemplate.fromStack(stack).hasGuTaggedResource("AWS::RDS::DBInstance", { |
There was a problem hiding this comment.
We should add functionality to check for a single tag, but no worries here! This would encourage good test practice (one assertion per test), and eliminate noise from cases like this one where we are focused on a single tag.
There was a problem hiding this comment.
@adamnfish - there's a suggestion for this here: #2280.
What does this change?
This PR adds support for DevX Backups to our RDS instance construct. Users are now forced to opt-in/out of backups when provisioning an RDS instance via this construct. This makes it less likely that we will forget to add protection to important backups. Users who choose to provision an RDS instance via an AWS construct (instead of the GuCDK one) can still currently forget to make this choice (we will improve this in a follow up PR by modifying this aspect to check for the presence of the correct tag).
Additionally, we help to avoid conflicts by preventing users who opt-in to this feature from setting native RDS backup properties that cannot be managed via CloudFormation once AWS Backup is enabled. Users who opt-out of DevX Backups are still able to set these properties as there is no conflict.
The deletion of automated backups (on instance deletion) is now prevented by default (i.e. we override the CFN default). This is necessary where DevX Backups are enabled (the lock on these backups will prevent the instance from being deleted otherwise). In cases where users have opted-out of DevX Backups I still consider retaining the automated backups to be a sensible precaution so I have not made this change conditional.
Finally, I've removed some old unit tests that seemed to be testing functionality that is provided via the AWS libraries rather than our own construct: c5f348e.
How to test
I've added unit tests to cover the new functionality.
How can we measure success?
Users provisioning new RDS instances via GuCDK will be more likely to remember to opt-in to DevX Backups.
Have we considered potential risks?
This construct is not widely used and opting-in is an easy change, so I don't think this breaking change poses much of a risk in terms of generating work for teams.
Checklist
Footnotes
Consider whether this is something that will mean changes to projects that have already been migrated, or to the CDK CLI tool. If changes are required, consider adding a checklist here and/or linking to related PRs. ↩
If you are adding a new construct or pattern, has new documentation been added? If you are amending defaults or changing behaviour, are the existing docs still valid? ↩