feat(iotevents): support actions#18869
Conversation
|
|
d665d82 to
61a4bae
Compare
skinny85
left a comment
There was a problem hiding this comment.
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*
skinny85
left a comment
There was a problem hiding this comment.
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 🙂.
| 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.
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.
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.
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.
Also, add a unit test for it, and we'll find out 🙂.
There was a problem hiding this comment.
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.
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! |
skinny85
left a comment
There was a problem hiding this comment.
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.
When does this return undefined?
There was a problem hiding this comment.
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.
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.
|
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.  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