feat(aws-codecommit): use CloudWatch Events instead of polling by default in the CodePipeline Action by skinny85 · Pull Request #1026 · aws/aws-cdk

skinny85 · 2018-10-27T00:23:29Z

This is the preferred way now by the CodePipeline team, so we should make it the default.

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

eladb · 2018-10-28T08:04:56Z

packages/@aws-cdk/aws-codecommit/lib/pipeline-action.ts

      outputArtifactName: props.outputArtifactName
    });

+    if (props.pollForSourceChanges !== true) {


Ideally you wouldn't want to codify the default multiple times in your code.

Since the default is false, I would just write this if (!props.pollForSourceChanges).
Alternatively, if you want to be more formal, create a local variable:

const pollForSourceChanges = props.pollForSourceChanges || false;

And use it everywhere.

We can't use a variable here, as the call to super must be the first statement in the constructor.

Will change to if (!props.pollForSourceChanges).

That’s not correct. You can definitely use a variable :-)

Can you write out the code for that? Cause I can't see it.

eladb · 2018-10-28T08:05:42Z

packages/@aws-cdk/aws-codecommit/lib/pipeline-action.ts

    });

+    if (props.pollForSourceChanges !== true) {
+      props.repository.onCommit('CodePipeline', props.stage.pipelineAsEventTarget, props.branch || 'master');


What happens if we want multiple pipelines to subscribe to the same repo? I think your ID should include the pipeline's .uniqueId or something.

Good call. It's actually a miss on my part, as I meant to change this name to something else, but forgot. Will add the Pipeline's uniqueId.

eladb · 2018-10-28T08:06:35Z

packages/@aws-cdk/aws-codecommit/lib/pipeline-action.ts

    });

+    if (props.pollForSourceChanges !== true) {
+      props.repository.onCommit('CodePipeline', props.stage.pipelineAsEventTarget, props.branch || 'master');


Again, codify the default for branch once in your code:

const branch = props.branch || 'master';

Again, can't use a variable here (super call, yada yada yada). The only alternative is a function, and that seemed like overkill to me here, hence the duplication.

Yada yada you can ;-)

eladb · 2018-10-28T08:07:29Z

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

    return this.pipeline.role;
  }

+  public get pipelineAsEventTarget(): events.IEventRuleTarget {


Why is this needed? The caller can just do stage.pipeline - it will also be much more readable at the call site.

A caller cannot do stage.pipeline, because you cannot reference the codepipeline module from the codepipeline-api one (where the IStage interface lives).

eladb · 2018-10-28T08:07:40Z

packages/@aws-cdk/aws-codecommit/lib/pipeline-action.ts

    });

+    if (props.pollForSourceChanges !== true) {
+      props.repository.onCommit('CodePipeline', props.stage.pipelineAsEventTarget, props.branch || 'master');


Why not just stage.pipeline?

See above - it would cause a circular dependency. The entire reason for breaking out the codepipeline-api module is because Actions cannot reference the codepipeline module, where the Pipeline type lives.

eladb · 2018-10-28T08:08:30Z

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

+    pollForSourceChanges: true,
  });

  /** Build! */


Missing unit test for the default and setting to false

skinny85 · 2018-10-29T21:04:22Z

Updated with @eladb's feebdack.

eladb · 2018-10-30T05:22:16Z

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

    throw new Error('Unsupported');
  }

+  public get pipelineUniqueId(): string {


I am wondering if perhaps it makes more sense to add cpapi.IPipeline and have Pipeline implement it. This is becoming quite messy, no?

Agreed. Refactored in the newest revision.

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

eladb · 2018-10-31T03:51:45Z

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

  }

