feat(iot): allow setting Actions of TopicRule by yamatatsu · Pull Request #17110 · aws/aws-cdk

yamatatsu · 2021-10-22T04:44:43Z

I'm trying to implement aws-iot L2 Constructs.

This PR is the next step of #16681

refar:

feat(iot): add the TopicRule L2 construct #16681 (comment)

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

gitpod-io · 2021-10-22T04:44:46Z

yamatatsu · 2021-10-22T06:14:00Z

I met following error on yarn build at aws-iot 🤔

error: [awslint:no-unused-type:@aws-cdk/aws-iot.ActionConfig] type or enum is not used by exported classes (declared at lib/action.ts:19)
error: [awslint:no-unused-type:@aws-cdk/aws-iot.IAction] type or enum is not used by exported classes (declared at lib/action.ts:7)

And I confirmed this error was suppressed when using the IAction in TopicRule.

Shall we implement using the IAction in TopicRule as possible as simple? (ex. not implement addAction())

skinny85 · 2021-10-22T17:13:43Z

Shall we implement using the IAction in TopicRule as possible as simple? (ex. not implement addAction())

Yes, we should implement using IAction in this PR too (add actions: IAction[] to TopicRuleProps, and then use it inside TopicRule). If you want to implement TopicRule.addAction() in this PR, I'm fine with that too.

skinny85

Putting in "Request changes" to clear this one from my To-Do list. @yamatatsu please re-request my review when this is ready!

Pull request has been modified.

This change adds `IAction` to aws-iot for preparing to implemamt AWS IoT Rule actions.

skinny85

Now that we have the @aws-cdk/aws-iot-actions module on master, let's add a single implementation of IAction there, and use that in the ReadMe of @aws-cdk/aws-iot, instead of an anonymous object.

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

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

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

Pull request has been modified.

skinny85

Looks great @yamatatsu! Minor suggestions before we merge this in.

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

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

skinny85 · 2021-10-26T19:22:10Z

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

