-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat(dynamodb): adding ContributorInsightsMode feature #35293
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
Conversation
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 your contribution! It looks like two similar functions here seem like they can be combined into one shared utility?
| } | ||
|
|
||
| private validateCCI (props: IContributorInsightsConfigurable): ContributorInsightsSpecification | undefined { | ||
| if (props.contributorInsightsSpecification !==undefined && props.contributorInsightsEnabled !== undefined) { |
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.
This looks very similar to the validateCCI function in table-v2. Can we combine them in the shared file as a utility? They essentially do the same thing, and you would need to parameterize the old and new names.
Also, looks like code here might not be linted?
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.
They are similar, but for TableV2 we also have to check for replicas. I also did not see any other shared utility functions in the shared file, so I created two standalone functions. This passed the internal view already, but I can combine them into a more complex function if you feel its the right direction?
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.
I agree with @iankhou, since they are quite similar, you could do, in the shared file, something like:
export function validateContributorInsights(
contributorInsights: boolean | undefined,
contributorInsightsSpecification: ContributorInsightsSpecification | undefined,
deprecatedPropertyName: string,
construct: Construct
): ContributorInsightsSpecification | undefined {
if (contributorInsightsSpecification !== undefined && contributorInsights !== undefined) {
throw new ValidationError(`\`contributorInsightsSpecification\` and \`${deprecatedPropertyName}\` are set. Use \`contributorInsightsSpecification\` only.`, construct);
}
return contributorInsightsSpecification ??
(contributorInsights !== undefined ? { enabled: contributorInsights } : undefined);
}
And then on each validation in the table and table-v2:
// Updated table-v2.ts - replace the validateCCI method with:
private validateCCI(props: IContributorInsightsConfigurable): ContributorInsightsSpecification | undefined {
const contributorInsights = props?.contributorInsights ?? this.tableOptions?.contributorInsights;
const contributorInsightsSpecification = props?.contributorInsightsSpecification || this.tableOptions?.contributorInsightsSpecification;
return validateContributorInsights(contributorInsights, contributorInsightsSpecification, 'contributorInsights', this);
}
// Updated table.ts - replace the validateCCI method with:
private validateCCI(props: IContributorInsightsConfigurable): ContributorInsightsSpecification | undefined {
return validateContributorInsights(props.contributorInsightsEnabled, props.contributorInsightsSpecification, 'contributorInsightsEnabled', this);
}
This way you still keep the table v2 specific validation logic where it belongs (table-v2 file) but dont repeat code if not needed.
|
@LeeroyHannigan there was an issue with this build, and since I could not push the commit change to your branch, I created a duplicate of this PR and added the fix (completely unrelated to what you did, but still blocking the build). That PR was successfully merged, so I will close this one. The duplicate is this one: #35332 |
|
Comments on closed issues and PRs are hard for our team to see. |
Reason for this change
Adding new feature for DynamoDB Contributor Insights Mode: https://docs.aws.amazon.com/AWSCloudFormation/latest/TemplateReference/aws-properties-dynamodb-table-contributorinsightsspecification.html#cfn-dynamodb-table-contributorinsightsspecification-mode
Description of changes
Changes to Table and TableV2 to provide CCI Mode
Describe any new or updated permissions being added
Description of how you validated changes
Yes, 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