-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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): Action to send messages to SQS queues #18087
Conversation
Title does not follow the guidelines of Conventional Commits. Please adjust title before merge. |
@dyoshikawa is this ready for a review, or are you still working on it? |
Sorry, I working now, so not ready for review. |
@skinny85 Ready for review now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great @dyoshikawa! I have one major comment about naming, and then a few minor ones, but this is great.
const stream = new firehose.DeliveryStream(this, 'MyStream', { | ||
destinations: [new destinations.S3Bucket(bucket)], | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this example contain a Firehose DeliveryStream
? It's not used anywhere from what I can see.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I fixed.
/** | ||
* Configuration properties of an action for s3. | ||
*/ | ||
export interface SQSQueueActionProps extends CommonActionProps { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We camel-case acronyms in CDK:
export interface SQSQueueActionProps extends CommonActionProps { | |
export interface SqsQueueActionProps extends CommonActionProps { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I fixed.
/** | ||
* The action to write the data from an MQTT message to an Amazon SQS queue. | ||
*/ | ||
export class SQSQueueAction implements iot.IAction { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export class SQSQueueAction implements iot.IAction { | |
export class SqsQueueAction implements iot.IAction { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I fixed.
topicRule.addAction( | ||
new actions.SQSQueueAction(queue), | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
topicRule.addAction( | |
new actions.SQSQueueAction(queue), | |
); | |
topicRule.addAction(new actions.SQSQueueAction(queue)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I fixed.
import * as cdk from '@aws-cdk/core'; | ||
import * as actions from '../../lib'; | ||
|
||
test('Default sqs queue action', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test('Default sqs queue action', () => { | |
test('Default SQS queue action', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I fixed.
packages/@aws-cdk/aws-iot-actions/test/sqs/sqs-queue-action.test.ts
Outdated
Show resolved
Hide resolved
topicRule.addAction( | ||
new actions.SQSQueueAction(queue), | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
topicRule.addAction( | |
new actions.SQSQueueAction(queue), | |
); | |
topicRule.addAction(new actions.SQSQueueAction(queue)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I fixed.
topicRule.addAction( | ||
new actions.SQSQueueAction(queue, { | ||
useBase64: true, | ||
}), | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
topicRule.addAction( | |
new actions.SQSQueueAction(queue, { | |
useBase64: true, | |
}), | |
); | |
topicRule.addAction(new actions.SQSQueueAction(queue, { | |
useBase64: true, | |
})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I fixed.
topicRule.addAction( | ||
new actions.SQSQueueAction(queue, { role }), | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
topicRule.addAction( | |
new actions.SQSQueueAction(queue, { role }), | |
); | |
topicRule.addAction(new actions.SQSQueueAction(queue, { role })); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I fixed.
@skinny85 Thank you for your review. I fixed by your advices. Can you review one more time? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good @dyoshikawa! A few las tiny details, and we can get this merged in!
const topicRule = new iot.TopicRule(stack, 'MyTopicRule', { | ||
sql: iot.IotSql.fromStringAsVer20160323("SELECT topic(2) as device_id FROM 'device/+/data'"), | ||
}); | ||
const queue = sqs.Queue.fromQueueArn(stack, 'MyQueue', 'arn:aws:s3::123456789012:test-queue'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s3
is probably the wrong service name here 🙂.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I fixed.
Co-authored-by: Adam Ruka <[email protected]>
Co-authored-by: Adam Ruka <[email protected]>
Co-authored-by: Adam Ruka <[email protected]>
"arn:aws:sqs::123456789012:test-queue"
…ws-cdk into aws-iot-actions-sqs
@skinny85 Thank you so much for your reviews. I fixed all, so can you review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic contribution, thanks for working on this @dyoshikawa!
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Fixes aws#17699 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Fixes #17699
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license