+  constructor(private readonly lambdaFn: lambda.IFunction) {}
+
+  bind(rule: iot.ITopicRule): iot.ActionConfig {
+    this.lambdaFn.addPermission('invokedByAwsIotRule', {


Can you use function.grantInvoke() instead?

@skinny85 Thank you for your review!

In my understanding, using addPermission() is better. Because a lambda resource policy created by using grantInvoke() is more lax than using addPermission().

As described in this document, lambda resource policy fields sourceAccount and sourceArn are required (or recommended?). But functions.grantInvoke() cannot add sourceAccount and sourceArn as this code.

For confirmation, I tried following code instead of this.func.addPermission():

this.func.grantInvoke({ grantPrincipal: new iam.ServicePrincipal('iot.amazonaws.com') });

Then there is template difference as following:

OK, fair enough 🙂.

packages/@aws-cdk/aws-iot-actions/test/lambda/integ.lambda-action.ts

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

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

yarn.lock

Pull request has been modified.

skinny85

This is almost perfect @yamatatsu! Change the account in LambdaAction, and rename it to LambdaFunctionAction, and we'll merge this in!

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

skinny85 · 2021-10-27T18:34:48Z

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

+   */
+  constructor(private readonly func: lambda.IFunction) {}
+
+  bind(rule: iot.ITopicRule): iot.ActionConfig {


Suggested change

bind(rule: iot.ITopicRule): iot.ActionConfig {

bind(topicRule: iot.ITopicRule): iot.ActionConfig {

skinny85 · 2021-10-27T18:37:30Z

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

+   *
+   * @param rule The TopicRule that would trigger this action.
+   */
+  bind(rule: ITopicRule): ActionConfig;


Suggested change

bind(rule: ITopicRule): ActionConfig;

bind(topicRule: ITopicRule): ActionConfig;

skinny85 · 2021-10-27T18:40:06Z

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

+/**
+ * The action to invoke an AWS Lambda function, passing in an MQTT message.
+ */
+export class LambdaAction implements iot.IAction {


Let's rename this to LambdaFunctionAction.

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

Pull request has been modified.

yamatatsu · 2021-10-28T08:42:11Z

@skinny85 I've addressed the comments!👍

skinny85

Looks perfect @yamatatsu, thanks for incorporating all of my comments!

mergify · 2021-10-28T17:16:04Z

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).

mergify · 2021-10-28T18:09:03Z

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-28T18:09:07Z

AWS CodeBuild CI Report

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

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

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*

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

github-actions bot assigned skinny85 Oct 22, 2021

yamatatsu force-pushed the aws-iot-rule-action branch from 7388b1c to f68be0a Compare October 22, 2021 05:21

skinny85 previously requested changes Oct 22, 2021

View reviewed changes

yamatatsu force-pushed the aws-iot-rule-action branch from f68be0a to 9fcb358 Compare October 23, 2021 09:05

feat(iot): Add IAction

7a72b32

This change adds `IAction` to aws-iot for preparing to implemamt AWS IoT Rule actions.

yamatatsu force-pushed the aws-iot-rule-action branch from 9fcb358 to 7a72b32 Compare October 23, 2021 14:29

yamatatsu requested a review from skinny85 October 24, 2021 01:20

skinny85 previously requested changes Oct 25, 2021

View reviewed changes

skinny85 reviewed Oct 25, 2021

View reviewed changes

packages/@aws-cdk/aws-iot/test/topic-rule.test.ts Outdated Show resolved Hide resolved

skinny85 reviewed Oct 25, 2021

View reviewed changes

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

skinny85 reviewed Oct 25, 2021

View reviewed changes

packages/@aws-cdk/aws-iot/test/topic-rule.test.ts Outdated Show resolved Hide resolved

yamatatsu added 3 commits October 27, 2021 00:17

fix(iot): about feedbacks

7511ed0

Merge branch 'master' into aws-iot-rule-action

9c704cc

feat(iot-actions): Add lambda action

db70b7f

fix(iot-actions): fix pkglint for depending on experimental aws-iot

e8e9652

yamatatsu requested a review from skinny85 October 26, 2021 17:03

skinny85 previously requested changes Oct 26, 2021

View reviewed changes

yamatatsu added 2 commits October 27, 2021 15:15

revert: yarn.lock

dd965da

fix: as feadback

3382316

skinny85 previously requested changes Oct 27, 2021

View reviewed changes

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

9058c33

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

fix(iot): about feedbacks

621cdf4

refactor(iot-actions): rename files

9eca063

yamatatsu requested a review from skinny85 October 28, 2021 07:20

skinny85 approved these changes Oct 28, 2021

View reviewed changes

skinny85 changed the title ~~feat(iot): Add IAction~~ feat(iot): add the concept of Actions to TopicRule Oct 28, 2021

skinny85 changed the title ~~feat(iot): add the concept of Actions to TopicRule~~ feat(iot): allow setting Actions of TopicRule Oct 28, 2021

Merge branch 'master' into aws-iot-rule-action

c86e5f0

mergify bot merged commit 0cabb9f into aws:master Oct 28, 2021

yamatatsu mentioned this pull request Nov 4, 2021

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

Merged

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

This was referenced Nov 9, 2021

support lambda actions for IoT topic rules #5420

Closed

@aws-cdk/aws-iot: Add an option for CfnTopicRule to automatically create CfnPermission to invoke Lambda Function #16339

Closed

	bind(rule: iot.ITopicRule): iot.ActionConfig {
	bind(topicRule: iot.ITopicRule): iot.ActionConfig {

	bind(rule: ITopicRule): ActionConfig;
	bind(topicRule: ITopicRule): ActionConfig;

Conversation

yamatatsu commented Oct 22, 2021

Uh oh!

gitpod-io bot commented Oct 22, 2021

Uh oh!

yamatatsu commented Oct 22, 2021

Uh oh!

skinny85 commented Oct 22, 2021

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

skinny85 Oct 26, 2021

Choose a reason for hiding this comment

Uh oh!

yamatatsu Oct 27, 2021

Choose a reason for hiding this comment

Uh oh!

skinny85 Oct 27, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

skinny85 Oct 27, 2021

Choose a reason for hiding this comment

Uh oh!

skinny85 Oct 27, 2021

Choose a reason for hiding this comment

Uh oh!

skinny85 Oct 27, 2021

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Oct 28, 2021

Uh oh!

mergify bot commented Oct 28, 2021

Uh oh!

aws-cdk-automation commented Oct 28, 2021

AWS CodeBuild CI Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yamatatsu commented Oct 28, 2021 •

edited

Loading