Added two new construct patterns: aws-events-rule-kinesisstream and aws-events-rule-kinesisfirehose-s3 #92

aijunpeng · 2020-10-21T15:57:08Z

Issue #, if available:
#59: aws-events-rule-kinesisstream
#72: aws-events-rule-kinesisfirehose-s3

Description of changes:

Added a new construct pattern aws-events-rule-kinesisstream that implements an Amazon CloudWatch Events rule to send data to an Amazon Kinesis Data Stream.
Added a new construct pattern aws-events-rule-kinesisfirehose-s3 that implements an Amazon CloudWatch Events rule to send data to an Amazon Kinesis Data Firehose delivery stream connected to an Amazon S3 bucket.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

…inesisstream-no-arguments.ts

…dated aws-events-rule-kinesisstream

aws-solutions-constructs-team · 2020-10-21T16:29:42Z

AWS CodeBuild CI Report

CodeBuild project: codebuildgithubautobuildPro-rp7jMDIK1wBN
Commit ID: ed925a7
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

hnishar

Overall looks good, few minor comments, the main one is missing recommended CW Alarms for Kinesisstream pattern, check any other aws-kinesisstreams-* pattern for reference.

hnishar · 2020-10-21T21:19:57Z

source/patterns/@aws-solutions-constructs/aws-events-rule-kinesisfirehose-s3/README.md

+Here is a minimal deployable pattern definition in Typescript:
+
+``` javascript
+const { EventsRuleToKinesisFirehoseToS3, EventsRuleToKinesisFirehoseToS3Props } from '@aws-solutions-constructs/aws-events-rule-kinesisfirehose-s3';


Change const to import

hnishar · 2020-10-21T21:20:44Z

source/patterns/@aws-solutions-constructs/aws-events-rule-kinesisfirehose-s3/README.md

+
+const props: EventsRuleToKinesisFirehoseToS3Props = {
+    eventRuleProps: {
+    description: 'event rule props',


Is description necessary ? It might be nice to have, but this example is for a minimal deployable pattern

removed description

hnishar · 2020-10-21T23:04:25Z

source/patterns/@aws-solutions-constructs/aws-events-rule-kinesisfirehose-s3/README.md

+|eventsRole|[`iam.Role`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-iam.Role.html)|Returns an instance of the iam.Role created by the construct for Events Rule|
+|kinesisFirehoseRole|[`iam.Role`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-iam.Role.html)|Returns an instance of the iam.Role created by the construct for Kinesis Data Firehose delivery stream|
+|kinesisFirehoseLogGroup|[`logs.LogGroup`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-logs.LogGroup.html)|Returns an instance of the LogGroup created by the construct for Kinesis Data Firehose delivery stream|
+|firehoseToS3|[`aws_solutions_constructs.aws-kinesisfirehose-s3`](https://docs.aws.amazon.com/solutions/latest/constructs/aws-kinesisfirehose-s3.html)|Returns an instance of aws-kinesisfirehose-s3 construct created by the construct|


Don't think we should expose the underlying solutions construct, as long all the resulting CDK objects are available as properties, IMO this exposes the unnecessary complexity to the user, especially the whole point of these patterns is to abstract away these complexities from the user.

The reason I exposed firehoseToS3 instance as a property of the construct is because that I want to make it easier to access properties of firehoseToS3 that aren’t exposed in the firehoseToS3 construct without having to changing the firehoseToS3 construct itself. Since firehoseToS3 is the base construct rather than the top construct, it won’t affect user experience. But I got your point and updated the code per request.

hnishar · 2020-10-21T23:05:01Z

source/patterns/@aws-solutions-constructs/aws-events-rule-kinesisfirehose-s3/lib/index.ts

+  public readonly kinesisFirehoseRole: iam.Role;
+  public readonly s3Bucket?: s3.Bucket;
+  public readonly s3LoggingBucket?: s3.Bucket;
+  public readonly firehoseToS3: KinesisFirehoseToS3;


See the comment above. Recommend removing it.

hnishar · 2020-10-22T01:51:09Z

source/patterns/@aws-solutions-constructs/aws-events-rule-kinesisfirehose-s3/lib/index.ts

+      })
+    };
+
+    // Add the kinese event source mapping


Fix the comment, probably "Set up the events rule props"

hnishar · 2020-10-22T02:33:14Z

source/patterns/@aws-solutions-constructs/aws-events-rule-kinesisstream/package.json

+      },
+      "dotnet": {
+        "namespace": "Amazon.Constructs.AWS.eventsrulekinesisstream",
+        "packageId": "Amazon.Constructs.AWS.eventsrulekinesisstream",


Use pascal case EventsRuleKinesisStream

hnishar · 2020-10-22T02:34:07Z

source/patterns/@aws-solutions-constructs/aws-events-rule-kinesisstream/package.json

+        "iconUrl": "https://raw.githubusercontent.com/aws/aws-cdk/master/logo/default-256-dark.png"
+      },
+      "python": {
+        "distName": "aws-solutions-constructs.aws-events-rule-kinesisstream",


Use kinesis-stream instead of kinesisstream

hnishar · 2020-10-22T02:34:25Z

source/patterns/@aws-solutions-constructs/aws-events-rule-kinesisstream/package.json

+      },
+      "python": {
+        "distName": "aws-solutions-constructs.aws-events-rule-kinesisstream",
+        "module": "aws_solutions_constructs.aws_events_rule_kinesisstream"


Use kinesis_stream instead of kinesisstream

hnishar · 2020-10-22T02:43:56Z

source/patterns/@aws-solutions-constructs/aws-events-rule-kinesisstream/README.md

+    }
+};
+
+const constructStack = new EventsRuleToKinesisStream(this, 'test-events-rule-kinesis-stream', props);


Remove const constructStack =

hnishar · 2020-10-22T02:52:39Z

...onstructs/aws-events-rule-kinesisstream/test/integ.events-rule-kinesisstream-no-arguments.ts

+
+new EventsRuleToKinesisStream(stack, 'test-events-rule-kinesis-stream', props);
+
+app.synth();


Recommend adding an integ test for existing stream i.e. existingStreamObj

aijunpeng · 2020-10-22T22:34:37Z

Addressed all the items that were raised in the code review and pushed the new code changes. Please take a look at the new changes. Thanks!

aws-solutions-constructs-team · 2020-10-22T22:39:07Z

AWS CodeBuild CI Report

CodeBuild project: codebuildgithubautobuildPro-rp7jMDIK1wBN
Commit ID: 714ba6b
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-solutions-constructs-team · 2020-10-22T23:11:15Z

AWS CodeBuild CI Report

CodeBuild project: codebuildgithubautobuildPro-rp7jMDIK1wBN
Commit ID: b2472a1
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

hnishar

Few minor updates requested and a name change for the kinsesisstream pattern, hope that's not an issue.

hnishar · 2020-10-26T13:50:28Z

source/patterns/@aws-solutions-constructs/aws-events-rule-kinesisfirehose-s3/README.md

+const props: EventsRuleToKinesisFirehoseToS3Props = {
+    eventRuleProps: {
+    schedule: events.Schedule.rate(cdk.Duration.minutes(5))
+};


Missing closing bracket }

const props: EventsRuleToKinesisFirehoseToS3Props = { eventRuleProps: { schedule: events.Schedule.rate(cdk.Duration.minutes(5)) } };

hnishar · 2020-10-26T13:51:33Z

source/patterns/@aws-solutions-constructs/aws-events-rule-kinesisfirehose-s3/README.md

+
+``` javascript
+import { EventsRuleToKinesisFirehoseToS3, EventsRuleToKinesisFirehoseToS3Props } from '@aws-solutions-constructs/aws-events-rule-kinesisfirehose-s3';
+


