Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(iot): add the TopicRule L2 construct #16681
Changes from 7 commits
e6c26f5
259ca2e
61c9d70
5b3f786
84001cd
94dcbbe
f969810
e2b78f5
4023a08
912fdaf
1562ac6
1ada690
9e7256b
59815ad
89a8571
8a94620
a252d75
30d0fa6
f21567e
e150499
13b4701
1bcb870
f2c0a38
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I believe this should be required, no? It's required in the
CfnTopicRule
, at least.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.
I think, no. I think it is better that this is not required.
Because
addAction()
is providedThere 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.
OK. Do we have validation somewhere that at least one Action was provided? Perhaps you want to override the
validate()
method here? Similar like we do in CodePipeline, for example?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.
Oh, sorry, I was mistaken about AWS IoT specification. AWS IoT can be defined with empty actions as following image:
So I'll not implement
validate()
, but it is very useful knowledge for me. Thanks!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.
Hmmm.
Given providing
IAction
s is not required to deploy aTopicRule
- is there any way we can get rid ofIAction
completely from this PR? And introduce it later?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.
@skinny85
OK! I'd like to remove
actions:
from sample codes in the README but not removeIAction
from implementation.Because
Actions
property of CloudFormation is required (though we can use empty array). So if we remove it, here's code will be as following:I think this is a bit confusing.
What do you think?
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.
I don't think this is confusing at all 🙂. This is an implementation detail, the user of
TopicRule
will never see that 🙂.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.
@skinny85
Thank you for your opinion! I think it is better to follow your opinion for simplicity of this PR. I will commit removing!
If I create a PR to implement
IAction
after this PR, is it should includes to implementaws-iot-actions
? Or onlyIAction
?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.
I think the next PR should include:
IAction
interface in@aws-cdk/aws-iot
.IAction
inTopicRule
(specifying it inTopicRuleProps
,TopicRule.addAction()
method, callingIAction.bind()
, passingactions
to theCfnTopicRule
, etc.).@aws-cdk/aws-iot-actions
module (all of the files CDK requires, likepackage.json
,NOTICE
,LICENSE
, etc.).IAction
in@aws-cdk/aws-iot-actions
(it's your choice which one you want to do first 🙂).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.
And of course, ReadMe files updates 🙂.
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.
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:
Take a look at how these are implemented in the AppMesh library, which uses this pattern often; here's a good example.
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.
OK. I will implement as
lambda.Code
andappmesh.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?
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.
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.This file was deleted.