+  public grantBucketRead(identity?: iam.IPrincipal): void {
+    this.artifactBucket.grantRead(identity);


dependency-wise, would it make sense to just expose artifactBucket in IPipeline?

Do you think I would bother with these methods, if I could just use artifactBucket here? 🙂

You can't expose artifactBucket in IPipeline, as that would be a circular dependency between the aws-codepipeline-api and aws-s3 modules.

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

skinny85 · 2018-10-31T18:22:25Z

Latest round of changes after @eladb's comments.

…ault in the CodePipeline Action. BREAKING CHANGE: this modifies the default behavior of the CodeCommit Action. It also changes the internal API contract between the aws-codepipeline-api module and the CodePipeline Actions in the service packages.

skinny85 · 2018-11-02T17:41:52Z

Noticed that there were actually existing unit tests for the CodeCommit's pollForSourceChanges functionality in test.pipeline.ts, so re-used those ones and removed the redundant ones in test.actions.ts.

Bug Fixes ========= * **aws-autoscaling:** allow minSize to be set to 0 ([#1015](#1015)) ([67f7fa1](67f7fa1)) * **aws-codebuild:** correctly pass the timeout property to CFN when creating a Project. ([#1071](#1071)) ([b1322bb](b1322bb)) * **aws-codebuild:** correctly set S3 path when using it as artifact. ([#1072](#1072)) ([f32cba9](f32cba9)) * **aws-kms:** add output value when exporting an encryption key ([#1036](#1036)) ([cb490be](cb490be)) * Switch from `js-yaml` to `yaml` ([#1092](#1092)) ([0b132b5](0b132b5)) Features ========= * **applets:** integrate into toolkit ([#1039](#1039)) ([fdabe95](fdabe95)), closes [#849](#849) [#342](#342) [#291](#291) * **aws-codecommit:** use CloudWatch Events instead of polling by default in the CodePipeline Action. ([#1026](#1026)) ([d09d30c](d09d30c)) * **aws-dynamodb:** allow specifying partition/sort keys in props ([#1054](#1054)) ([ec87331](ec87331)), closes [#1051](#1051) * **aws-ec2:** AmazonLinuxImage supports AL2 ([#1081](#1081)) ([97b57a5](97b57a5)), closes [#1062](#1062) * **aws-lambda:** high level API for event sources ([#1063](#1063)) ([1be3442](1be3442)) * **aws-sqs:** improvements to IAM grants API ([#1052](#1052)) ([6f2475e](6f2475e)) * don't upload the same asset multiple times ([#1011](#1011)) ([35937b6](35937b6)), closes [#989](#989) * **codepipeline/cfn:** Use fewer statements for pipeline permissions ([#1009](#1009)) ([8f4c2ab](8f4c2ab)) * add a new construct library for ECS ([#1058](#1058)) ([ae03ddb](ae03ddb)) * **pkglint:** Make sure .snk files are ignored ([#1049](#1049)) ([53c8d76](53c8d76)), closes [#643](#643) * **toolkit:** deployment ui improvements ([#1067](#1067)) ([c832eaf](c832eaf)) BREAKING CHANGES ========= * The ec2.Connections object has been changed to be able to manage multiple security groups. The relevant property has been changed from `securityGroup` to `securityGroups` (an array of security group objects). * **aws-codecommit:** This modifies the default behavior of the CodeCommit Action. It also changes the internal API contract between the aws-codepipeline-api module and the CodePipeline Actions in the service packages. * **applets:** The applet schema has changed to allow Multiple applets can be define in one file by structuring the files like this: * **applets:** The applet schema has changed to allow definition of multiple applets in the same file. The schema now looks like this: applets: MyApplet: type: ./my-applet-file properties: property1: value ... By starting an applet specifier with npm://, applet modules can directly be referenced in NPM. You can include a version specifier (@1.2.3) to reference specific versions. * **aws-sqs:** `queue.grantReceiveMessages` has been removed. It is unlikely that this would be sufficient to interact with a queue. Alternatively you can use `queue.grantConsumeMessages` or `queue.grant('sqs:ReceiveMessage')` if there's a need to only grant this action.

Bug Fixes ======== * **aws-autoscaling:** allow minSize to be set to 0 ([#1015](#1015)) ([67f7fa1](67f7fa1)) * **aws-codebuild:** correctly pass the timeout property to CFN when creating a Project. ([#1071](#1071)) ([b1322bb](b1322bb)) * **aws-codebuild:** correctly set S3 path when using it as artifact. ([#1072](#1072)) ([f32cba9](f32cba9)) * **aws-kms:** add output value when exporting an encryption key ([#1036](#1036)) ([cb490be](cb490be)) * Switch from `js-yaml` to `yaml` ([#1092](#1092)) ([0b132b5](0b132b5)) Features ======== * don't upload the same asset multiple times ([#1011](#1011)) ([35937b6](35937b6)), closes [#989](#989) * **app-delivery:** CI/CD for CDK Stacks ([#1022](#1022)) ([f2fe4e9](f2fe4e9)) * add a new construct library for ECS ([#1058](#1058)) ([ae03ddb](ae03ddb)) * **applets:** integrate into toolkit ([#1039](#1039)) ([fdabe95](fdabe95)), closes [#849](#849) [#342](#342) [#291](#291) * **aws-codecommit:** use CloudWatch Events instead of polling by default in the CodePipeline Action. ([#1026](#1026)) ([d09d30c](d09d30c)) * **aws-dynamodb:** allow specifying partition/sort keys in props ([#1054](#1054)) ([ec87331](ec87331)), closes [#1051](#1051) * **aws-ec2:** AmazonLinuxImage supports AL2 ([#1081](#1081)) ([97b57a5](97b57a5)), closes [#1062](#1062) * **aws-lambda:** high level API for event sources ([#1063](#1063)) ([1be3442](1be3442)) * **aws-sqs:** improvements to IAM grants API ([#1052](#1052)) ([6f2475e](6f2475e)) * **codepipeline/cfn:** Use fewer statements for pipeline permissions ([#1009](#1009)) ([8f4c2ab](8f4c2ab)) * **pkglint:** Make sure .snk files are ignored ([#1049](#1049)) ([53c8d76](53c8d76)), closes [#643](#643) * **toolkit:** deployment ui improvements ([#1067](#1067)) ([c832eaf](c832eaf)) * Update to CloudFormation resource specification v2.11.0 BREAKING CHANGES ======== * The ec2.Connections object has been changed to be able to manage multiple security groups. The relevant property has been changed from `securityGroup` to `securityGroups` (an array of security group objects). * **aws-codecommit:** this modifies the default behavior of the CodeCommit Action. It also changes the internal API contract between the aws-codepipeline-api module and the CodePipeline Actions in the service packages. * **applets:** The applet schema has changed to allow Multiple applets can be define in one file by structuring the files like this: * **applets:** The applet schema has changed to allow definition of multiple applets in the same file. The schema now looks like this: applets: MyApplet: type: ./my-applet-file properties: property1: value ... By starting an applet specifier with npm://, applet modules can directly be referenced in NPM. You can include a version specifier (@1.2.3) to reference specific versions. * **aws-sqs:** `queue.grantReceiveMessages` has been removed. It is unlikely that this would be sufficient to interact with a queue. Alternatively you can use `queue.grantConsumeMessages` or `queue.grant('sqs:ReceiveMessage')` if there's a need to only grant this action.

skinny85 requested a review from eladb October 27, 2018 00:23

eladb suggested changes Oct 28, 2018

View reviewed changes

eladb mentioned this pull request Oct 29, 2018

codepipeline: pollForSourceChanges defaults to true #1030

Closed

skinny85 force-pushed the feature/codecommit-actions-events branch from f207fe7 to 9232591 Compare October 29, 2018 21:03

eladb approved these changes Oct 30, 2018

View reviewed changes

skinny85 force-pushed the feature/codecommit-actions-events branch from 9232591 to 9ac276b Compare October 30, 2018 21:21

eladb reviewed Oct 31, 2018

View reviewed changes

skinny85 force-pushed the feature/codecommit-actions-events branch from 9ac276b to 8ef8cd2 Compare October 31, 2018 18:21

eladb approved these changes Nov 1, 2018

View reviewed changes

skinny85 force-pushed the feature/codecommit-actions-events branch from 8ef8cd2 to 0185081 Compare November 2, 2018 17:40

skinny85 merged commit d09d30c into aws:master Nov 2, 2018

skinny85 deleted the feature/codecommit-actions-events branch November 2, 2018 18:23

rix0rrr mentioned this pull request Nov 6, 2018

v0.15.0 #1094

Merged

skinny85 mentioned this pull request Nov 7, 2018

Make IStage#grantPipelineBucketReadWrite's one parameter optional #567

Closed

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

Conversation

skinny85 commented Oct 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skinny85 commented Oct 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

skinny85 commented Oct 31, 2018

Uh oh!

skinny85 commented Nov 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants