feat(aws-s3): add the option to not poll to the CodePipeline Action #1260

skinny85 · 2018-11-29T00:56:57Z

Pull Request Checklist

Please check all boxes (including N/A items)

Testing

Unit test and/or integration test added
Toolkit change?: integration
tests
manually executed (paste output to the PR description)
Init template change?: coordinated update of integration tests
(currently maintained in a private repo).

Documentation

README: README and/or documentation topic updated
jsdocs: All public APIs documented

Title and description

Change type: Title is prefixed with change type:
- fix(module): <title> bug fix (patch)
- feat(module): <title> feature/capability (minor)
- chore(module): <title> won't appear in changelog
- build(module): <title> won't appear in changelog
Title format: Title uses lower case and doesn't end with a period
Breaking change?: Last paragraph of description is: BREAKING CHANGE: <describe exactly what changed and how to achieve similar behavior + link to documentation/gist/issue if more details are required>
References: Indicate issues fixed via: Fixes #xxx or Closes #xxx

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

skinny85 · 2018-11-29T01:46:11Z

Note: please don't merge this yet, there's apparently some CloudTrail integration that's required to make bucket events work (details: https://docs.aws.amazon.com/codepipeline/latest/userguide/create-cloudtrail-S3-source.html ). I'm still working on this.

packages/@aws-cdk/aws-s3/lib/bucket.ts

packages/@aws-cdk/aws-s3/test/test.notifications.ts

skinny85 · 2018-11-30T02:42:57Z

The CloudTrail investigation proved to be quite the rabbit hole. I had to change the solution quite substantially (I've updated the PR description with details). Feedback is welcome!

skinny85 · 2018-11-30T16:22:08Z

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

      },
      ...props,
    });

