Add support for CloudFormation actions, continued #525

rix0rrr · 2018-08-08T13:19:55Z

Address review comments, fix input/output artifacts issue, expand README

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

@hallmaxw

--- Author's note: This work mostly belongs to Max Hall (@hallmaxw) It was at some point commented out during a refactor - this is just adding it back. Most of the credit should go to him. --- Anywho - this adds the ability for customers to use CloudForamtion actions in CodePipeline. This follows the same pattern as previous CodePipeline actions by making it it's own package.

rix0rrr · 2018-08-08T13:24:14Z

Because you're probably wondering, this is the inheritance diagram of actions, and what each shared class adds to the mix.

                  ┌─────────────────────────┐                                                
                  │                         │   stackname                                    
                  │  CloudFormationAction   │   outputartifact                               
                  │                         │                                                
                  └─────────────────────────┘                                                
                               ▲                                                             
                               │                                                             
             ┌─────────────────┴────────────────────────┐                                    
             │                                          │                                    
             │                                          │                                    
┌─────────────────────────┐             ┌──────────────────────────────┐                     
│                         │             │                              │                     
│    ExecuteChangeSet     │             │  CloudFormationDeployAction  │ permissions         
│                         │             │                              │                     
└─────────────────────────┘             └──────────────────────────────┘                     
                                                        ▲                                    
                                                        │                                    
                                ┌───────────────────────┼────────────────────────┐           
                                │                       │                        │           
                                │                       │                        │           
                     ┌────────────────────┐  ┌────────────────────┐   ┌─────────────────────┐
                     │                    │  │                    │   │                     │
                     │  PrepareChangeSet  │  │ CreateUpdateStack  │   │   DeleteStackOnly   │
                     │                    │  │                    │   │                     │
                     └────────────────────┘  └────────────────────┘   └─────────────────────┘

RomainMuller · 2018-08-08T13:26:59Z

packages/@aws-cdk/aws-cloudformation-codepipeline/lib/index.ts

@@ -0,0 +1 @@
+export * from './pipeline-action';


RomainMuller · 2018-08-08T13:27:28Z

packages/@aws-cdk/aws-cloudformation-codepipeline/lib/pipeline-action.ts

+import iam = require('@aws-cdk/aws-iam');
+import cdk = require('@aws-cdk/cdk');
+
+// tslint:disable:max-line-length


Because of the long URL

Disable only around that comment block?

RomainMuller · 2018-08-08T13:29:36Z

packages/@aws-cdk/aws-cloudformation-codepipeline/lib/pipeline-action.ts

+    /**
+     * The name of the output artifact to generate
+     *
+     * Only applied if `outputFileName` is set as well.


I'd be tempted to have

output?: { fileName: string; artifactName?: string; }

So you cannot specify outputArtifactName if it makes no sense.

Mmmyeah. But in general we keep the property namespace flat.

That's why I only say tempted.

Agree with Rico.Optimize for the common case which is to just specify the file name

Generally artifact names exist only to allow references in the Pipeline model which are achieved much better with object references in the CDK.

Copy: @skinny89

RomainMuller · 2018-08-08T13:30:58Z

packages/@aws-cdk/aws-cloudformation-codepipeline/lib/pipeline-action.ts

+        });
+
+        if (props.outputFileName) {
+            this.artifact = this.addOutputArtifact(props.outputArtifactName || (this.parent!.name + this.name + 'Artifact'));


Why use this.parent! when you have the guaranteed non-null parent available?

In the constructor parent is not optional, so therefore it must be assigned.

I mean why can't you use parent instead of this.parent!?

Oh. Well. I guess I could :)

RomainMuller · 2018-08-08T13:32:28Z

packages/@aws-cdk/aws-cloudformation-codepipeline/lib/pipeline-action.ts

+
+    constructor(parent: codepipeline.Stage, id: string, props: CloudFormationDeploymentActionCommonProps, configuration: any) {
+        super(parent, id, props, {
+            Capabilities: props.trustTemplate ? [CloudFormationCapabilities.NamedIAM] : props.capabilities,


So if you trust the template, you cannot add other capabilities than NamedIAM?

Oh yes, good point.

Although, what other capabilities are there :)

Right now, none, but nothing says it'll remain like this.

I wouldn’t future proof this

It's risky... very likely to be overlooked if/when new capabilities get added. And the fix is not horribly expensive either...

RomainMuller · 2018-08-08T13:33:24Z

packages/@aws-cdk/aws-cloudformation-codepipeline/lib/pipeline-action.ts

+            ParameterOverrides: props.parameterOverrides,
+            TemplateConfiguration: props.templateConfiguration,
+            StackName: props.stackName,
+            ...configuration,


I'd move ...configuration up-front, because otherwise it can override the values you specified before...

RomainMuller · 2018-08-08T13:34:41Z

packages/@aws-cdk/aws-cloudformation-codepipeline/lib/pipeline-action.ts

+        });
+
+        if (props.role) {
+            this.role = props.role;


If you specify your own role, and say you trust the template, the policy doesn't get amended to extend any action on all resources. This seems to contradict the documentation of trustTemplate.

It does not contradict the documentation of role, and it was intended this way (similar to default of capabilities). This is a shortcut if you can't be bothered to specify anything. If you can, then you should just spell it out all the way.

Ah no, the docs say it adds to the "default role", which is only the role that gets created if you don't specify anything.

Feels to me like a warning probably should be emitted...

RomainMuller · 2018-08-08T13:37:53Z

packages/@aws-cdk/aws-cloudformation-codepipeline/tsconfig.json

+    "jsx": "react",
+    "jsxFactory": "jsx.create"
+  },
+  "_generated_by_jsii_": "generated by jsii - you can delete, and ideally add to your .gitignore"


Do what the tool says!

eladb · 2018-08-08T13:26:05Z

packages/@aws-cdk/aws-cloudformation-codepipeline/lib/pipeline-action.ts

+     *
+     * @default false
+     */
+    trustTemplate?: boolean;


