feat(iot): add the TopicRule L2 construct by yamatatsu · Pull Request #16681 · aws/aws-cdk

yamatatsu · 2021-09-28T06:26:06Z

I tried to implement aws-iot L2 Constructs.

I did following:

add L2 construct
add unit tests
add integration test
test with deploying to my AWS account and sending MQTT.
update package.json

resolves: #16602

I should do following for undrafting:

write comments
implement other actions
Following is not implemented yet, but I will implements other PR.
- Elasticsearch
- Kinesis Firehose
- Kinesis Stream
- http
- IoT Analytics
- IoT Events
- IoT SiteWise
- kafka
- Step Functions
write README

Design

TopicRule and IAction

Implements of IAction

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

gitpod-io · 2021-09-28T06:26:08Z

1. add The TopicRule L2 construct 2. add unit tests 3. add integration test 4. update package.json

yamatatsu · 2021-10-12T06:23:00Z

@skinny85
Could you have time for this review?
Or should I ask someone else to review it?

skinny85

This looks like a great contribution @yamatatsu! It needs some work, but it's a great start.

Can I have one suggestion? Right now, the PR is gigantic. The chance of quickly merging a PR with 51 files changed and 5,000 lines of code added are very, very slim.

What do you think about splitting it up? Perhaps something like:

Just TopicRule, and minimal properties to get it deployed (with unit and integration tests).
Remaining properties of the TopicRule.
New @aws-cdk/aws-iot-actions package (see my comments), with like 1 or 2 IAction implementations?
(and higher) Separate PRs for new @aws-cdk/aws-iot-actions implementations - depending on the size, maybe one to a few in each PR?

Let me know what you think!

Thanks,
Adam

package.json

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

packages/@aws-cdk/aws-iot/lib/action.ts

packages/@aws-cdk/aws-iot/lib/topic-rule.ts

packages/@aws-cdk/aws-iot/package.json

packages/@aws-cdk/aws-iot/test/actions/cloudwatch-alarm/cloudwatch-alarm-action.test.ts

packages/@aws-cdk/aws-iot/lib/action.ts

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

Pull request has been modified.

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

yamatatsu · 2021-10-13T12:05:59Z

@skinny85
Thank you for your review!

I think too that it is better to split PRs and packages. But I could not image steps to complete these implementations.

Your steps is sounds good and I will do it!

Just TopicRule, and minimal properties to get it deployed (with unit and integration tests).

Remaining properties of the TopicRule.

New @aws-cdk/aws-iot-actions package (see my comments), with like 1 or 2 IAction implementations?

(and higher) Separate PRs for new @aws-cdk/aws-iot-actions implementations - depending on the size, maybe one to a few in each PR?

yamatatsu · 2021-10-14T06:09:27Z

@skinny85
I've done to make the PR small!

skinny85

Thanks so much for shrinking the PR @yamatatsu! Now it's very easy to review 🙂.

The general direction is great. I have some comments on the details, but I think we're off to a great start here - awesome work!

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

skinny85 · 2021-10-14T19:34:22Z

packages/@aws-cdk/aws-iot/lib/topic-rule.ts

+   *
+   * @see https://docs.aws.amazon.com/iot/latest/developerguide/iot-sql-reference.html
+   */
+  readonly sql: string;


Let's model this a little bit differently.

I have a feeling we might want to add a nice API around constructing the SQL string too. Let's "future proof" this a little bit.

I know you had an enum here before that allowed you to choose the version used. Let's combine the version with this concept to make it what CDK calls a union-like class.

So, we'll have:

