-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat(backup): add indexActions prop to BackupPlanRule #34051
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
base: main
Are you sure you want to change the base?
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.
(This review is outdated)
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
|
||
/** | ||
* To help search your backups, you can enable Backup indexes by assigning index actions. | ||
* | ||
* @default - no index actions. | ||
*/ | ||
readonly indexActions?: BackupPlanIndexActionProps[]; |
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.
Based on the CloudFormation documentation, IndexActions
can have either 0
or 1
per backup rule - this limitation should be added to the property's documentation string. Since CloudFormation defines IndexAction as an array, I think we could flatten the L2 property with enum IndexAction
to make the API more intuitive, while keeping it as an array. Though AWS Backup currently accepts only 1 index action per backup rule, using array of enum would be more future-proof I assume if AWS later supports multiple index actions
plan.addRule(new backup.BackupPlanRule({
indexActions: [backup.IndexAction.S3]
}));
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.
@godwingrs22 I guess my only concern with flattening it is if they're planning to add more props other than ResourceTypes
in the future, which perhaps is why it's an array currently? I agree that in its current state it probably doesn't need to have any arrays at all and we could probably get away with indexAction: backup.IndexAction.S3
, but if they add another prop then we'd have to make quite a big change and implement something like indexActionsV2
.
Whereas with the current implementation, if they add a new prop then we'd only need to add that to the BackupPlanIndexActionProps
type.
Currently
indexActions: [{
resourceTypes: [backup.IndexActionResourceType.S3],
}],
Hypothetical future prop
indexActions: [
{
resourceTypes: [backup.IndexActionResourceType.S3, backup.IndexActionResourceType.EBS],
xyzProp: true,
},
{
resourceTypes: [backup.IndexActionResourceType.XYZ],
xyzProp: false,
},
],
I guess is a bit more simplicity now worth a potential rewrite in the future?
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Issue # (if applicable)
Closes #34050.
Reason for this change
Allows Backup indexes to be configured on specific resource types.
Description of changes
I have matched the CFN schema by creating
BackupPlanIndexActionProps
andIndexActionResourceType
. I'm not sure if it makes sense forindexActions
to be an array in its current state, but that's what CFN requires and maybe additional features are planned to utilise multiple actions.Describe any new or updated permissions being added
N/A
Description of how you validated changes
I have added a unit test, updated the backup integ test and deployed to verify the changes in the AWS console.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license