I think something like fullTrust or adminPrivileges or root. Something that has to do with "admin"

eladb · 2018-08-08T13:29:23Z

packages/@aws-cdk/aws-cloudformation-codepipeline/lib/pipeline-action.ts

+    /**
+     * Whether the deployed templates are trusted.
+     *
+     * If `true`, the default `role` will have full permissions and the default


Be more explicit in the docs about the IAM policy that this implies

eladb · 2018-08-08T13:32:31Z

packages/@aws-cdk/aws-cloudformation-codepipeline/test/integ.pipeline.ts

+const changeSetName = "ChangeSetIntegTest";
+const stackName = "IntegTest-TestActionStack";
+
+const role = new Role(stack, 'CfnChangeSetRole', {


Looks like the default role

It is, now we're integ-testing both variants :).

RomainMuller · 2018-08-08T13:43:13Z

packages/@aws-cdk/aws-cloudformation-codepipeline/package.json

+          "artifactId": "cloudformation-codepipeline"
+        }
+      },
+      "dotnet": {


You'd need to remove this target from here and "re-add" it into #524 ideally.

…rmation-actions

RomainMuller · 2018-08-08T14:08:04Z

packages/@aws-cdk/aws-cloudformation-codepipeline/test/test.pipeline-actions.ts

+import cdk = require('@aws-cdk/cdk');
+import { PolicyStatement, ServicePrincipal } from '@aws-cdk/cdk';
+import { Test } from 'nodeunit';
+import { CreateReplaceChangeSet, CreateUpdateStack, ExecuteChangeSet } from '../lib/pipeline-action';


missing 's'

…bs/aws-cdk into mindstorms6/cloudformation-actions

mindstorms6 and others added 2 commits August 5, 2018 14:53

Address review comments, fix input/output artifacts issue, expand README

b9ddbaa

rix0rrr requested review from eladb and RomainMuller August 8, 2018 13:19

rix0rrr mentioned this pull request Aug 8, 2018

Add support for CloudFormation actions in CodePipeline #509

Closed

RomainMuller reviewed Aug 8, 2018

View reviewed changes

eladb approved these changes Aug 8, 2018

View reviewed changes

RomainMuller reviewed Aug 8, 2018

View reviewed changes

Rico Huijbers added 3 commits August 8, 2018 15:49

Address review comments

ed3f01e

Improve naming and documentation around "full" permissions.

5c23983

Merge remote-tracking branch 'origin/master' into mindstorms6/cloudfo…

640f003

…rmation-actions

RomainMuller approved these changes Aug 8, 2018

View reviewed changes

RomainMuller reviewed Aug 8, 2018

View reviewed changes

RomainMuller and others added 4 commits August 8, 2018 16:08

Add missing s

bc3fa64

Final comments, make integ test use fullPermissions flag

116c4bb

Merge branch 'mindstorms6/cloudformation-actions' of github.com:awsla…

9a43da8

…bs/aws-cdk into mindstorms6/cloudformation-actions

Update test to match new property name

166f732

RomainMuller approved these changes Aug 8, 2018

View reviewed changes

rix0rrr merged commit c618614 into master Aug 8, 2018

rix0rrr deleted the mindstorms6/cloudformation-actions branch August 8, 2018 14:45

RomainMuller mentioned this pull request Aug 8, 2018

v0.8.1 #523

Merged

NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for CloudFormation actions, continued #525

Add support for CloudFormation actions, continued #525

rix0rrr commented Aug 8, 2018

rix0rrr commented Aug 8, 2018

RomainMuller Aug 8, 2018

RomainMuller Aug 8, 2018

rix0rrr Aug 8, 2018

RomainMuller Aug 8, 2018

RomainMuller Aug 8, 2018

rix0rrr Aug 8, 2018

RomainMuller Aug 8, 2018

eladb Aug 8, 2018

eladb Aug 8, 2018

RomainMuller Aug 8, 2018

rix0rrr Aug 8, 2018

RomainMuller Aug 8, 2018

rix0rrr Aug 8, 2018

RomainMuller Aug 8, 2018

rix0rrr Aug 8, 2018

rix0rrr Aug 8, 2018

RomainMuller Aug 8, 2018

eladb Aug 8, 2018

RomainMuller Aug 8, 2018

RomainMuller Aug 8, 2018

RomainMuller Aug 8, 2018

rix0rrr Aug 8, 2018

rix0rrr Aug 8, 2018

RomainMuller Aug 8, 2018

RomainMuller Aug 8, 2018

eladb Aug 8, 2018

eladb Aug 8, 2018

eladb Aug 8, 2018

rix0rrr Aug 8, 2018

RomainMuller Aug 8, 2018

RomainMuller Aug 8, 2018

Add support for CloudFormation actions, continued #525

Add support for CloudFormation actions, continued #525

Conversation

rix0rrr commented Aug 8, 2018

rix0rrr commented Aug 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment