Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

SanderKnape
Copy link

Fixes #1652


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)

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

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

export enum TriggerType {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

*
* @default false
* @default "WebHook"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"Name": "Two"
}
]
}));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"
        }
      ]
    }));

"Name": "Two"
}
]
}));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment here as above.

@@ -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": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for the contribution @SanderKnape !

@skinny85 skinny85 merged commit ed39a8c into aws:master Apr 30, 2019
SanderKnape pushed a commit to SanderKnape/aws-cdk that referenced this pull request May 14, 2019
… 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

codepipeline: GitHubSourceAction should allow setup without polling or webhook
2 participants