export interface TopicRuleProps { // ... readonly sql: IotSql; } export abstract class IotSql { public static ver_2015_10_08_fromString(sql: string): IotSql { ... } // ... public abstract bind(scope: Construct): IotSqlConfig; }

Take a look at how these are implemented in the AppMesh library, which uses this pattern often; here's a good example.

OK. I will implement as lambda.Code and appmesh.HttpRoutePathMatch.

But I'd like to understand more clearly.
I wonder why this implementation is "future proof"?
Is it because the user's code will explicit the SQL version?

I meant "future proof" because you removed the the SQL version enum in this iteration of the PR (which was the correct thing to do, BTW), and I'm kind of bringing it back with this vision for the IotSql class.

packages/@aws-cdk/aws-iot/test/integ.topic-rule.ts

packages/@aws-cdk/aws-iot/test/topic-rule.test.ts

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

Pull request has been modified.

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

mergify · 2021-10-21T17:54:17Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2021-10-21T18:46:33Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: f2c0a38
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2021-10-21T18:46:58Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

yamatatsu · 2021-10-22T04:38:25Z

@skinny85 I appreciate your help. arigato gozaimashita !!

I'm trying to implement aws-iot L2 Constructs. This PR is the next step of #16681 refar: - #16681 (comment) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…7225) I'm trying to implement aws-iot L2 Constructs. This PR is one of steps after following PR: - #16681 (comment) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

I'm trying to implement aws-iot L2 Constructs. This PR is one of steps after following PR: - #16681 (comment) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

I'm trying to implement aws-iot L2 Constructs. This PR is one of steps after following PR: - aws#16681 (comment) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

yamatatsu mentioned this pull request Sep 28, 2021

(iot): L2 support for TopicRule #16602

Closed

2 tasks

yamatatsu force-pushed the aws-iot branch 2 times, most recently from 2c58467 to 49a0bb2 Compare September 29, 2021 06:48

This comment has been minimized.

Sign in to view

RomainMuller force-pushed the master branch from 924c117 to ebfd5f2 Compare September 29, 2021 13:07

yamatatsu force-pushed the aws-iot branch from f11e2ea to e71ad76 Compare October 5, 2021 00:57

yamatatsu changed the title ~~feat(iot): Support L2 constructs~~ feat(iot): support the TopicRule L2 construct Oct 6, 2021

feat(iot): support the TopicRule L2 construct

e6c26f5

1. add The TopicRule L2 construct 2. add unit tests 3. add integration test 4. update package.json

yamatatsu force-pushed the aws-iot branch from 7379534 to e6c26f5 Compare October 6, 2021 10:54

Merge branch 'master' into aws-iot

259ca2e

yamatatsu marked this pull request as ready for review October 6, 2021 12:10

Merge branch 'master' into aws-iot

61c9d70

skinny85 previously requested changes Oct 12, 2021

View reviewed changes

skinny85 reviewed Oct 12, 2021

View reviewed changes

packages/@aws-cdk/aws-iot/lib/action.ts Outdated Show resolved Hide resolved

Update packages/@aws-cdk/aws-iot/lib/action.ts

5b3f786

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

Update packages/@aws-cdk/aws-iot/lib/topic-rule.ts

84001cd

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

yamatatsu added 2 commits October 13, 2021 23:02

fix(iot): be a small PR

94dcbbe

Merge branch 'master' into aws-iot

f969810

yamatatsu requested a review from skinny85 October 13, 2021 16:59

skinny85 previously requested changes Oct 14, 2021

View reviewed changes

Update packages/@aws-cdk/aws-iot/lib/topic-rule.ts

e2b78f5

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

yamatatsu and others added 3 commits October 15, 2021 22:19

Update packages/@aws-cdk/aws-iot/lib/topic-rule.ts

4023a08

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

Update packages/@aws-cdk/aws-iot/test/topic-rule.test.ts

912fdaf

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

Update packages/@aws-cdk/aws-iot/test/topic-rule.test.ts

1562ac6

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

github-actions bot assigned skinny85 Oct 21, 2021

github-actions bot added the @aws-cdk/aws-iot Related to AWS IoT label Oct 21, 2021

skinny85 removed their assignment Oct 21, 2021

Merge branch 'master' into aws-iot

f2c0a38

mergify bot merged commit 86f85ce into aws:master Oct 21, 2021

This was referenced Oct 22, 2021

feat(iot): allow setting Actions of TopicRule #17110

Merged

feat(iot): create new aws-iot-actions module #17112

Merged

This was referenced Oct 29, 2021

feat(iot): allow setting description and enabled of TopicRule #17225

Merged

feat(iot-actions): add the action to put CloudWatch Logs #17228

Merged

This was referenced Nov 3, 2021

feat(iot): allow setting errorAction of TopicRule #17287

Merged

feat(iot): add Action to put objects in S3 Buckets #17307

Merged

yamatatsu deleted the aws-iot branch November 5, 2021 01:39

yamatatsu mentioned this pull request Nov 11, 2021

feat(iot): add Action to put records to a Firehose stream #17466

Merged

yamatatsu mentioned this pull request Nov 15, 2021

feat(iot): add Action to capture CloudWatch metrics #17503

Merged

renovate bot mentioned this pull request Dec 7, 2021

fix(deps): update aws-cdk monorepo to v1.135.0 capralifecycle/liflig-repo-metrics#52

Merged

1 task

renovate bot mentioned this pull request Jan 7, 2022

Update aws cdk to v1.149.0 cohalz/cirrocumulus#130

Open

1 task

overheat mentioned this pull request Mar 20, 2022

(module name): Add the TopicRule L2 construct for CDK v2. #19474

Closed

2 tasks

Conversation

yamatatsu commented Sep 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Design

TopicRule and IAction

Implements of IAction

Uh oh!

gitpod-io bot commented Sep 28, 2021

Uh oh!

This comment has been minimized.

yamatatsu commented Oct 12, 2021

Uh oh!

skinny85 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yamatatsu commented Oct 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yamatatsu commented Oct 14, 2021

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

skinny85 Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!

yamatatsu Oct 15, 2021

Choose a reason for hiding this comment

Uh oh!

skinny85 Oct 15, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mergify bot commented Oct 21, 2021

Uh oh!

aws-cdk-automation commented Oct 21, 2021

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Oct 21, 2021

Uh oh!

yamatatsu commented Oct 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yamatatsu commented Sep 28, 2021 •

edited

Loading

skinny85 left a comment •

edited

Loading

yamatatsu commented Oct 13, 2021 •

edited

Loading

yamatatsu commented Oct 22, 2021 •

edited

Loading