feat(codebuild): add filter groups feature for GitHub, GitHubEnterpri… #2319

Kaixiang-AWS · 2019-04-18T05:11:08Z

…se and BitBucket

Fixes #1842

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.

skinny85

A few questions for me to better understand how this functionality works.

skinny85 · 2019-04-22T22:23:34Z

packages/@aws-cdk/aws-codebuild/lib/source.ts

+  /**
+   * A list of lists of WebhookFilter objects used to determine which webhook events are triggered.
+   */
+  readonly filterGroups?: WebhookFilter[][];


Why a list of lists? What are the semantics here?

It's an OR operation between different lists and an AND operation inside the list.
For example, if I have filter groups [[A,B],[C,D]], build will only be triggered if (A&B)|(C&D)

skinny85 · 2019-04-22T22:23:45Z

packages/@aws-cdk/aws-codebuild/test/test.codebuild.ts

+          ],
+        })
+      });
+    }, Error, "filterGroups property could only be set when webhook property is true");


I would actually change this behavior. I think if filterGroups was specified, and webhook was not, I think webhook should default to true. Only specifying filterGroups and webhook: false should result in an exception.

What do you think?

I'm thinking it in a different way. If customers want to remove filter groups but keep webhook enabled, they don't need to add webhook: true which is a cognitive load for customers.

Right. So when a dilemma like this happens, I like to compare the number of customers in each case. Here, you have every single person who wants to use filter groups, vs. a group that first uses filter groups, and then stops using them. It's pretty clear that the first group is larger (as the second group is a subset of the first one), so I think it makes more sense to optimize the experience for the larger group.

I think we should follow the principle of least astonishment here. Customers won't be surprised to see an error if they specify filterGroups but don't specify webhook. However, they will feel surprised and confused when removing filterGroups causes their webhook being deleted. Could you please give me an example of somewhere else doing this kind of override behavior to help me better understand this case? I couldn't think of any at this moment.

Could you please give me an example of somewhere else doing this kind of override behavior to help me better understand this case? I couldn't think of any at this moment.

Sure. As an example, in CodeDeploy's ServerDeploymentGroup, we change the value of the rollback alarm configuration depending on whether the customer has provided any alarms (see here and here).

skinny85 · 2019-04-22T22:27:58Z

packages/@aws-cdk/aws-codebuild/lib/source.ts

+ * Filter types for webhook filters
+ */
+export enum WebhookFilterType {
+  Event = 'EVENT',


What does Event exactly mean? Are there some limits to what can pattern be when type is Event? If so, I think we might be missing a modeling opportunity here...

Different from other types, pattern for Event could only be one or more actions concatenated with ,. I couldn't think of a good model to model this behavior. Please let me know if you have some idea in mind.

Refs:
filterGroups API ref
filterGroups user guide

skinny85 · 2019-05-01T01:11:17Z

Hey @Kaixiang-AWS ,

I've uploaded my proposal on how I see this API. The usage looks like as follows:

new codebuild.Project(stack, 'Project', {
  source: new codebuild.BitBucketSource({
    // ...
    filterGroups: [
      codebuild.FilterGroup.inEventOf(codebuild.EventAction.PUSH).andBranchIs('master'),
      codebuild.FilterGroup.inEventOf(codebuild.EventAction.PULL_REQUEST_CREATED).andBaseBranchIs('master'),
    ],
  })
});

I'm curious what you think of doing it this way. Let me know!

Thanks,
Adam

skinny85 · 2019-05-02T00:23:21Z

Updated with the proposed final version, let me know what you think @Kaixiang-AWS !

…se and BitBucket Fixes aws#1842

skinny85 · 2019-05-02T18:16:51Z

Rebased on top of 0.30.0.

Kaixiang-AWS · 2019-05-02T18:25:53Z

LGTM.

Fixes aws#1842

Kaixiang-AWS requested review from RomainMuller, skinny85 and a team as code owners April 18, 2019 05:11

skinny85 reviewed Apr 22, 2019

View reviewed changes

skinny85 force-pushed the filter-groups branch from dd903ea to 0af5b2b Compare May 1, 2019 01:08

skinny85 force-pushed the filter-groups branch 2 times, most recently from a729a89 to 5a67cb0 Compare May 2, 2019 00:22

Kaixiang-AWS and others added 2 commits May 2, 2019 11:15

feat(codebuild): add filter groups feature for GitHub, GitHubEnterpri…

0039150

…se and BitBucket Fixes aws#1842

My proposal.

9ebe526

skinny85 force-pushed the filter-groups branch from 5a67cb0 to 9ebe526 Compare May 2, 2019 18:16

skinny85 approved these changes May 2, 2019

View reviewed changes

skinny85 merged commit fd74d07 into aws:master May 2, 2019

Kaixiang-AWS deleted the filter-groups branch May 14, 2019 04:14

SanderKnape pushed a commit to SanderKnape/aws-cdk that referenced this pull request May 14, 2019

feat(codebuild): add webhook Filter Groups. (aws#2319)

97a329d

Fixes aws#1842

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(codebuild): add filter groups feature for GitHub, GitHubEnterpri… #2319

feat(codebuild): add filter groups feature for GitHub, GitHubEnterpri… #2319

Kaixiang-AWS commented Apr 18, 2019 •

edited

Loading

skinny85 left a comment

skinny85 Apr 22, 2019

Kaixiang-AWS Apr 22, 2019

skinny85 Apr 22, 2019

Kaixiang-AWS Apr 22, 2019

skinny85 Apr 22, 2019

Kaixiang-AWS Apr 23, 2019

skinny85 Apr 30, 2019

skinny85 Apr 22, 2019

Kaixiang-AWS Apr 22, 2019

skinny85 commented May 1, 2019

skinny85 commented May 2, 2019

skinny85 commented May 2, 2019

Kaixiang-AWS commented May 2, 2019

feat(codebuild): add filter groups feature for GitHub, GitHubEnterpri… #2319

feat(codebuild): add filter groups feature for GitHub, GitHubEnterpri… #2319

Conversation

Kaixiang-AWS commented Apr 18, 2019 • edited Loading

Pull Request Checklist

skinny85 left a 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 May 1, 2019

skinny85 commented May 2, 2019

skinny85 commented May 2, 2019

Kaixiang-AWS commented May 2, 2019

Kaixiang-AWS commented Apr 18, 2019 •

edited

Loading