-
Notifications
You must be signed in to change notification settings - Fork 4k
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(iotevents): support actions #18869
Conversation
d665d82
to
61a4bae
Compare
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 @yamatatsu, but we need a few more iterations before we can merge this in.
Thanks for the great contribution, as always!
Co-authored-by: Adam Ruka <adamruka85@gmail.com>
Pull request has been modified.
Co-authored-by: Adam Ruka <adamruka85@gmail.com>
Co-authored-by: Adam Ruka <adamruka85@gmail.com>
Co-authored-by: Adam Ruka <adamruka85@gmail.com>
Co-authored-by: Adam Ruka <adamruka85@gmail.com>
Co-authored-by: Adam Ruka <adamruka85@gmail.com>
Co-authored-by: Adam Ruka <adamruka85@gmail.com>
Co-authored-by: Adam Ruka <adamruka85@gmail.com>
Co-authored-by: Adam Ruka <adamruka85@gmail.com>
Co-authored-by: Adam Ruka <adamruka85@gmail.com>
Co-authored-by: Adam Ruka <adamruka85@gmail.com>
Co-authored-by: Adam Ruka <adamruka85@gmail.com>
This PR creates new module for aws-iotevents. This PR will contain implementations of Detector Model action classes. About #18869 (comment) . ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
This PR creates new module for aws-iotevents. This PR will contain implementations of Detector Model action classes. About aws#18869 (comment) . ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
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 for the contribution @yamatatsu! This is starting to take shape, but I think we need a few more iterations before we nail the final API of this 🙂.
@@ -150,6 +214,7 @@ function toTransitionEventsJson(transitionEvents: TransitionEvent[]): CfnDetecto | |||
return transitionEvents.map(transitionEvent => ({ | |||
eventName: transitionEvent.eventName, | |||
condition: transitionEvent.condition.evaluate(), | |||
actions: transitionEvent.actions?.map(action => action.renderActionConfig().configuration), |
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 should not be calling renderActionConfig()
(soon called bind()
) more than once, because it can have side effects, like creating Constructs.
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.
This code will call bind()
once by actions by calling DetectorModel constructor. But if a action is used in two detector models, bind()
of the action will be called twice.
Is it problem?
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.
Nope! That's exactly how it's supposed to work.
Notice that the second detector model will call bind()
with different arguments than the first one.
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.
Also, add a unit test for it, and we'll find out 🙂.
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 added the test! Calling bind twice worked without errors.
Ah.. But if an action set twice to an detector model and the action grant the role something, probably same policy will be added to the role. Is it better that we control it? 🤔
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.
Nope! Just let IAM de-duplication handle it.
packages/@aws-cdk/aws-iotevents-actions/lib/lambda-invoke-action.ts
Outdated
Show resolved
Hide resolved
I'll address the comments and revert something to keep the difference small... Sorry! |
No worries @yamatatsu, thanks for all of your hard work on this! |
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 @yamatatsu! 2 minor comments, and we'll get this merged.
scope: Construct, | ||
actionBindOptions: ActionBindOptions, | ||
events: Event[], | ||
): CfnDetectorModel.EventProperty[] | undefined { |
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.
When does this return undefined
?
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.
Ah.. It is a remnant of big refactoring..
}); | ||
// 2st => 1st | ||
offlineState.transitionTo(onlineState, { | ||
when: iotevents.Expression.eq( | ||
iotevents.Expression.inputAttribute(input, 'payload.temperature'), |
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.
Can we just leave this test unchanged? We already have an integ test in the @aws-cdk/aws-iotevents-actions
module, I don't think we need this too.
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 @yamatatsu!
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 |
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 allow IoT Events detector model to perform actions as [this documentation](https://docs.aws.amazon.com/iotevents/latest/developerguide/iotevents-supported-actions.html). This PR is in roadmap of aws#17711. ![スクリーンショット 2022-02-08 22 43 33](https://user-images.githubusercontent.com/11013683/152999288-81721f15-fefb-4108-b34b-aab3f88a7ab8.png) With this fix, all the interfaces of the DetectorModel are now implemented! And next works is implementing expressions and actions. The exapmle in readme became not simple, so also this PR has sorted explanation of readme. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
This PR allow IoT Events detector model to perform actions as this documentation.
This PR is in roadmap of #17711.
With this fix, all the interfaces of the DetectorModel are now implemented! And next works is implementing expressions and actions.
The exapmle in readme became not simple, so also this PR has sorted explanation of readme.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license