Skip to content
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(iot): add the TopicRule L2 construct #16681

Merged
merged 23 commits into from
Oct 21, 2021
Merged

Conversation

yamatatsu
Copy link
Contributor

@yamatatsu yamatatsu commented Sep 28, 2021

I tried to implement aws-iot L2 Constructs.

I did following:

  1. add L2 construct
  2. add unit tests
  3. add integration test
  4. test with deploying to my AWS account and sending MQTT.
  5. 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

image

Implements of IAction

image


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

@gitpod-io
Copy link

gitpod-io bot commented Sep 28, 2021

@yamatatsu

This comment has been minimized.

@yamatatsu yamatatsu changed the title feat(iot): Support L2 constructs feat(iot): support the TopicRule L2 construct Oct 6, 2021
1. add The TopicRule L2 construct
2. add unit tests
3. add integration test
4. update package.json
@yamatatsu yamatatsu marked this pull request as ready for review October 6, 2021 12:10
@yamatatsu
Copy link
Contributor Author

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

Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Just TopicRule, and minimal properties to get it deployed (with unit and integration tests).
  2. Remaining properties of the TopicRule.
  3. New @aws-cdk/aws-iot-actions package (see my comments), with like 1 or 2 IAction implementations?
  4. (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 Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-iot/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-iot/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-iot/lib/action.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-iot/lib/action.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-iot/lib/topic-rule.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-iot/lib/topic-rule.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-iot/lib/topic-rule.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-iot/package.json Outdated Show resolved Hide resolved
Co-authored-by: Adam Ruka <adamruka85@gmail.com>
@mergify mergify bot dismissed skinny85’s stale review October 13, 2021 11:24

Pull request has been modified.

Co-authored-by: Adam Ruka <adamruka85@gmail.com>
@yamatatsu
Copy link
Contributor Author

yamatatsu commented Oct 13, 2021

@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!

  1. Just TopicRule, and minimal properties to get it deployed (with unit and integration tests).
  2. Remaining properties of the TopicRule.
  3. New @aws-cdk/aws-iot-actions package (see my comments), with like 1 or 2 IAction implementations?
  4. (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
Copy link
Contributor Author

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

Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-iot/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-iot/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-iot/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-iot/README.md Show resolved Hide resolved
*
* @see https://docs.aws.amazon.com/iot/latest/developerguide/iot-sql-reference.html
*/
readonly sql: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-iot/test/topic-rule.test.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-iot/test/topic-rule.test.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-iot/test/topic-rule.test.ts Outdated Show resolved Hide resolved
Co-authored-by: Adam Ruka <adamruka85@gmail.com>
@mergify mergify bot dismissed skinny85’s stale review October 15, 2021 13:18

Pull request has been modified.

yamatatsu and others added 3 commits October 15, 2021 22:19
Co-authored-by: Adam Ruka <adamruka85@gmail.com>
Co-authored-by: Adam Ruka <adamruka85@gmail.com>
Co-authored-by: Adam Ruka <adamruka85@gmail.com>
mergify bot pushed a commit that referenced this pull request Oct 22, 2021
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*
mergify bot pushed a commit that referenced this pull request Oct 28, 2021
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*
mergify bot pushed a commit that referenced this pull request Nov 1, 2021
…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*
mergify bot pushed a commit that referenced this pull request Nov 2, 2021
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*
mergify bot pushed a commit that referenced this pull request Nov 4, 2021
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*
@yamatatsu yamatatsu deleted the aws-iot branch November 5, 2021 01:39
mergify bot pushed a commit that referenced this pull request Nov 10, 2021
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*
mergify bot pushed a commit that referenced this pull request Nov 12, 2021
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*
mergify bot pushed a commit that referenced this pull request Nov 24, 2021
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*
beezly pushed a commit to beezly/aws-cdk that referenced this pull request Nov 29, 2021
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*
eladb pushed a commit to cdklabs/decdk that referenced this pull request Jan 18, 2022
I'm trying to implement aws-iot L2 Constructs.

This PR is the next step of #16681

refar: 
- aws/aws-cdk#16681 (comment)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
I tried to implement aws-iot L2 Constructs.

I did following:

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

resolves: aws#16602

I should do following for undrafting:

- [x] write comments
- [x] 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
- [x] write README

----

## Design

### TopicRule and IAction

![image](https://user-images.githubusercontent.com/11013683/136200920-9aa1aa58-2e9f-4a0d-a161-bbe251d02f7d.png)

### Implements of IAction

![image](https://user-images.githubusercontent.com/11013683/136201053-4f693683-3318-4fbf-9a7e-cd3f8ac1a93e.png)



----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
I'm trying to implement aws-iot L2 Constructs.

This PR is the next step of aws#16681

refar: 
- aws#16681 (comment)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
I'm trying to implement aws-iot L2 Constructs.

This PR is the next step of aws#16681

refar: 
- aws#16681 (comment)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
…s#17225)

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*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
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*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
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*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
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*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
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*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-iot Related to AWS IoT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(iot): L2 support for TopicRule
3 participants