-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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(codepipeline-actions): Add CAPABILITY_AUTO_EXPAND (#2851) #2852
Conversation
BREAKING CHANGE: the capabilities property is now an array to support multiple capabilities. Adds CAPABILITY_AUTO_EXPAND and support to define a list of capabilities for the CodePipeline action.
Properly handle CloudFormationCapabilities.None
Update app-handler to support capabilities[]
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.
Thank you for your contribution!
It looks pretty good, I really just have a couple of minor comments on there, which Ir reckon you should be able to chew through pretty quickly!
if (adminPermissions && capabilities === undefined) { | ||
// admin true default capability to NamedIAM | ||
return cfn.CloudFormationCapabilities.NamedIAM; | ||
return [cfn.CloudFormationCapabilities.NamedIAM]; |
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 wonder if this should not contain AutoExpand
, too.
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.
Yes, makes sense, I've added it. Please let me know if the changes are acceptable.
adminPermissions: false, | ||
}); | ||
new PipelineDeployStackAction(pipelineStack, 'DeployStack2', { | ||
stage: selfUpdateStage3, | ||
stack: stackWithAnonymousCapability, | ||
input: selfUpdatingStack.synthesizedApp, | ||
capabilities: cfn.CloudFormationCapabilities.AnonymousIAM, | ||
capabilities: [cfn.CloudFormationCapabilities.AnonymousIAM], |
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.
Would like to have an additional test that uses AutoExpand
.
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.
Added a couple more tests.
@@ -540,3 +540,14 @@ interface StatementTemplate { | |||
} | |||
|
|||
type StatementCondition = { [op: string]: { [attribute: string]: string } }; | |||
|
|||
function parseCapabilities(capabilities: any): any { |
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.
Can we avoid using any
here? I would rather those be tightly typed as much as possible.
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.
Yes, sorry for missing out on this one. I've updated the function.
Update capabilities() function with strong types.
Add CAPABILITY_AUTO_EXPAND to the default capabilities of the CloudFormation stack deploy role.
@RomainMuller sorry, I was delayed resolving the merge conflict. It should be alright now. |
BREAKING CHANGE: the capabilities property is now an array to support multiple capabilities.
Adds CAPABILITY_AUTO_EXPAND and support to define a list of capabilities for the CodePipeline action.
Closes #2851