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

yamatatsu · 2021-11-03T15:43:18Z

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

This PR is one of steps after following PR:

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

1. add the action 2. add tests 3. describe to README

gitpod-io · 2021-11-03T15:43:21Z

In CodeBuild CI, there was following error: ``` @aws-cdk/aws-iot-actions-alpha: - [yarn/nohoist-bundled-dependencies] Repository-level 'workspaces.nohoist' directive is missing: @aws-cdk/aws-iot-actions-alpha/case, @aws-cdk/aws-iot-actions-alpha/case/** (fixable) @aws-cdk/aws-iot-actions-alpha: Error: Some package.json files had errors @aws-cdk/aws-iot-actions-alpha: at main (/codebuild/output/src514100616/src/github.com/aws/aws-cdk/tools/@aws-cdk/pkglint/bin/pkglint.js:30:15) @aws-cdk/aws-iot-actions-alpha: at Object.<anonymous> (/codebuild/output/src514100616/src/github.com/aws/aws-cdk/tools/@aws-cdk/pkglint/bin/pkglint.js:33:1) @aws-cdk/aws-iot-actions-alpha: at Module._compile (internal/modules/cjs/loader.js:999:30) @aws-cdk/aws-iot-actions-alpha: at Object.Module._extensions..js (internal/modules/cjs/loader.js:1027:10) @aws-cdk/aws-iot-actions-alpha: at Module.load (internal/modules/cjs/loader.js:863:32) @aws-cdk/aws-iot-actions-alpha: at Function.Module._load (internal/modules/cjs/loader.js:708:14) @aws-cdk/aws-iot-actions-alpha: at Module.require (internal/modules/cjs/loader.js:887:19) @aws-cdk/aws-iot-actions-alpha: at require (internal/modules/cjs/helpers.js:74:18) @aws-cdk/aws-iot-actions-alpha: at Object.<anonymous> (/codebuild/output/src514100616/src/github.com/aws/aws-cdk/tools/@aws-cdk/pkglint/bin/pkglint:2:1) @aws-cdk/aws-iot-actions-alpha: at Module._compile (internal/modules/cjs/loader.js:999:30) ```

skinny85

Looks great @yamatatsu! A few minor comments.

skinny85 · 2021-11-03T17:18:16Z

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

