feat(codepipeline): add support for CloudFormation StackSet actions #14225
Conversation
|
|
github-actions
bot
added
the
@aws-cdk/aws-codepipeline
|
Which README am I supposed to update? Edit: omg I can't believe I missed the big glaring README in aws-codepipeline-actions. Will update :) |
|
Thanks for the contribution @ndchelsea!
The ReadMe of the module you're changing - in this case, it should be the |
It should actually be codepipelineactions I think. I'll reword my commits - but I am wondering should I squash them or leave them separate? |
Yes, you're right, it should be As for the commits, it doesn't matter (they will all be squashed anyway when this is merged), so do whatever you prefer. |
ndchelsea
force-pushed
the
codepipeline_stacksetaction
branch
from
d49b064 to
f607d47
Compare
Sounds good - I fixed it. Should be all good now. |
There was a problem hiding this comment.
Thanks for the contribution @ndchelsea!
This is a very complicated feature. Right now, I'm not super happy with the customer-facing API for this. It's a maze of inter-dependent properties that is pretty much a direct translation of the CloudFormation schema. In the CDK, we really strive to make things easier to use, and harder to misuse. I think we need to introduce a few helper classes that will make it easier to use the CloudFormationStackSetAction. I've given you an example in the comments below, hopefully it's clear enough what I mean.
Let me know if you need any help with the details of the design of the API!
Thanks,
Adam
| @@ -997,3 +997,31 @@ pipeline.addStage({ | |||
|
|
|||
| See [the AWS documentation](https://docs.aws.amazon.com/codepipeline/latest/userguide/action-reference-StepFunctions.html) | |||
| for information on Action structure reference. | |||
|
|
|||
| ### AWS Stack Sets | |||
There was a problem hiding this comment.
Let's move this to the CloudFormation section of the ReadMe: https://github.com/aws/aws-cdk/blob/e7f09cb5159fbfad3a5c5fcbbc1fa8eab3d711dd/packages/@aws-cdk/aws-codepipeline-actions/README.md#aws-cloudformation
There was a problem hiding this comment.
does the updated one work? I got rid of the example since most things under cfn don't have examples.
...aws-cdk/aws-codepipeline-actions/test/cloudformation/cloudformation-pipeline-actions.test.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-codepipeline-actions/lib/cloudformation/pipeline-actions.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-codepipeline-actions/lib/cloudformation/pipeline-actions.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-codepipeline-actions/lib/cloudformation/pipeline-actions.ts
Outdated
Show resolved
Hide resolved
| * SELF_MANAGED permissions model, you can only provide accounts. | ||
| * | ||
| * Note | ||
| * When this parameter is selected, you must also select Regions. |
There was a problem hiding this comment.
Again, this constraint means we are missing a higher-level concept here that probably deserves its own class, and the deploymentTargets and regions properties should be merged.
| * "ou-examplerootid222-exampleouid222" | ||
| * ] | ||
| * | ||
| * One of deploymentTargets or deploymentTargetsPath is required, but not both. |
There was a problem hiding this comment.
Again - this means this should be one required property with a new type.
packages/@aws-cdk/aws-codepipeline-actions/lib/cloudformation/pipeline-actions.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-codepipeline-actions/lib/cloudformation/pipeline-actions.ts
Outdated
Show resolved
Hide resolved
| singletonPolicy.grantPassRole(cdk.Stack.of(scope).formatArn({ | ||
| region: '', | ||
| service: 'iam', | ||
| resource: 'role', | ||
| resourceName: 'AWSCloudFormationStackSetAdministrationRole', | ||
| })); |
There was a problem hiding this comment.
Instead of this, just use Role.fromRoleName(). This will also save you changing the signature of grantPassRole() below.
There was a problem hiding this comment.
I was originally going to do that (well, fromRoleArn) - no issue creating a construct within the bound method?
…hould have example now? (most others do not)
There was a problem hiding this comment.
Putting this in 'Request changes' to clear it from my ToDo list, @ndchelsea please re-request my review (there's a button in the top-right of the PR window, next to my avatar) when you're ready with the last changes!
|
Happy to help with any of this to get it merged in as we'd love to use this functionality. Anything I can do to help? Thanks for your efforts so far. |
RomainMuller
force-pushed
the
master
branch
from
924c117 to
ebfd5f2
Compare
|
@ndchelsea just checking in - were you planning on picking this up again? @dannyburke1 are you still interested in picking this up? |
|
@skinny85 yes, happy to help! Forgot about this, we still need the functionality. |
|
I think I have everything |
There was a problem hiding this comment.
Thanks for extracting that common interface for the props - I think that was the right direction, and it made the code of the actions so much smaller and easier to understand.
My only comment is that I still think the CloudFormationDeployStackInstancesAction class should have the word "StackSets" in it. I just think it's confusing without it. I understand you hate long names, and want to make them shorter, but this name is already long, and adding 3 more characters doesn't really change that.
So, my only ask is to rename CloudFormationDeployStackInstancesAction to CloudFormationDeployStackSetInstancesAction.
| * set. Then all instance statuses are set to OUTDATED until the changes are | ||
| * deployed to that instance. | ||
| */ | ||
| export class CloudFormationDeployStackInstancesAction extends Action { |
There was a problem hiding this comment.
I still think this action should contain the word "StackSet" in it, otherwise it's a little misleading.
I would ask for it to be renamed to CloudFormationDeployStackSetInstancesAction. I understand you hate long names, but I think those 3 characters here are really worth it.
|
|
||
| const instancesResult = this.props.stackInstances?._bind(scope); | ||
|
|
||
| if ((this.actionProperties.inputs || []).length > 0) { |
| /** | ||
| * Properties for the CloudFormationDeployStackInstancesAction | ||
| */ | ||
| export interface CloudFormationStackInstancesActionProps extends codepipeline.CommonAwsActionProps, CommonCloudFormationStackSetOptions { |
There was a problem hiding this comment.
I believe this should be called CloudFormationDeployStackInstancesActionProps.
skinny85
changed the title
| * Options in common between both StackSet actions | ||
| */ | ||
| export interface CommonCloudFormationStackSetOptions { | ||
|
|
There was a problem hiding this comment.
No need for this one?
Well. I am somewhat opposed to that, given that this is the term CloudFormation uses. In all existing material, they are not called "stack set instances", they are called "stack instances".
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/stacksets-concepts.html I'm not sure it's worth deviating from existing terminology here. |
|
In my opinion, it is. I don't think calling them "stack set instances" is in any way confusing or unclear. When using the CDK, you don't see the CloudFormation docs, you see the names we chose, and having "stack set" in the name clearly distinguishes these two classes from the existing 4 classes that deal with individual stacks. But I don't care that much, and I've already approved the PR, so the final call is up to you (although you probably want to address this comment: #14225 (comment) before merging the PR). |
mergify
bot
dismissed
skinny85’s stale review
Pull request has been modified.
|
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). |
…ws#14225) Adds support for CloudFormationStackSet CodePipeline actions. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
|
Hey just wanted to say thanks for finishing up this PR for me! I left the job I was at while this was up in the air and forgot about it in all the excitement. (I don't have access to ndchelsea anymore as it was on my work email)... anyway thanks again :D |
Adds support for CloudFormationStackSet CodePipeline actions.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license