feat(codepipeline): allow creation of pipelines without source trigger #2332

SanderKnape · 2019-04-19T14:05:16Z

Pull Request Checklist

Testing
- Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
- CLI change?: coordinate update of integration tests with team
- cdk-init template change?: coordinated update of integration tests with team
Docs
- jsdocs: All public APIs documented
- README: README and/or documentation topic updated
Title and Description
- Change type: title prefixed with fix, feat will appear in changelog
- Title: use lower-case and doesn't end with a period
- Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
- Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
Sensitive Modules (requires 2 PR approvers)
- IAM Policy Document (in @aws-cdk/aws-iam)
- EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
- Grant APIs (only if not based on official documentation with a reference)

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

Notes

My first PR here, I went through the CONTRIBUTING file but I certainly may have missed something
I changed the existing github action uses ThirdParty owner test to verify that the default behaviour has remained the same
Since there are now three options possible with the replaced property, I added two more test to include all use cases

BREAKING CHANGE: this changes the GitHub source action pollForSourceChanges property to the trigger property that allows Webhook (default), Poll and None (new)

skinny85

Thanks a lot for the great contribution @SanderKnape! A few small comments (the most important is changing the enum name).

skinny85 · 2019-04-22T21:53:13Z

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

@@ -1,6 +1,12 @@
 import codepipeline = require('@aws-cdk/aws-codepipeline');
 import { SecretValue } from '@aws-cdk/cdk';

+export enum TriggerType {


Can you rename this enum to GitHubTrigger? (there will be more triggers for other sources like CodeCommit, S3, etc.).

Some JSDocs explaining what this is, and what's the default, would be great (especially None is important to explain).

skinny85 · 2019-04-22T21:53:47Z

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

   *
-   * @default false
+   * @default "WebHook"


@default GitHubTrigger.WebHook (it's not a string)

skinny85 · 2019-04-22T21:54:19Z

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

@@ -39,12 +45,11 @@ export interface GitHubSourceActionProps extends codepipeline.CommonActionProps
  readonly oauthToken: SecretValue;

  /**
-   * Whether AWS CodePipeline should poll for source changes.
-   * If this is `false`, the Pipeline will use a webhook to detect source changes instead.


Leave the webhook comment (of course, adjust it to the current situation).

skinny85 · 2019-04-22T21:56:03Z

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

@@ -63,7 +68,7 @@ export class GitHubSourceAction extends codepipeline.SourceAction {
        Repo: props.repo,
        Branch: props.branch || "master",
        OAuthToken: props.oauthToken.toString(),
-        PollForSourceChanges: props.pollForSourceChanges || false,
+        PollForSourceChanges: (props.trigger && props.trigger === TriggerType.Poll) || false,


This entire expression should be simply: props.trigger === GitHubTrigger.Poll.

skinny85 · 2019-04-22T22:03:46Z

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

+        "Name": "Two"
+      }
+      ]
+    }));


When you're using haveResourceLike, you don't need all these extra properties. Focus on just the ones important for your test, something like:

expect(stack).to(haveResourceLike('AWS::CodePipeline::Pipeline', { "Stages": [ { "Actions": [ { "Name": "GH", "Configuration": { "PollForSourceChanges": true }, } ], "Name": "Source" }, { "Actions": [ { "Name": "Boo" } ], "Name": "Two" } ] }));

skinny85 · 2019-04-22T22:04:03Z

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

+        "Name": "Two"
+      }
+      ]
+    }));


Same comment here as above.

skinny85 · 2019-04-22T22:04:42Z

packages/@aws-cdk/cfnspec/tsconfig.tsbuildinfo

@@ -1,431 +1,431 @@
 {
  "program": {
    "fileInfos": {
-      "/users/benisrae/code/cdk/aws-cdk-4/tools/cdk-build-tools/node_modules/typescript/lib/lib.es5.d.ts": {


This was committed accidentally, and should not be included in the PR.

SanderKnape · 2019-04-23T12:02:46Z

Thanks for the feedback @skinny85. I have implemented your requested changes - let me know if there is anything else I need to change!

skinny85

Thanks so much for the contribution @SanderKnape !

… trigger (aws#2332) BREAKING CHANGE: the `pollForSourceChanges` property in `GitHubSourceAction` has been renamed to `trigger`, and its type changed from a `boolean` to an enum.

SanderKnape requested review from RomainMuller, skinny85 and a team as code owners April 19, 2019 14:05

SanderKnape force-pushed the sanderknape/codepipeline-without-trigger branch 2 times, most recently from b967299 to b086c59 Compare April 19, 2019 15:25

skinny85 suggested changes Apr 22, 2019

View reviewed changes

feat(codepipeline): allow creation of pipelines without source trigger

7f60617

SanderKnape force-pushed the sanderknape/codepipeline-without-trigger branch from b086c59 to 1d8310f Compare April 23, 2019 11:24

Implement requested changes

7b08676

SanderKnape force-pushed the sanderknape/codepipeline-without-trigger branch from 1d8310f to 7b08676 Compare April 23, 2019 11:30

RomainMuller assigned skinny85 Apr 24, 2019

skinny85 approved these changes Apr 30, 2019

View reviewed changes

skinny85 merged commit ed39a8c into aws:master Apr 30, 2019

jamesiri unassigned skinny85 Jul 11, 2019

This was referenced Dec 12, 2019

[Snyk] Upgrade @aws-cdk/cdk from 0.22.0 to 0.35.0 MechanicalRock/scdk#3

Closed

[Snyk] Upgrade @aws-cdk/cdk from 0.24.1 to 0.35.0 MechanicalRock/cdk-constructs#8

Open

[Snyk] Upgrade @aws-cdk/cdk from 0.24.1 to 0.35.0 MechanicalRock/account-reaper#6

Open

sumodgeorge mentioned this pull request Nov 28, 2023

[Snyk] Security upgrade verdaccio from 3.8.1 to 5.1.4 sumodgeorge/aws-cdk#17

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(codepipeline): allow creation of pipelines without source trigger #2332

feat(codepipeline): allow creation of pipelines without source trigger #2332

SanderKnape commented Apr 19, 2019

skinny85 left a comment

skinny85 Apr 22, 2019

skinny85 Apr 22, 2019

skinny85 Apr 22, 2019

skinny85 Apr 22, 2019

skinny85 Apr 22, 2019

skinny85 Apr 22, 2019

skinny85 Apr 22, 2019

SanderKnape commented Apr 23, 2019

skinny85 left a comment

feat(codepipeline): allow creation of pipelines without source trigger #2332

feat(codepipeline): allow creation of pipelines without source trigger #2332

Conversation

SanderKnape commented Apr 19, 2019

Pull Request Checklist

skinny85 left a comment

Choose a reason for hiding this comment

skinny85 Apr 22, 2019

Choose a reason for hiding this comment

skinny85 Apr 22, 2019

Choose a reason for hiding this comment

skinny85 Apr 22, 2019

Choose a reason for hiding this comment

skinny85 Apr 22, 2019

Choose a reason for hiding this comment

skinny85 Apr 22, 2019

Choose a reason for hiding this comment

skinny85 Apr 22, 2019

Choose a reason for hiding this comment

skinny85 Apr 22, 2019

Choose a reason for hiding this comment

SanderKnape commented Apr 23, 2019

skinny85 left a comment

Choose a reason for hiding this comment