feat(dynamodb): recovery period in days for dynamodb table v1 #33010
Conversation
github-actions
bot
added
effort/medium
github-actions
bot
added
the
valued-contributor
| @@ -631,6 +631,15 @@ const table = new dynamodb.TableV2(this, 'Table', { | |||
| }); | |||
| ``` | |||
|
|
|||
| `recoveryPeriodInDays` specifies the number of preceding days for which continuous backups are taken and maintained. If this property is set without configuring `pointInTimeRecovery` then point-in-time recovery will be enabled by default. | |||
There was a problem hiding this comment.
Happy to discuss default behaviour documented here if there's any concerns. IMO it makes sense to set pointInTimeRecovery = true by default if recoveryPeriodInDays is set (with the ability to override pointInTimeRecovery = false if desired).
| pointInTimeRecoverySpecification = { pointInTimeRecoveryEnabled: true, recoveryPeriodInDays }; | ||
| } | ||
|
|
||
| if (pointInTimeRecovery != null) { |
There was a problem hiding this comment.
This was lifted-and-shifted from existing logic, but I think we should refactor this to a more precise check for undefined.
From this change, I understand the check was added to distinguish between undefined and false, which the proposed refactor would still fulfil.
Thoughts?
| if (pointInTimeRecovery != null) { | |
| if (pointInTimeRecovery !== undefined) { |
| pointInTimeRecovery?: boolean, | ||
| recoveryPeriodInDays?: number, | ||
| ): CfnTable.PointInTimeRecoverySpecificationProperty | undefined { | ||
| if (pointInTimeRecovery == null && recoveryPeriodInDays === undefined) return undefined; |
There was a problem hiding this comment.
I'd prefer to refactor this to the below suggestion but conditional on thoughts to this thread.
| if (pointInTimeRecovery == null && recoveryPeriodInDays === undefined) return undefined; | |
| if (pointInTimeRecovery === undefined && recoveryPeriodInDays === undefined) return undefined; |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #33010 +/- ##
=======================================
Coverage 81.39% 81.39%
=======================================
Files 225 225
Lines 13714 13714
Branches 2411 2411
=======================================
Hits 11162 11162
Misses 2277 2277
Partials 275 275
Flags with carried forward coverage won't be shown. Click here to find out more.
|
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Looks like there's ongoing changes to point-in-time recovery configuration so I'll keep this PR as a draft. May be able to close this if the other PR covers all required changes. |
|
Hi @rrhodes thank you for drafting this PR. There's actually another PR from internal AWS DynamoDB team and we're working to get that merged https://github.com/aws/aws-cdk/pull/33059/files. I apologize for the wasted effort but appreciate you for contributing. If you have any other PR that's pending review, feel free to ping me and I'll prioritize the review (so that this doesn't happen again). I'm going to close this PR in regards to the other PR we have. |
|
Comments on closed issues and PRs are hard for our team to see. |
Issue
Closes #32888.
Reason for this change
Support for point-in-time recovery period in days was introduced to L1 construct
CfnTablein PointInTimeRecoverySpecification.Description of changes
Added L2 construct support for point-in-time recovery period in days. This applies to the
Tableconstruct only. Happy to raise a separate PR forTableV2or revisit the scope of this PR on request.Describe any new or updated permissions being added
N/A.
Description of how you validated changes
New unit and integration test coverage provided.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license