Skip to content
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

Merged
merged 6 commits into from
Jun 21, 2019

Conversation

ScOut3R
Copy link
Contributor

@ScOut3R ScOut3R commented Jun 13, 2019

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

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.
@ScOut3R ScOut3R requested review from RomainMuller, skinny85 and a team as code owners June 13, 2019 03:33
@RomainMuller RomainMuller changed the title feat(aws-codepipeline-actions): Add CAPABILITY_AUTO_EXPAND (#2851) feat(codepipeline-actions): Add CAPABILITY_AUTO_EXPAND (#2851) Jun 14, 2019
Copy link
Contributor

@RomainMuller RomainMuller left a 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];
Copy link
Contributor

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.

Copy link
Contributor Author

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],
Copy link
Contributor

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.

Copy link
Contributor Author

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 {
Copy link
Contributor

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.

Copy link
Contributor Author

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.
@ScOut3R
Copy link
Contributor Author

ScOut3R commented Jun 20, 2019

@RomainMuller sorry, I was delayed resolving the merge conflict. It should be alright now.

@rix0rrr rix0rrr merged commit c9340a6 into aws:master Jun 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add CAPABILITY_AUTO_EXPAND
3 participants