-
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(aws-codepipeline): Pipeline now accepts existing IAM role #2587
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.
Please add a test as well.
* The IAM role to be assumed by this Pipeline. | ||
* If not specified, a new IAM role will be created. | ||
*/ | ||
readonly role?: iam.Role; |
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.
IRole
@@ -68,6 +68,12 @@ export interface PipelineProps { | |||
*/ | |||
readonly artifactBucket?: s3.IBucket; | |||
|
|||
/** | |||
* The IAM role to be assumed by this Pipeline. | |||
* If not specified, a new IAM role will be created. |
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.
Use a @default
marker in the docs to indicate the default behavior.
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.
@rix0rrr do you have an example of the @default
format that's expected? I'm not familiar with it, and I can't see where this is used anywhere else in this file
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.
Here is an example:
I would not copy the second line "Both supplied and generated...". That feature is a pattern all over the library that can be assumed and doesn't have to be documented in every location.
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.
Gotcha, done that. How does it look?
@rix0rrr sure 😄 - anything in particular that you'd want to be covered by this test? I'd copy from something like |
Yes, that if a role is passed in, it is referenced by the pipeline. |
@rix0rrr cool, I'll get on that - thanks! |
@rix0rrr I've added a test, mind having a look over and seeing what you think? I didn't think it fit into any of the existing test files so I created a new one |
Originally submitted as #2573, only this time without a horrible git history 😄
Fixes #2572.
Pull Request Checklist
design
folderBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.