Missing import statement for cdk.Duration, please add import * as cdk from '@aws-cdk/core';

hnishar · 2020-10-26T13:53:34Z

source/patterns/@aws-solutions-constructs/aws-events-rule-kinesisfirehose-s3/README.md

+``` javascript
+import { EventsRuleToKinesisFirehoseToS3, EventsRuleToKinesisFirehoseToS3Props } from '@aws-solutions-constructs/aws-events-rule-kinesisfirehose-s3';
+
+const props: EventsRuleToKinesisFirehoseToS3Props = {


When this code is copied into the cdk init app the props variable collides with the existing one, please change the props variable name to eventsRuleToKinesisFirehoseToS3Props here and on line 37

hnishar · 2020-10-26T14:02:34Z

source/patterns/@aws-solutions-constructs/aws-events-rule-kinesisstream/README.md

+
+``` typescript
+import {EventsRuleToKinesisStream, EventsRuleToKinesisStreamProps} from "@aws-solutions-constructs/aws-events-rule-kinesisstream";
+


Missing import statement for cdk.Duration, please add import * as cdk from '@aws-cdk/core';

hnishar · 2020-10-26T14:20:39Z

source/patterns/@aws-solutions-constructs/aws-events-rule-kinesisstream/README.md

+``` typescript
+import {EventsRuleToKinesisStream, EventsRuleToKinesisStreamProps} from "@aws-solutions-constructs/aws-events-rule-kinesisstream";
+
+const props: EventsRuleToKinesisStreamProps = {


When this code is copied into the cdk init app the props variable collides with the existing one, please change the props variable name to eventsRulesKinesisstreamProps here and on line 37

hnishar · 2020-10-26T14:28:04Z

source/patterns/@aws-solutions-constructs/aws-events-rule-kinesisstream/package.json

@@ -0,0 +1,83 @@
+{
+  "name": "@aws-solutions-constructs/aws-events-rule-kinesisstream",


Suggest name change of the pattern from aws-events-rule-kinesisstream to aws-events-rule-kinesisstreams to be consistent with other aws-kinesisstreams-* patterns in the library, it will entain changing the directory names as well, sorry should have caught it the first time..

…nts-rule-kinesisstream to aws-events-rule-kinesisstreams

aijunpeng · 2020-10-26T20:56:58Z

Pushed code changes per PR feedback.

aws-solutions-constructs-team · 2020-10-26T21:19:08Z

AWS CodeBuild CI Report

CodeBuild project: codebuildgithubautobuildPro-rp7jMDIK1wBN
Commit ID: e544a87
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-solutions-constructs-team · 2020-10-26T22:23:11Z

AWS CodeBuild CI Report

CodeBuild project: codebuildgithubautobuildPro-rp7jMDIK1wBN
Commit ID: d374ed1
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

… and integration template

aws-solutions-constructs-team · 2020-10-26T23:26:25Z

AWS CodeBuild CI Report

CodeBuild project: codebuildgithubautobuildPro-rp7jMDIK1wBN
Commit ID: 4d30b8c
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

hnishar · 2020-10-26T23:57:28Z

Thanks for the contribution, these will be published in the next release

aijunpeng and others added 7 commits October 17, 2020 00:40

Added new pattern aws-events-rule-kinesisstream

fe2cdbf

Added file events-rule-kinesisstream.test.ts

e854aa6

Added file test/integ.events-rule-kinesistream-no-arguments.ts

55fce09

Fixed header validation error and added file test/integ.events-rule-k…

902c6d2

…inesisstream-no-arguments.ts

Added new construct pattern aws-events-rule-kinesisfirehose-s3 and up…

5bf05cc

…dated aws-events-rule-kinesisstream

Cleaned up .gitignore

39381d1

Reverted change to change log

ed925a7

aijunpeng requested a review from hnishar as a code owner October 21, 2020 15:57

hnishar suggested changes Oct 22, 2020

View reviewed changes

Updates per code review feedback

714ba6b

removed references to firehoseToS3

b2472a1

hnishar suggested changes Oct 26, 2020

View reviewed changes

Updated aws-events-rule-kinesisfirehose-s3 readme and renamed aws-eve…

e544a87

…nts-rule-kinesisstream to aws-events-rule-kinesisstreams

Merge branch 'master' into master

d374ed1

Updated version to 1.69.0 and events-rule-kinesisfirehose-s3 snapshot…

4d30b8c

… and integration template

hnishar merged commit ddc0f03 into awslabs:master Oct 26, 2020


		new EventsRuleToKinesisStream(stack, 'test-events-rule-kinesis-stream', props);

		app.synth();


		``` javascript
		import { EventsRuleToKinesisFirehoseToS3, EventsRuleToKinesisFirehoseToS3Props } from '@aws-solutions-constructs/aws-events-rule-kinesisfirehose-s3';


		``` typescript
		import {EventsRuleToKinesisStream, EventsRuleToKinesisStreamProps} from "@aws-solutions-constructs/aws-events-rule-kinesisstream";

		@@ -0,0 +1,83 @@
		{
		"name": "@aws-solutions-constructs/aws-events-rule-kinesisstream",

Added two new construct patterns: aws-events-rule-kinesisstream and aws-events-rule-kinesisfirehose-s3 #92

Added two new construct patterns: aws-events-rule-kinesisstream and aws-events-rule-kinesisfirehose-s3 #92

Conversation

aijunpeng commented Oct 21, 2020

aws-solutions-constructs-team commented Oct 21, 2020

AWS CodeBuild CI Report

hnishar 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

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

Choose a reason for hiding this comment

aijunpeng commented Oct 22, 2020

aws-solutions-constructs-team commented Oct 22, 2020

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Oct 22, 2020

AWS CodeBuild CI Report

hnishar 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aijunpeng commented Oct 26, 2020

aws-solutions-constructs-team commented Oct 26, 2020

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Oct 26, 2020

AWS CodeBuild CI Report

aws-solutions-constructs-team commented Oct 26, 2020

AWS CodeBuild CI Report

hnishar commented Oct 26, 2020