feat(backup): support continuous backup and point-in-time restores #17602
Conversation
|
|
jumic
changed the title
jumic
changed the title
There was a problem hiding this comment.
Thanks for the detailed PR description, it was very helpful. My main comment is that I think we should default deleteAfter to Duration.days(35) if not supplied. We should still be documenting these limitations both in the README and in the docstring as well, though.
mergify
bot
dismissed
kaizencc’s stale review
Pull request has been modified.
| } | ||
|
|
||
| if (props.enableContinuousBackup && props.deleteAfter && | ||
| ((props.deleteAfter?.toSeconds() < Duration.days(1).toSeconds() || |
There was a problem hiding this comment.
Here I would prefer just props.deleteAfter?.toDays() < 1 || props.deleteAfter?.toDays() > 35
There was a problem hiding this comment.
I tried to change it. Now, the test case failed with message
Received message: "'12 hours' cannot be converted into a whole number of days.".
Should I change it anyway? I would prefer the existing solution because it shows a clear error message.
There was a problem hiding this comment.
This actually requires some more investigation. Can you confirm the granularity that deleteAfter allows? I know its called deleteAfterDays on cloudformation but that it also accepts a double. Which makes me think we are allowed fractions of days. On our side, we convert deleteAfter to days here (so we would run into the message you got anyway):
This could be a small bug, in that our checks are more stringent then cloudformation.
If you're interested in checking this out, please do! If not, I like even more the conditional I suggested. It looks like the current CDK behavior is that deleteAfter must be a whole number of days, so it is good to stay with that level of granularity here.
There was a problem hiding this comment.
I changed it as proposed. I checked some other modules in CDK where Duration is used. It's implemented in the same way.
The granularity of deleteAfter is days. Message "'12 hours' cannot be converted into a whole number of days." indicates that developers have to specify deleteAfter in days if something else is used. If this is resolved, the logic checks if deleteAfter has a valid range. So it's fine for me. :)
There was a problem hiding this comment.
Perhaps I am misunderstanding you. But the '12 hours... error message comes from the line I linked above. It is a CDK limitation. I am weary of this because it seems like Cloudformation may allow fractions of days. This is good for now, I'll create an issue to investigate this further.
mergify
bot
dismissed
kaizencc’s stale review
Pull request has been modified.
mergify
bot
dismissed
kaizencc’s stale review
Pull request has been modified.
|
Build failed ( |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
…ws#17602) Adds support for [continuous backup and point-in-time restores](https://docs.aws.amazon.com/aws-backup/latest/devguide/point-in-time-recovery.html). Implemented validations when continuous backup and point-in-time restores is enabled: - `deleteAfter` between 1 and 35 days. "The minimum retention period is 1 day. The maximum retention period is 35 days." (see [docs](https://docs.aws.amazon.com/aws-backup/latest/devguide/point-in-time-recovery.html#point-in-time-recovery-working-with)) - `deleteAfter` must be specified. Mandatory in AWS console. CloudFormation error if not specified: `Lifecycle must be specified for backup rule enabled continuous backup` - `moveToColdStorageAfter` is not supported. Field not available in AWS console. CloudFormation error if specified: `MoveToColdStorageAfterDays is unavailable` Closes aws#15922. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Adds support for continuous backup and point-in-time restores.
Implemented validations when continuous backup and point-in-time restores is enabled:
deleteAfterbetween 1 and 35 days. "The minimum retention period is 1 day. The maximum retention period is 35 days." (see docs)deleteAftermust be specified. Mandatory in AWS console. CloudFormation error if not specified:Lifecycle must be specified for backup rule enabled continuous backupmoveToColdStorageAfteris not supported. Field not available in AWS console. CloudFormation error if specified:MoveToColdStorageAfterDays is unavailableCloses #15922.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license