-
Notifications
You must be signed in to change notification settings - Fork 249
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(s3-stepfunctions): removed CloudTrail dependency after new S3 feature #529
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes to the README mean the code needs changing before I review it.
@@ -22,7 +22,7 @@ | |||
|
|||
This AWS Solutions Construct implements an Amazon S3 bucket connected to an AWS Step Function. | |||
|
|||
*Note - This construct uses Amazon EventBridge (Amazon CloudWatch Events) to trigger AWS Step Functions. EventBridge is more flexible, but triggering Step Functions with S3 Event Notifications has less latency and is more cost effective. If cost and/or latency is an issue, you should consider deploy aws-s3-lambda and aws-lambda-stepfunctions in place of this construct.* | |||
*Note - This construct uses Amazon EventBridge (S3 Event Notifications) to trigger AWS Step Functions State Machine executions. EventBridge is more flexible, but triggering executions with S3 Event Notifications has less latency and is more cost effective. If cost is an issue, you should consider deploying aws-s3-lambda and aws-lambda-stepfunctions in place of this construct.* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is confusing, because everything is now directly dependent upon S3 Event Notifications, and we use that term to discuss both solutions. Reading between the lines it sounds like the options are:
- Send S3 Event Notifications to EventBridge (what this now does) - more flexible, more expensive, more latency
- Trigger a Lambda function from S3 Event Notifications, have Lambda function invoke the Step Functions state machine. Less flexible, more work, more cost effective, lower latency.
Let's rewrite this accordingly (and can we confirm that option 2 is still cheaper and faster?)
source/patterns/@aws-solutions-constructs/aws-s3-step-function/README.md
Show resolved
Hide resolved
source/patterns/@aws-solutions-constructs/aws-s3-step-function/README.md
Show resolved
Hide resolved
source/patterns/@aws-solutions-constructs/aws-s3-step-function/README.md
Show resolved
Hide resolved
source/patterns/@aws-solutions-constructs/aws-s3-step-function/README.md
Show resolved
Hide resolved
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple wording changes in the doc, but the functionality looks right and we can go ahead and code.
source/patterns/@aws-solutions-constructs/aws-s3-step-function/README.md
Outdated
Show resolved
Hide resolved
source/patterns/@aws-solutions-constructs/aws-s3-step-function/README.md
Outdated
Show resolved
Hide resolved
source/patterns/@aws-solutions-constructs/aws-s3-step-function/README.md
Show resolved
Hide resolved
source/patterns/@aws-solutions-constructs/aws-s3-step-function/lib/index.ts
Outdated
Show resolved
Hide resolved
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple more wording changes.
source/patterns/@aws-solutions-constructs/aws-s3-step-function/README.md
Outdated
Show resolved
Hide resolved
...ce/patterns/@aws-solutions-constructs/aws-s3-step-function/test/integ.pre-existing-bucket.ts
Show resolved
Hide resolved
@@ -61,18 +44,10 @@ test('override eventRuleProps', () => { | |||
eventRuleProps: { | |||
eventPattern: { | |||
source: ['aws.s3'], | |||
detailType: ['AWS API Call via CloudTrail'], | |||
detailType: ['Object Created'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This defines the single event we're currently worried about, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
source/patterns/@aws-solutions-constructs/aws-s3-stepfunctions/README.md
Outdated
Show resolved
Hide resolved
source/patterns/@aws-solutions-constructs/aws-s3-stepfunctions/lib/index.ts
Outdated
Show resolved
Hide resolved
source/patterns/@aws-solutions-constructs/aws-s3-stepfunctions/lib/index.ts
Show resolved
Hide resolved
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Issue #519 , if available:
Description of changes:
-Removed CloudTrail dependency, uses S3 Event Notifications
-Enable EventBridge in S3 Bucket creation
-Updated integ tests and README
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
fixes #519