-
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(pipeline): allow enabling KMS key rotation for cross-region Stacks #16468
Conversation
…ws-cdk into feat-pipelines-keyrotation
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 contribution @ppuritscher!
Like I said in the previous Pull Request (#14381 (comment)), we should strive to minimize the changes to existing templates, and so we should not default enableKeyRotation
to false
; instead, we should leave it as undefined
if it was not provided. This will have the consequence of not having to change all of the integration test files that you have currently changed in the PR (you'll be able to revert those changes).
Other than that, the changes look good!
packages/@aws-cdk/aws-codepipeline/lib/private/cross-region-support-stack.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-codepipeline/lib/private/cross-region-support-stack.ts
Outdated
Show resolved
Hide resolved
Pull request has been modified.
@skinny85 overread that. Fixed, thanks for the feedback! |
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.
Looks good @ppuritscher, but I still want to see a few changes before this is merged in 🙂.
Pull request has been modified.
Makes sense, thanks @skinny85! |
Pull request has been modified.
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.
@ppuritscher I think we're not understanding each other, so I'll try to spell out what I want to see here as clearly as I can 🙂.
Please revert all changes to the file packages/@aws-cdk/aws-codepipeline/README.md
besides the one paragraph that talks about the new property.
If it's still unclear how to do it:
- Open the file
packages/@aws-cdk/aws-codepipeline/README.md
in your editor. - Cut out the following paragraph:
If you want to enable key rotation for the generated KMS keys,
you can configure it by passing `enableKeyRotation: true` when creating the pipeline.
Note that key rotation will incur an additional cost of **$1/month**.
```ts
const pipeline = new codepipeline.Pipeline(this, 'MyFirstPipeline', {
// ...
enableKeyRotation: true,
});
```
- Execute the following command in the root of the project:
$ git checkout origin/master -- packages/@aws-cdk/aws-codepipeline/README.md
This will discard all changes to that file.
- Copy back the paragraph you cut out in step 2 into the same place you cut it from in the
packages/@aws-cdk/aws-codepipeline/README.md
file. - Commit the result, & push to your fork to update the PR.
Hope this is clear now!
Thanks,
Adam
packages/@aws-cdk/aws-codepipeline/lib/private/cross-region-support-stack.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-codepipeline/lib/private/cross-region-support-stack.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-codepipeline/lib/private/cross-region-support-stack.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-codepipeline/lib/private/cross-region-support-stack.ts
Outdated
Show resolved
Hide resolved
Hi @skinny85 ! You're right, sorry. Was only looking on the diffs your commenting on. Patrick |
…ws-cdk into feat-pipelines-keyrotation
…ws-cdk into feat-pipelines-keyrotation
Yep. Messed up the rebase. Will fix and get back to you. |
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.
Looks great @ppuritscher, thanks for the contribution!
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
1 similar comment
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
As suggested by @skinny85 I created an updated PR as successor of #14381
How does it work for v2. Do I need to create another PR for
v2-main
?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license