Proposal for final CodePipeline Actions API shape #459

skinny85 · 2018-07-31T23:58:44Z

This PR contains what will (hopefully) be the final iteration of the way we structure our CodePipeline Actions.

The API shape is the following:

All Actions integrating with AWS services live in the service's package (so, the CodeCommit Action lives in the aws-codecommit package, the CodeBuild ones - in aws-codebuild, etc.).
There is a separate aws-codepipeline-api package that contains the Actions API, on which the service packages depend on (in order to prevent dependency cycles).
The API of Actions changed - you now provide a Construct as the parent, while the Stage is a required property in the props.

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

eladb · 2018-08-01T05:27:57Z

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

@@ -0,0 +1,59 @@
+{


Sadly, there is no support for “re-exporting” symbols in jsii, which means we can’t vend aws-actions to all languages. What I suggest is providing pointers in the codepipeline README to all action types. This will be where people will naturally look for them.

What if we kept the aws-codepipeline-aws-actions module in other languages, but remove the re-exporting part? This way, it would leave it as only depending on the other Action modules, and in this way make it a little easier for customers to just import all of them in one go. Does that have enough value?

Also, any comments on the changed names of the Actions?

aws-actions feels convoluted and confusing... how about we start without, and see if it's needed?
No comments on the the action names - 👍

skinny85 · 2018-08-02T22:46:45Z

Thanks for the review @eladb . I've submitted a new version, removing the aws-codebuild-aws-actions module completely, and updating the ReadMe of the aws-codepipeline module.

If this looks right to you, feel free to merge this in.

skinny85 · 2018-08-03T00:07:15Z

Missed updating some outdated comments in the actions.ts file in the aws-codepipeline package in the previous revision, corrected now.

eladb · 2018-08-03T04:52:00Z

packages/@aws-cdk/aws-codepipeline/README.md

-    }),
-})
-```
+* [`aws-codecommit-codepipeline`](../aws-codecommit-codepipeline) contains the AWS CodeCommit source Action


These links are not going to work outside of the repo (i.e. in our docs).
Perhaps you can just put links to the doc website: e.g. https://awslabs.github.io/aws-cdk/refs/_aws-cdk_aws-config.html

eladb · 2018-08-03T04:53:19Z

packages/@aws-cdk/aws-codepipeline/README.md

-```ts
-const sourceStage = new Stage(pipeline, 'Source');
-```
+The most popular Actions for external integrations


I wouldn't use the term "the most popular".
Also, the word "integration", which I know is used internally, is a bit confusing. I would just use something like "Pipeline actions for external services" and then, "Pipeline actions for AWS services"

Will change.

skinny85 · 2018-08-03T21:29:39Z

I had one more thought in favor of the aws-codepipeline-aws-actionsmodule (I'm not in love with the name either just to be clear, I'm open to suggestions on a better one) that I wanted your opinion on @eladb .

Currently, we face a question of where to put the external Actions (like GitHub source, Jenkins build, Jenkins test, etc.). Right now, they live in the codepipeline package, but that makes them "special", and different from the AWS Actions. Also, with 100% certainty I can say that not all of the external Actions available in the CodePipeline UI will be in that package (I don't imagine we will put the less popular ones, like Solano CI and Ghost Inspector UI Testing, for example, in there).

We could create a dedicated package for them, something like aws-codepipeline-external-actions (working name). In that case, it seems like having an aws-codepipeline-aws-actions module could be nice, for symmetry and discoverability.

We could even make aws-codepipeline-external-actions be just an empty package that depends on the particular integrations package (like aws-codepipeline-github, aws-codepipeline-jenkins, etc.), to make it more similar with AWS Actions.

I would love to know what's your opinion on this @eladb .

Thanks!

skinny85 · 2018-08-10T01:14:00Z

After our meeting, I've completely re-structured the API according to what we agreed to. I've updated the PR description accordingly.

Sorry for the gigantic CR, but I'd rather rip the band-aid off. Also, it's 95% just moving stuff around (and the necessary changes resulting from that moving around, like different imports because of changing package memberships, etc.).

eladb · 2018-08-10T06:47:29Z

packages/@aws-cdk/aws-codepipeline-api/README.md

@@ -0,0 +1 @@
+## AWS CodePipeline Actions API


Worth documenting this module..:

Yep, a miss on my part. Will fix.

eladb · 2018-08-10T06:49:06Z

packages/@aws-cdk/aws-codepipeline-api/lib/action.ts

+    };
+}
+
+export interface IBucketPermissions {


Please add extensive jsdocs to all these APIs

It's strictly an internal API (not customer facing), but I agree some comments are needed.

eladb · 2018-08-10T06:49:45Z

packages/@aws-cdk/aws-codepipeline-api/lib/action.ts

+export interface IStage {
+    readonly name: string;
+    readonly pipelineArn: cdk.Arn;
+    readonly pipelineRole: iam.Role;


Instead of exposing the role, can we expose a function ‘addToRolePolcy’?

We cannot. In the S3 Action, we pass pipelineRole as a parameter into Bucket#grantRead.

eladb · 2018-08-10T06:50:36Z

packages/@aws-cdk/aws-codepipeline-api/lib/action.ts

+}
+
+export interface IStage {
+    readonly name: string;


Why do actions need access to the stage name?

It's used in 2 places right now:

The onStateChange method in Action class (the root of the Actions hierarchy).

The CloudFormationAction abstract class (uses it to name the output artifact).

eladb · 2018-08-10T06:52:24Z

packages/@aws-cdk/aws-codepipeline-api/lib/action.ts

+    _addAction(action: Action): void;
+}
+
+export interface AbstractActionProps {


I think this might be redundant. If you require this prop by the abstract base class it will force everyone to include it in their props. It also has the benefit of have “stage” appear in docs in every actio props. Otherwise, people will have to traverse to the hierarchy.

We can always add this abstraction in the future if we see there’s More to it than just “stage” but this feels like taking it one step too far

The reason I did it this way is otherwise, we will copy & paste the same documentation for the stage property into 8+ different places. Granted, I didn't write any JsDocs for this property either, but that's a separate discussion ;p.

Not sure that's a good enough reason... It's not such a complicated description...

I actually have a second candidate property for this interface in mind: runOrder.

Does having 2 properties here make this interface valuable enough, in your opinion?

eladb · 2018-08-10T06:57:13Z

packages/@aws-cdk/aws-codepipeline/lib/s3-source-action.ts

+/**
+ * Construction properties of the {@link AmazonS3Source S3 source action}.
+ */
+export interface AmazonS3SourceProps extends actions.AbstractActionProps {


Shouldn’t that be in S3?

Yes. It will be moved as part of #492 once this is shipped (or I'll rebase #492 on top of this, depending on how much back-and-forth we'll have).

eladb · 2018-08-10T06:58:46Z

packages/@aws-cdk/aws-codepipeline-api/lib/action.ts

+    readonly name: string;
+    readonly pipelineArn: cdk.Arn;
+    readonly pipelineRole: iam.Role;
+    readonly bucketPermissions: IBucketPermissions;


Same here: keep it flat: “grantBucketReadWrite”

Considering it will be called on stage, how about: grantPipelineBucketReadWrite?

eladb · 2018-08-10T07:01:52Z

packages/@aws-cdk/aws-codepipeline-api/lib/action.ts

+
+// export class ECSDeploy extends DeployAction {
+//     constructor(parent: Stage, name: string, clusterName: string, serviceName: string, fileName?: string) {
+//         super(parent, name, 'ECS', { minInputs: 1, maxInputs: 1, minOutputs: 0, maxOutputs: 0 }, {


Can we save this code somewhere else (issue?) instead of commented out.. it’s a bit messy

I have a backlog item in our internal issue tracker for each of these - trust me, I will not forget them :).

I was referring to the commented out code. Let's remove it from here. If you want to "save" it somewhere, just paste into your issue.

eladb · 2018-08-10T07:04:19Z

packages/@aws-cdk/aws-codepipeline/lib/stage.ts

@@ -29,25 +41,37 @@ export class Stage extends cdk.Construct {
     */
    constructor(parent: Pipeline, name: string) {


I was expecting the pipeline/stage relationship to also be modeled more idiomatically as a prop
Instead of abusing the construct tree. Let’s align the API completely

Agreed 100%, but, like I said above, I'd rather do that in a separate PR than make this one even bigger than it already is.

eladb · 2018-08-10T07:06:04Z

packages/@aws-cdk/aws-codepipeline/lib/stage.ts

    }

    /**
     * Get a duplicate of this stage's list of actions.
     */
-    public get actions(): Action[] {
+    public get actions(): actions.Action[] {
        return this._actions.slice();
    }

    public validate(): string[] {
        return this.validateHasActions();
    }



This looks like what I proposed earlier instead of another interface. I would allude to the bucket in the name of the method (because stage.grantReadWrite) is weird

Like I proposed above: does stage.grantPipelineBucketReadWrite sound good to you?

Yes, if you prefer (I hate long names). Since this method is part of the pipeline's stage, I think "pipeline" is not really needed by the context, but that's fine :-)

skinny85 · 2018-08-10T18:24:55Z

Updated with the feedback from the comments.

skinny85 · 2018-08-10T18:44:44Z

Missed a VS-Code auto-generated method signature with a union type.

skinny85 · 2018-08-11T01:41:21Z

No idea why the build is failing... Stage definitely implements grantPipelineBucketReadWriteTo correctly in TypeScript... could it be because the type in the declaration of grantPipelineBucketReadWriteTo is optional (identity?: iam.IPrincipal)?

eladb · 2018-08-12T06:45:21Z

packages/@aws-cdk/aws-cloudformation-codepipeline/README.md

@@ -1,24 +0,0 @@
-## AWS CodePipline Actions for AWS CloudFormation


Looks like a merge issue. Probably needs to be merged into aws-cloudformation

Sorry, I'm not following this comment 🤔This file has been removed (alongside the entire aws-cloudformation-codepipeline package). This text has been added into the aws-cloudformation README file (here).

Oops you are right

eladb · 2018-08-12T06:55:44Z

packages/@aws-cdk/aws-codepipeline-api/lib/action.ts

+/**
+ * Common properties shared by all Actions.
+ */
+export interface AbstractActionProps {


I still think this is redundant, but if you insist, I'd call this CommonActionProps.

eladb · 2018-08-12T06:56:11Z

packages/@aws-cdk/aws-codepipeline-api/lib/action.ts

+     *
+     * @param identity the IAM Identity to grant the permissions to
+     */
+    grantPipelineBucketReadWriteTo(identity?: iam.IPrincipal): void;


Rename the To suffix. This is not Objective-C here :-)

When you say "rename the To suffix"... do you mean "drop it"? Cause I'm not sure what to rename it to (no pun intended)...

Yes. “Remove” (typo)

eladb · 2018-08-12T06:56:33Z

packages/@aws-cdk/aws-codepipeline-api/lib/action.ts

+}
+
+export interface ActionProps extends AbstractActionProps {
+    category: ActionCategory;


document...

eladb · 2018-08-12T07:19:41Z

Regarding the build break. Seems like a jsii bug, but we need to dive deeper. You should be able to run jsii-pacmak --recurse from your package and it will generate and build the java version of your module:

lerna exec jsii-pacmak --scope="@aws-cdk/aws-codepipeline-api" -- --recurse --no-clean -v

--no-clean will preserve the sources (you should see a path somewhere in the output), which will allow you to investigate a bit further.

Let us know what you find out...

skinny85 · 2018-08-13T20:23:28Z

This is the JSII-generated code for IStage:

package software.amazon.awscdk.services.codepipeline.api;
/**
 * The abstract interface of a Pipeline Stage that is used by Actions.
 */
public interface IStage extends software.amazon.jsii.JsiiSerializable {
    /**
     * The physical, human-readable name of this Pipeline Stage.
     */
    java.lang.String getName();
    /**
     * The ARN of the Pipeline.
     */
    software.amazon.awscdk.Arn getPipelineArn();
    /**
     * The service Role of the Pipeline.
     */
    software.amazon.awscdk.services.iam.Role getPipelineRole();
    /**
     * Grants read & write permissions to the Pipeline's S3 Bucket to the given Identity.
     * @param identity the IAM Identity to grant the permissions to
     */
    void grantPipelineBucketReadWriteTo(@javax.annotation.Nullable final software.amazon.awscdk.services.iam.IPrincipal identity);
    /**
     * Grants read & write permissions to the Pipeline's S3 Bucket to the given Identity.
     */
    void grantPipelineBucketReadWriteTo();

    /**
     * A proxy class which for javascript object literal which adhere to this interface.
     */
    class Jsii$Proxy extends software.amazon.jsii.JsiiObject implements software.amazon.awscdk.services.codepipeline.api.IStage {
        protected Jsii$Proxy(final software.amazon.jsii.JsiiObject.InitializationMode mode) {
            super(mode);
        }
        /**
         * The physical, human-readable name of this Pipeline Stage.
         */
        public java.lang.String getName() {
            return this.jsiiGet("name", java.lang.String.class);
        }
        /**
         * The ARN of the Pipeline.
         */
        public software.amazon.awscdk.Arn getPipelineArn() {
            return this.jsiiGet("pipelineArn", software.amazon.awscdk.Arn.class);
        }
        /**
         * The service Role of the Pipeline.
         */
        public software.amazon.awscdk.services.iam.Role getPipelineRole() {
            return this.jsiiGet("pipelineRole", software.amazon.awscdk.services.iam.Role.class);
        }
        /**
         * Grants read & write permissions to the Pipeline's S3 Bucket to the given Identity.
         * @param identity the IAM Identity to grant the permissions to
         */
        public void grantPipelineBucketReadWriteTo(@javax.annotation.Nullable final software.amazon.awscdk.services.iam.IPrincipal identity) {
            this.jsiiCall("grantPipelineBucketReadWriteTo", Void.class, java.util.stream.Stream.of(identity).toArray());
        }
    }
}

The interface has 2 variants of the grantPipelineBucketReadWriteTo method - one with an argument, one without, while the Jsii$Proxy only implements the one with the argument.

skinny85 · 2018-08-13T22:18:43Z

Updated with the latest comments from @eladb . I won't merge until we figure out the above problem with JSII though.

skinny85 · 2018-08-13T22:51:37Z

Missed that an interface rename violated TS lint's "imports must be alphabetized" rule.

eladb · 2018-08-14T05:36:12Z

@skinny85 you discovered a bug in jsii (aws/jsii#175). Workaround for now is to modify the signature of grantPipelineBucketReadWriteTo such that identity is required and not optional. Open an issue to track fixing this one the jsii bug is fixed please.

BREAKING CHANGE: 1. Introduce a new aws-codepipeline-api package, and move the Actions API there. 2. Move the service-specific Actions back into their service packages.

skinny85 · 2018-08-14T19:59:56Z

Updated to make the argument to grantPipelineBucketReadWrite required instead of optional. Created an issue to make it optional again in #567 .

eladb · 2018-08-14T20:14:01Z

Hallelujah!

skinny85 · 2018-08-14T20:28:29Z

You said it brother 🙇

…instead of parent. BREAKING CHANGE: this commit changes the way we pass a Pipeline to a newly created Stage. Instead of passing it as a parent, in the first argument of the constructor, we now pass it through a separate props object, in the pipeline property. This is in order to make Stage more consistent with other Constructs, and with the changes made to the Actions API in aws#459.

@rix0rrr

### Features * __@aws-cdk/cdk__: Tokens can now be transparently embedded into strings and encoded into JSON without losing their semantics. This makes it possible to treat late-bound (deploy-time) values as if they were regular strings ([@rix0rrr] in [#518](#518)). * __@aws-cdk/aws-s3__: add support for bucket notifications to Lambda, SNS, and SQS targets ([@eladb] in [#201](#201), [#560](#560), [#561](#561), [#564](#564)) * __@aws-cdk/cdk__: non-alphanumeric characters can now be used as construct identifiers ([@eladb] in [#556](#556)) * __@aws-cdk/aws-iam__: add support for `maxSessionDuration` for Roles ([@eladb] in [#545](#545)). ### Changes * __@aws-cdk/aws-lambda__ (_**BREAKING**_): most classes renamed to be shorter and more in line with official service naming (`Lambda` renamed to `Function` or ommitted) ([@eladb] in [#550](#550)) * __@aws-cdk/aws-codepipeline__ (_**BREAKING**_): move all CodePipeline actions from `@aws-cdk/aws-xxx-codepipeline` packages into the regular `@aws-cdk/aws-xxx` service packages ([@skinny85] in [#459](#459)). * __@aws-cdk/aws-custom-resources__ (_**BREAKING**_): package was removed, and the Custom Resource construct added to the __@aws-cdk/aws-cloudformation__ package ([@rix0rrr] in [#513](#513)) ### Fixes * __@aws-cdk/aws-lambda__: Lambdas that are triggered by CloudWatch Events now show up in the console, and can only be triggered the indicated Event Rule. _**BREAKING**_ for middleware writers (as this introduces an API change), but transparent to regular consumers ([@eladb] in [#558](#558)) * __@aws-cdk/aws-codecommit__: fix a bug where `pollForSourceChanges` could not be set to `false` ([@maciejwalkowiak] in [#534](#534)) * __aws-cdk__: don't fail if the `~/.aws/credentials` file is missing ([@RomainMuller] in [#541](#541)) * __@aws-cdk/aws-cloudformation__: fix a bug in the CodePipeline actions to correctly support TemplateConfiguration ([@mindstorms6] in [#571](#571)). * __@aws-cdk/aws-cloudformation__: fix a bug in the CodePipeline actions to correctly support ParameterOverrides ([@mindstorms6] in [#574](#574)). ### Known Issues * `cdk init` will try to init a `git` repository and fail if no global `user.name` and `user.email` have been configured.

…instead of parent. BREAKING CHANGE: this commit changes the way we pass a Pipeline to a newly created Stage. Instead of passing it as a parent, in the first argument of the constructor, we now pass it through a separate props object, in the pipeline property. This is in order to make Stage more consistent with other Constructs, and with the changes made to the Actions API in aws#459.

…instead of parent. (#568) BREAKING CHANGE: this commit changes the way we pass a Pipeline to a newly created Stage. Instead of passing it as a parent, in the first argument of the constructor, we now pass it through a separate props object, in the pipeline property. This is in order to make Stage more consistent with other Constructs, and with the changes made to the Actions API in #459.

skinny85 mentioned this pull request Jul 31, 2018

Proposal for final CodePipeline Actions scheme: naming and code organization #424

Closed

eladb reviewed Aug 1, 2018

View reviewed changes

skinny85 force-pushed the feature/codepipeline-aws-actions branch from 57515b9 to dd9a394 Compare August 2, 2018 22:45

skinny85 mentioned this pull request Aug 2, 2018

[BREAKING] Align the S3 CodePipeline Action with other Actions #492

Merged

skinny85 force-pushed the feature/codepipeline-aws-actions branch from dd9a394 to 21e1c29 Compare August 3, 2018 00:06

eladb approved these changes Aug 3, 2018

View reviewed changes

skinny85 force-pushed the feature/codepipeline-aws-actions branch from 21e1c29 to d422f5d Compare August 10, 2018 01:07

eladb reviewed Aug 10, 2018

View reviewed changes

skinny85 force-pushed the feature/codepipeline-aws-actions branch from d422f5d to 1dcc371 Compare August 10, 2018 18:24

skinny85 force-pushed the feature/codepipeline-aws-actions branch from 1dcc371 to dfec209 Compare August 10, 2018 18:44

eladb approved these changes Aug 12, 2018

View reviewed changes

skinny85 force-pushed the feature/codepipeline-aws-actions branch from dfec209 to 4682338 Compare August 13, 2018 22:18

skinny85 force-pushed the feature/codepipeline-aws-actions branch 2 times, most recently from f33d12c to 015b2fa Compare August 13, 2018 22:40

refactor(aws-codepipeline): Re-structure the CodePipeline API.

663d970

BREAKING CHANGE: 1. Introduce a new aws-codepipeline-api package, and move the Actions API there. 2. Move the service-specific Actions back into their service packages.

skinny85 force-pushed the feature/codepipeline-aws-actions branch from 015b2fa to 663d970 Compare August 14, 2018 18:55

skinny85 mentioned this pull request Aug 14, 2018

Make IStage#grantPipelineBucketReadWrite's one parameter optional #567

Closed

skinny85 merged commit 389e9bc into aws:master Aug 14, 2018

skinny85 deleted the feature/codepipeline-aws-actions branch August 14, 2018 20:06

skinny85 mentioned this pull request Aug 14, 2018

[BREAKING] Pass the Pipeline to Stage through props instead of parent #568

Merged

skinny85 mentioned this pull request Aug 15, 2018

Add an addToPipeline method for L2 Constructs like CodeCommit Repositories and CodeBuild Projects #265

Closed

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

		@@ -29,25 +41,37 @@ export class Stage extends cdk.Construct {
		*/
		constructor(parent: Pipeline, name: string) {

		@@ -1,24 +0,0 @@
		## AWS CodePipline Actions for AWS CloudFormation

Proposal for final CodePipeline Actions API shape #459

Proposal for final CodePipeline Actions API shape #459

Conversation

skinny85 commented Jul 31, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 commented Aug 2, 2018

skinny85 commented Aug 3, 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

skinny85 commented Aug 3, 2018

skinny85 commented Aug 10, 2018 • edited

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

skinny85 commented Aug 10, 2018

skinny85 commented Aug 10, 2018

skinny85 commented Aug 11, 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

eladb commented Aug 12, 2018

skinny85 commented Aug 13, 2018

skinny85 commented Aug 13, 2018

skinny85 commented Aug 13, 2018

eladb commented Aug 14, 2018

skinny85 commented Aug 14, 2018

eladb commented Aug 14, 2018

skinny85 commented Aug 14, 2018

skinny85 commented Jul 31, 2018 •

edited

skinny85 commented Aug 10, 2018 •

edited