-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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(backup): support for advancedBackupSettings #14891
Conversation
Title does not follow the guidelines of Conventional Commits. Please adjust title before merge. |
Can you please review this @MrArnoldPalmer ? |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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.
Please consult the contributing guide for the requirements for pull requests in this repository.
In particular, please address the following issues:
- PRs should be named following the conventional commit specification (I fixed this for you already)
- public interfaces must have associated documentation
- new features must add to the README
- changes should modify or create tests
- build must pass
Besides these (somewhat) administrative issues, I'm not sure if this is the correct solution to implement #14803. Given that the only supported key for BackupOptions is "WindowsVSS", adding a property like "windowsVssBackup?: boolean" and generating the correct options will be more literate to the user.
@@ -27,6 +27,7 @@ export interface BackupPlanProps { | |||
* @default - A CDK generated name | |||
*/ | |||
readonly backupPlanName?: string; | |||
readonly backupOptions? : object; |
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.
public interface properties require documentation
@@ -125,6 +126,7 @@ export class BackupPlan extends Resource implements IBackupPlan { | |||
const plan = new CfnBackupPlan(this, 'Resource', { | |||
backupPlan: { | |||
backupPlanName: props.backupPlanName || id, | |||
backupOptions: props.backupOptions || {}, |
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 is not the correct location for this value, please consult the documentation for this resource.
924c117
to
ebfd5f2
Compare
Closes #14803. Reference Pull Request : #14891 Lets you set the 'WindowsVss' option when you create a new backup plan like this: ```ts const plan = new BackupPlan(stack, 'Plan', { windowsVss: true, }); ``` ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Closes aws#14803. Reference Pull Request : aws#14891 Lets you set the 'WindowsVss' option when you create a new backup plan like this: ```ts const plan = new BackupPlan(stack, 'Plan', { windowsVss: true, }); ``` ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
This PR has been deemed to be abandoned, and will be closed. Please create a new PR for these changes if you think this decision has been made in error. |
Fixes #14803