+    if (props.pollForSourceChanges === false && props.cloudTrail === undefined) {
+      throw new Error('When not polling for source changes, ' +


I'm actually having second thoughts about this validation :/. While the intention was good I think, I'm not sure if it's not too strict. For example, I might be using an imported Bucket that already belongs to some Trail, and in that case I can safely say pollForSourceChanges: false and cloudTrail: undefined.

Thoughts on this?

I like the validation, but there definitely is some interest in skipping for an imported bucket... Maybe you could attach a warning metadata entry to the resource instead of completely failing, and at a later stage, if/when we're able to detect "imported" entities, then you could switch it around and start failing again?

skinny85 · 2018-12-13T22:27:21Z

@RomainMuller I'm actually having second thoughts about this entire change :/. I don't like the whole @aws-cdk/aws-couldtrail-api dance that I have to do here, and I'm not even sure having a cloudTrail property on the S3 Action makes sense.

I'm thinking of leaving the pollForChanges as true by default for this, and just adding the CloudWatch Event if it's set to false, and making sure in the documentation that we explicitly say that your Bucket needs to be added to some Trail in order for the Events (and thus pollForChanges: false) to work. Yes, it's not ideal, but I don't think we can do better here.

I would love to hear your thoughts on the matter...?

eladb · 2018-12-13T22:56:05Z

Is it easy enough to just define a trail via cdk.Resource here and bypass all the layers?

skinny85 · 2018-12-13T23:05:32Z

The problem is that there's a hard limit of 5 Trails per region for each account, so I don't think we want to create one for every Bucket added to the Pipeline :(. Also, what if the Bucket already belongs to an existing Trail (the imported case)? Seems wasteful to create a new one...

eladb · 2018-12-13T23:06:41Z

yeah, seems heavy handed.

skinny85 · 2018-12-21T19:29:45Z

@RomainMuller let me know what you think of my revised proposal (get rid of aws-cloudtrail-api, keep pollForSourceChanges as true by default in the S3 PipelineSourceAction and explain in the docs that your Bucket needs to be a part of a CloudTrail for pollForSourceChanges: false to work).

RomainMuller · 2019-01-16T22:42:01Z

@skinny85 I'm cool with that. It'd be sweet if we could throw in a warning when the bucket isn't CloudTrail'd, but it sounds like prohibitively cumbersome... So yeah.

What happens if the bucket is not CloudTrail'd - does it fail deploying? Or does it deploy, but not work?

skinny85 · 2019-01-16T22:51:59Z

What happens if the bucket is not CloudTrail'd - does it fail deploying? Or does it deploy, but not work?

It deploys, but doesn't work (the CloudWatch Event is never emitted, so your Pipeline will never start a build).

RomainMuller · 2019-01-18T18:43:43Z

That kinda sucks. I think we should track this in an issue somewhere, so we eventually have a way to notify the user... But I don't think I want it to be a blocker for this PR.

skinny85 · 2019-01-24T01:55:54Z

Updated with the agreed changes. @RomainMuller please re-review, thanks.

skinny85 · 2019-01-24T02:01:21Z

Missed an incorrect method name in the CloudTrail ReadMe.

RomainMuller · 2019-01-24T23:30:47Z

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

-
-For example - this logs all ReadWriteEvents for the `magic-bucket` bucket:
+For using CloudTrail event selector to log specific S3 events,
+you can use the `CloudTrailProps` configuration object.


I'm a little confused because I don't see CloudTrailProps anywhere in the code example...

RomainMuller · 2019-01-24T23:34:26Z

packages/@aws-cdk/aws-s3/lib/bucket.ts

+        ],
+        detail: {
+          eventSource: [
+            's3.amazonaws.com',


Is it the same value in partitions other than aws?

skinny85 requested a review from eladb November 29, 2018 00:57

eladb reviewed Nov 29, 2018

View reviewed changes

packages/@aws-cdk/aws-s3/lib/bucket.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-s3/test/test.notifications.ts Show resolved Hide resolved

skinny85 closed this Nov 30, 2018

skinny85 deleted the feature/s3-action-events branch November 30, 2018 01:48

skinny85 restored the feature/s3-action-events branch November 30, 2018 01:48

skinny85 reopened this Nov 30, 2018

skinny85 force-pushed the feature/s3-action-events branch from d9f060e to ec47e4a Compare November 30, 2018 02:33

skinny85 changed the title ~~feat(aws-s3): do not poll by default in the CodePipeline source Action~~ feat(aws-s3): add the option to not poll with CloudTrail to the CodePipeline Action Nov 30, 2018

skinny85 requested review from rix0rrr and RomainMuller November 30, 2018 02:42

skinny85 commented Nov 30, 2018

View reviewed changes

sam-goodwin assigned RomainMuller Jan 16, 2019

sam-goodwin added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jan 16, 2019

RomainMuller approved these changes Jan 16, 2019

View reviewed changes

RomainMuller removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jan 16, 2019

skinny85 mentioned this pull request Jan 21, 2019

codepipeline: pollForSourceChanges defaults to true #1030

Closed

skinny85 force-pushed the feature/s3-action-events branch from ec47e4a to 65e4f68 Compare January 24, 2019 01:54

skinny85 requested a review from a team as a code owner January 24, 2019 01:54

skinny85 changed the title ~~feat(aws-s3): add the option to not poll with CloudTrail to the CodePipeline Action~~ feat(aws-s3): add the option to not poll to the CodePipeline Action Jan 24, 2019

feat(aws-s3): add the option to not poll to the CodePipeline Action.

f47c86c

skinny85 force-pushed the feature/s3-action-events branch from 65e4f68 to f47c86c Compare January 24, 2019 02:00

RomainMuller approved these changes Jan 24, 2019

View reviewed changes

skinny85 merged commit 876b26d into aws:master Jan 25, 2019

skinny85 deleted the feature/s3-action-events branch January 25, 2019 00:00

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(aws-s3): add the option to not poll to the CodePipeline Action #1260

feat(aws-s3): add the option to not poll to the CodePipeline Action #1260

skinny85 commented Nov 29, 2018 •

edited

Loading

skinny85 commented Nov 29, 2018

skinny85 commented Nov 30, 2018

skinny85 Nov 30, 2018

RomainMuller Dec 10, 2018

skinny85 commented Dec 13, 2018

eladb commented Dec 13, 2018

skinny85 commented Dec 13, 2018

eladb commented Dec 13, 2018

skinny85 commented Dec 21, 2018

RomainMuller commented Jan 16, 2019

skinny85 commented Jan 16, 2019

RomainMuller commented Jan 18, 2019

skinny85 commented Jan 24, 2019

skinny85 commented Jan 24, 2019

RomainMuller Jan 24, 2019

RomainMuller Jan 24, 2019

feat(aws-s3): add the option to not poll to the CodePipeline Action #1260

feat(aws-s3): add the option to not poll to the CodePipeline Action #1260

Conversation

skinny85 commented Nov 29, 2018 • edited Loading

Pull Request Checklist

Testing

Documentation

Title and description

skinny85 commented Nov 29, 2018

skinny85 commented Nov 30, 2018

skinny85 Nov 30, 2018

Choose a reason for hiding this comment

RomainMuller Dec 10, 2018

Choose a reason for hiding this comment

skinny85 commented Dec 13, 2018

eladb commented Dec 13, 2018

skinny85 commented Dec 13, 2018

eladb commented Dec 13, 2018

skinny85 commented Dec 21, 2018

RomainMuller commented Jan 16, 2019

skinny85 commented Jan 16, 2019

RomainMuller commented Jan 18, 2019

skinny85 commented Jan 24, 2019

skinny85 commented Jan 24, 2019

RomainMuller Jan 24, 2019

Choose a reason for hiding this comment

RomainMuller Jan 24, 2019

Choose a reason for hiding this comment

skinny85 commented Nov 29, 2018 •

edited

Loading