-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(dynamodb): add pointintimerecoveryspecification and deprecate old #33059
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #33059 +/- ##
=======================================
Coverage 81.52% 81.52%
=======================================
Files 224 224
Lines 13762 13762
Branches 2414 2414
=======================================
Hits 11220 11220
Misses 2270 2270
Partials 272 272
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 |
* | ||
* @default - point in time recovery is not enabled. | ||
*/ | ||
readonly pointInTimeRecoverySpecification?: PointInTimeRecoverySpecification; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
readonly pointInTimeRecoverySpecification?: PointInTimeRecoverySpecification; | |
readonly pointInTimeRecoveryPeriod?: Duration; |
We should follow the design guidelines and provide a flat interface:
- We can avoid deprecating the
pointInTimeRecovery
property and build thePointInTimeRecoverySpecification
CFN prop accordingly. - We should validate that
pointInTimeRecoveryPeriod
is between 1 and 35 days, and defaults to 35 (docs). - We should error if
pointInTimeRecovery
is explicitly set tofalse
andpointInTimeRecoveryPeriod
is set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review.
- I've discussed with the team on the continuation of extra fields for DynamoDB and this if we continue to utilize a flat interface we would have a very long list of parameters.
- Do we need this validation, as CFN validates this for us and will throw the appropriate exception?
- Same as above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @lpizzinidev for the detailed review.
-
I had discussions with Lee on the design. While the design guidelines advocate for flattening nested properties to accommodate languages like Java, given that TypeScript and Python users account for approximately 95% of all CDK users, it might be worth revisiting this guideline with the team.
In my view, flattening properties introduces unnecessary noise by adding all options to the top-level namespace, making it harder for users to identify the right property. For TypeScript and Python users, nested structures are not only idiomatic but also provide better organization and ease of use programmatically. -
For validation, it's an ongoing debate as well within the CDK team as some thinks we should leave the validation to CFN side in case if the rules change and CDK will need to make code changes to account for it (and often we won't be notified immediately) while some thinks validation on CDK side can help raise the error faster without deploying the template. IMO, I prefer to do the validation as well so I agree with you on this point.
-
I agree with you for the above reason.
Issue # (if applicable)
Closes #32786
Reason for this change
New feature of DynamoDB
Description of changes
Added
pointInTimeRecoverySpecification
which takespointInTimeRecoveryEnabled
andrecoveryPeriodInDays
.Deprecated
pointInTimeRecovery
as it could not takerecoveryPeriodInDays
Describe any new or updated permissions being added
Description of how you validated changes
Integ and Unit tests
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license