+
+new iot.TopicRule(this, 'TopicRule', {
+  sql: iot.IotSql.fromStringAsVer20160323("SELECT topic(2) as device_id FROM 'device/+/data'"),
+  actions: [new actions.S3Action(bucket)],


Can you illustrate the optional properties of S3Action in this example? Especially how to use the substitution templates.

Oh. It is important for S3Action! I will illustrate it!

skinny85 · 2021-11-03T17:24:07Z

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

+   *
+   * @default a new role will be created
+   */
+  readonly role?: iam.IRole;


Will every Action have a role property? Because if the answer is "yes", we should introduce a common interface that all Action prop interfaces extend. Like the CommonActionProps in @aws-cdk/aws-codepipeline.

@skinny85
Not every Action is need a role, but almost. 2 actions in all actions will not have a role property.

HTTP Action

Lambda Action

For example, LambdaFunctionAction constructor (most simple one) is as following:

constructor(private readonly func: lambda.IFunction) {}

So I thought there are no common property for all actions. But the way to align the arguments of the action's constructor is what I was also struggling with.

What do you think? I'm open to any and all refactoring!

Ok, so if all of actions besides two will need role, I think introducing a separate common interface makes sense. And make S3ActionProps (soon to be changed to S3PutObjectActionProps I hope 😉) extend this new interface.

OK! I will! 🚀

@skinny85
If we create CommonActionProps and CommonAwsActionProps, should these constructors be unified in the following form?

interface CommonActionProps { } intercace CommonAwsActionProps extends CommonActionProps { role?: iam.IRole; } class Action { constructor(props: CommonActionProps) {} }

Now, three Actions LambdaFunctionAction, CloudWatchLogsAction and S3PutObjectActionProps have following constructor, and first argument of them constroctor is not Props:

LambdaFunctionAction

constructor(func: lambda.IFunction) {}

CloudWatchLogsAction

constructor(logGroup: logs.ILogGroup, props: CloudWatchLogsActionProps) {}

S3PutObjectActionProps

constructor(bucket: s3.IBucket, props: S3PutObjectActionProps) {}

So if first question is "yes", I will do this refactoring in another PR!

No. Let's leave the constructors as they are now.

BTW, there's no point in having both CommonActionProps and CommonAwsActionProps for IoT Actions - you only need one common superinterface (it's a different story with CodePipeline Actions, but I don't want to get into the details of why that is here).

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

skinny85 · 2021-11-03T17:28:13Z

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

+   * Supports substitution templates.
+   * @see https://docs.aws.amazon.com/iot/latest/developerguide/iot-substitution-templates.html
+   *
+   * @default '${topic()}/${timestamp()}'


Should we have static, string-typed constants for these substitution templates? So that it's clear which ones can be used by which Actions?

I think we cannot define all static, string-typed constants. Because users can use MQTT payload property defined by themselves in template.

For example, there are following MQTT payload:

{ "year": "2021", "month": "Nov", "day": "04", "data": 123.4 }

The user can define the bucket key as ${year}/${month}/${day}/${timestamp()}.

Are we on the same page?

I don't think we need to have all of them - but you have a few that are always available, right? Like ${timestamp()} and ${topic()}. Maybe it's worth it to include those that are always available, and just document the other ones?

OK, I also want to make it as user-friendly as possible. So I'd like to be on same page!

there are about 70 functions, and they take arguments

So we need to use functions, instead of static constants.

there are also Operators.

So it is better that our functions return without ${} as topic().

in many cases, they are used embedded in strings without any regularity. For example, cloudwatch metric name could be used substitution templates.

And I thought 3 plans.

PLAN1, implement a set of functions

In light of the above, it might be better for us to implement a set of functions and for users to use them in template literals, as following:

key: `\${${iot.Template.topic(2)}}/\${${iot.Template.timestamp()}}`

PLAN2, just validation

Or, is it better to implement validation as JsonPath of @aws-cdk/aws-stepfunctions
https://github.com/aws/aws-cdk/tree/master/packages/@aws-cdk/aws-stepfunctions/lib/fields.ts#L216-L226

For substitution templates, I think we will be using a lot of regular expressions.

PLAN3, implement Expression and Template

Or, we implement Expression and Template?

I mean Expression as following:

iot.Expression.substring(iot.Expression.topic(2), 1) // return type of Expression

I mean Template as following:

iot.Template.format( '{}/{}', iot.Expression.substring(iot.Expression.topic(2), 5, 10), iot.Expression.timestamp(), ) // return type of Template

And, properties that can be used substitution templates will have the type Template.

{ key: iot.Template, }

The above may help to naturally communicate to users that substitution templates are available.

Before I answer, I have one more clarifying question.

Are any of the substitutions Action-dependent? So, are there any substitutions that can only be used with certain Action(s)?

In any case, I see now this is a much more complicated topic than I initially thought, and should be tackled in a separate PR. Let's continue the discussion on how to best model them - that's fine, but I don't want to block this PR on the discussion. Let's proceed with this PR without anything around substitutions, like it is now.

Are any of the substitutions Action-dependent? So, are there any substitutions that can only be used with certain Action(s)?

As far as I can tell from the documentation, no. And from the documentation, they all seem to be common functions and not Action-dependent. 🙂

Let's proceed with this PR without anything around substitutions, like it is now.

Sounds good!

Are any of the substitutions Action-dependent? So, are there any substitutions that can only be used with certain Action(s)?

As far as I can tell from the documentation, no. And from the documentation, they all seem to be common functions and not Action-dependent. 🙂

Great! Given that, I would go with Plan 1 from #17307 (comment).

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

Pull request has been modified.

…-cdk into aws-iot-actions-s3

as following: error JSII9002: Unable to resolve type "@aws-cdk/aws-iot-actions.CommonActionProps". It may be @internal or not exported from the module's entry point (as configured in "package.json" as "main").

skinny85

Looks great @yamatatsu! One minor naming change.

skinny85 · 2021-11-09T18:16:55Z

packages/@aws-cdk/aws-iot-actions/lib/private/common-action-props.ts

+/**
+ * Common properties shared by Actions it access to AWS service.
+ *
+ * @internal
+ */


I think it's fine to make this public.

skinny85 · 2021-11-09T18:17:14Z

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

+  ),
+  actions: [
+    new actions.S3PutObjectAction(bucket, {
+      key: '${year}/${month}/${day}/${topic(2)}',


Show cannedAcl too?

So, where is it? 🙂

OK, I see below. In the future, I would merge these examples, because otherwise the ReadMe becomes way too long.

skinny85 · 2021-11-09T18:19:16Z

packages/@aws-cdk/aws-iot-actions/lib/s3-put-object-action.ts

+   *
+   * @default None
+   */
+  readonly cannedAcl?: s3.BucketAccessControl;


I don't like the name cannedAcl. Let's call this accessControl, like we do in CodePipeline's S3DeployAction.

skinny85 · 2021-11-09T18:20:01Z

packages/@aws-cdk/aws-iot-actions/lib/s3-put-object-action.ts

+  private putEventStatement(bucket: s3.IBucket) {
+    return new iam.PolicyStatement({
+      actions: ['s3:PutObject'],
+      resources: [bucket.arnForObjects('*')],
+    });
+  }


Let's inline this method, it doesn't add much value.

skinny85 · 2021-11-09T18:20:43Z

packages/@aws-cdk/aws-iot-actions/test/s3-put-object/integ.s3-put-object-action.ts

+    });
+    topicRule.addAction(
+      new actions.S3PutObjectAction(bucket, {
+        key: '${year}/${month}/${day}/${topic(2)}',


Let's use ~~cannedAcl~~accessControl here too.

skinny85 · 2021-11-09T18:22:27Z

packages/@aws-cdk/aws-iot-actions/test/s3-put-object/s3-put-object-action.test.ts

+        {
+          S3: {
+            BucketName: 'test-bucket',
+            Key: 'test-key',


Let's only assert for the presence of this Key (you might have to use the objectLike and arrayLike helpers to achieve this).

Beautiful!🤩

packages/@aws-cdk/aws-iot-actions/test/s3-put-object/s3-put-object-action.test.ts

This PR refactor the test that I committed earlier based on the above comment. - #17307 (comment) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Pull request has been modified.

skinny85

Looks great @yamatatsu!

skinny85 · 2021-11-10T19:41:59Z

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

+  ),
+  actions: [
+    new actions.S3PutObjectAction(bucket, {
+      key: '${year}/${month}/${day}/${topic(2)}',


OK, I see below. In the future, I would merge these examples, because otherwise the ReadMe becomes way too long.

mergify · 2021-11-10T19:46:42Z

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

This PR refactor the test that I committed earlier, and is based on the following comment. - #17307 (comment) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

aws-cdk-automation · 2021-11-10T21:46:15Z

AWS CodeBuild CI Report

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

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

mergify · 2021-11-10T21:46:57Z

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

This PR refactor the test that I committed earlier based on the above comment. - aws#17307 (comment) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

This PR refactor the test that I committed earlier, and is based on the following comment. - aws#17307 (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*

feat(iot-actions): Add the action to put s3 bucket objects

be3be36

1. add the action 2. add tests 3. describe to README

github-actions bot added the @aws-cdk/aws-iot-actions label Nov 3, 2021

github-actions bot assigned skinny85 Nov 3, 2021

skinny85 previously requested changes Nov 3, 2021

View reviewed changes

yamatatsu added 2 commits November 7, 2021 22:34

fix(iot-actions): address comments

4b66370

fix(iot-actions): rename files

0a37bdd

yamatatsu mentioned this pull request Nov 7, 2021

chore(iot-actions): simplify cloudwatch-logs-action test #17385

Merged

yamatatsu added 4 commits November 9, 2021 23:10

Merge branch 'master' into aws-iot-actions-s3

9023c01

refactor(iot-actions): superinterface for action props

6fc3248

Merge branch 'aws-iot-actions-s3' of https://github.com/yamatatsu/aws…

a870670

…-cdk into aws-iot-actions-s3

fix(iot-actions): fix jsii lint error

f0a3480

as following: error JSII9002: Unable to resolve type "@aws-cdk/aws-iot-actions.CommonActionProps". It may be @internal or not exported from the module's entry point (as configured in "package.json" as "main").

yamatatsu requested a review from skinny85 November 9, 2021 15:50

skinny85 previously requested changes Nov 9, 2021

View reviewed changes

address comments

f4c292c

yamatatsu requested a review from skinny85 November 10, 2021 11:52

yamatatsu mentioned this pull request Nov 10, 2021

chore(iot-actions): clarify the test difference #17451

Merged

skinny85 approved these changes Nov 10, 2021

View reviewed changes

skinny85 changed the title ~~feat(iot-actions): Add the action to put s3 bucket objects~~ feat(iot-actions): add Action to put objects in S3 Buckets Nov 10, 2021

skinny85 changed the title ~~feat(iot-actions): add Action to put objects in S3 Buckets~~ feat(iot): add Action to put objects in S3 Buckets Nov 10, 2021

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

Merge branch 'master' into aws-iot-actions-s3

d524a80

mergify bot merged commit 49b87db into aws:master Nov 10, 2021

yamatatsu deleted the aws-iot-actions-s3 branch December 16, 2021 09:31

sthuber90 mentioned this pull request Feb 6, 2023

(iot): TopicRule action for Location #24035

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

yamatatsu commented Nov 3, 2021

gitpod-io bot commented Nov 3, 2021

skinny85 left a comment

skinny85 Nov 3, 2021

yamatatsu Nov 4, 2021

skinny85 Nov 3, 2021

yamatatsu Nov 4, 2021 •

edited

Loading

skinny85 Nov 4, 2021

yamatatsu Nov 5, 2021

yamatatsu Nov 8, 2021

skinny85 Nov 8, 2021

skinny85 Nov 3, 2021

yamatatsu Nov 4, 2021

skinny85 Nov 4, 2021

yamatatsu Nov 5, 2021

skinny85 Nov 5, 2021

yamatatsu Nov 8, 2021

skinny85 Nov 8, 2021

skinny85 left a comment

skinny85 Nov 9, 2021

skinny85 Nov 9, 2021

skinny85 Nov 10, 2021

skinny85 Nov 10, 2021

skinny85 Nov 9, 2021

skinny85 Nov 9, 2021

skinny85 Nov 9, 2021

skinny85 Nov 9, 2021

yamatatsu Nov 10, 2021

skinny85 left a comment

skinny85 Nov 10, 2021

mergify bot commented Nov 10, 2021

aws-cdk-automation commented Nov 10, 2021

mergify bot commented Nov 10, 2021

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

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

Conversation

yamatatsu commented Nov 3, 2021

gitpod-io bot commented Nov 3, 2021

skinny85 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

yamatatsu Nov 4, 2021 • edited Loading

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

PLAN1, implement a set of functions

PLAN2, just validation

PLAN3, implement Expression and Template

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 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

skinny85 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Nov 10, 2021

aws-cdk-automation commented Nov 10, 2021

AWS CodeBuild CI Report

mergify bot commented Nov 10, 2021

yamatatsu Nov 4, 2021 •

edited

Loading

PLAN3, implement `Expression` and `Template`