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-actions): support for sending messages to iot-events #19953

Merged
merged 9 commits into from
Aug 8, 2022

Conversation

yamatatsu
Copy link
Contributor

@yamatatsu yamatatsu commented Apr 18, 2022

This PR includes to support the action for sending messages to IoT Events.

This feature is described this documentation.

I actually confirmed that the behavior of the action deployed by integ-test is all right.


All Submissions:

Adding new Unconventional Dependencies:

  • This PR adds new unconventional dependencies following the process described here

New Features

  • Have you added the new feature to an integration test?
    • Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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 Apr 18, 2022

@aws-cdk-automation aws-cdk-automation requested a review from a team April 18, 2022 09:32
@github-actions github-actions bot added the p2 label Apr 18, 2022
@TheRealAmazonKendra TheRealAmazonKendra changed the base branch from v1-main to main July 13, 2022 00:06
Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

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

Same comment as #19949, unfortunately changing to the new branch made this messy.

@mergify mergify bot dismissed TheRealAmazonKendra’s stale review July 30, 2022 05:19

Pull request has been modified.

@yamatatsu
Copy link
Contributor Author

I’ve rebased it!

@TheRealAmazonKendra TheRealAmazonKendra changed the title feat(iot-actions): Support to send message to IoT Events feat(iot-actions): support to send message to iot-events Aug 1, 2022
@TheRealAmazonKendra TheRealAmazonKendra changed the title feat(iot-actions): support to send message to iot-events feat(iot-actions): support for sending messages to iot-events Aug 1, 2022
Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

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

Overall this is great work! Just a few comments inline.

*
* When batchMode is true and the rule SQL statement evaluates to an Array,
* each Array element is treated as a separate message when Events by calling BatchPutMessage.
* The resulting array can't have more than 10 messages.
Copy link
Contributor

Choose a reason for hiding this comment

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

If the array does have more than 10 messages how/when does this fail? What's the user experience here? We should probably have some enforcement here, if possible.

Copy link
Contributor Author

@yamatatsu yamatatsu Aug 3, 2022

Choose a reason for hiding this comment

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

Not explained in the documentation, I've experimented it with the below command:

aws iot-data publish --topic device/mydevice/data --qos 1 --payload (echo '[{"payload":{"deviceId":"001"}},{"payload":{"deviceId":"002"}},{"payload":{"deviceId":"003"}},{"payload":{"deviceId":"004"}},{"payload":{"deviceId":"005"}},{"payload":{"deviceId":"006"}},{"payload":{"deviceId":"007"}},{"payload":{"deviceId":"008"}},{"payload":{"deviceId":"009"}},{"payload":{"deviceId":"010"}},{"payload":{"deviceId":"011"}}]' | base64) --region us-east-1

and get below error message:

Failed to send message to Iot Events. The payload containing 11 messages cannot have more than 10 messages when batchMode=true.. 

I thing it is not possible to restrict it, because it depends to published payloads (instead of SQL) that is out of CDK.
WDYT?

export class IotEventsPutMessageAction implements iot.IAction {
private readonly batchMode?: boolean;
private readonly messageId?: string;
private readonly role?: iam.IRole;
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the behavior if a role is not set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If a role is not set, a new role will be created and be granted some policies needed.

It's explained in CommonActionProps.

/**
* The IAM role that allows access to AWS service.
*
* @default a new role will be created
*/
readonly role?: iam.IRole;

packages/@aws-cdk/aws-iot-actions/README.md Show resolved Hide resolved
@mergify mergify bot dismissed TheRealAmazonKendra’s stale review August 3, 2022 13:42

Pull request has been modified.

Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

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

I'm going to put this back into changes requested since the build is failing, that way I'll get notified when it's been updated.

@mergify mergify bot dismissed TheRealAmazonKendra’s stale review August 4, 2022 14:16

Pull request has been modified.

Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

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

Just one more comment. I don't really want to add exclusions to our lint rules.

@@ -100,6 +100,11 @@
"engines": {
"node": ">= 14.15.0"
},
"awslint": {
"exclude": [
"no-unused-type:@aws-cdk/aws-iot.ActionConfig"
Copy link
Contributor

Choose a reason for hiding this comment

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

What's causing this violation? I generally don't think we should be adding lint exclusions but instead should make sure the code aligns to them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TheRealAmazonKendra
I've researched it.
It was caused by addind @internal to method IAction._bind(), the message is:

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

It appears that IAction._bind() is no longer listed in Type.ownMethods.
This caused ActionConfig to be markded as unused-type.

Changing ActionConfig to not export will suppress this violation. However, if we do so, ActionConfig will not be able to be used in aws-iot-actions.

Like this, when an interfaces is extended across packages, adding lint exclusions seems reasonable.
WDYT?

@mergify
Copy link
Contributor

mergify bot commented Aug 8, 2022

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 96c2936
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify mergify bot merged commit 35fc169 into aws:main Aug 8, 2022
@mergify
Copy link
Contributor

mergify bot commented Aug 8, 2022

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

jmortlock pushed a commit to jmortlock/aws-cdk that referenced this pull request Aug 8, 2022
)

This PR includes to support the action for sending messages to IoT Events.

This feature is described [this documentation](https://docs.aws.amazon.com/iot/latest/developerguide/iotevents-rule-action.html).

I actually confirmed that the behavior of the action deployed by integ-test is all right.

----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)?
	* [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
josephedward pushed a commit to josephedward/aws-cdk that referenced this pull request Aug 30, 2022
)

This PR includes to support the action for sending messages to IoT Events.

This feature is described [this documentation](https://docs.aws.amazon.com/iot/latest/developerguide/iotevents-rule-action.html).

I actually confirmed that the behavior of the action deployed by integ-test is all right.

----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)?
	* [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants