feat(iotevents): allow setting description, evaluation method and key of DetectorModel#18644
Conversation
…`key` of DetectorModel
|
|
skinny85
left a comment
There was a problem hiding this comment.
Thanks @yamatatsu! Minor notes, mainly about naming.
| const input = new iotevents.Input(this, 'MyInput', { | ||
| inputName: 'my_input', // optional | ||
| attributeJsonPaths: ['payload.temperature'], | ||
| attributeJsonPaths: ['payload.deviceId', 'payload.temperature'], |
There was a problem hiding this comment.
Why this change - seems unrelated to the rest of this PR...?
There was a problem hiding this comment.
I modified this for the sake of reality.
The path passed in detectorKey must be declared in this attributeJsonPaths. And the value passed in detectorKey is often the unique value of the device or device group.
This is because IoT Events is often used to monitor devices and device groups.
I modified integ test for the same reason.
| /** | ||
| * Information about the order in which events are evaluated and how actions are executed. | ||
| */ | ||
| export enum EvaluationMethod { |
There was a problem hiding this comment.
Is this name too generic? Should this be EventEvaluationMethod, or StateEvaluationMethod, or DetectorActionEvaluationMethod, or something?
Actually, can we even kill the "Method" suffix here? I don't think it adds much, and just makes the name longer.
There was a problem hiding this comment.
EventEvaluation is good I think!
| * | ||
| * @default - none (single detector instance will be created and all inputs will be routed to it) | ||
| */ | ||
| readonly key?: string; |
There was a problem hiding this comment.
This name, however, is way, way too generic. How about calling it detectorKey, or something like that?
There was a problem hiding this comment.
detectorKey is good! Hmm... I should think for more understandability instead of CFn copy. Thank you!
| Template.fromStack(stack).hasResourceProperties('AWS::IoTEvents::DetectorModel', { | ||
| Key: 'payload.deviceId', | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Let's just do all 3 in one test. Spreading them out like that just makes the tests longer, it doesn't really improve coverage in any way.
| const input = new iotevents.Input(this, 'MyInput', { | ||
| inputName: 'test_input', | ||
| attributeJsonPaths: ['payload.temperature'], | ||
| attributeJsonPaths: ['payload.deviceId', 'payload.temperature'], |
There was a problem hiding this comment.
Again, why are we changing existing things?
| detectorModelName: 'test-detector-model', | ||
| detectorModelDescription: 'test-detector-model-description', | ||
| evaluationMethod: iotevents.EvaluationMethod.SERIAL, | ||
| key: 'payload.deviceId', |
There was a problem hiding this comment.
Can't we use payload.temperature here?
Pull request has been modified.
description, evaluationMethod and key of DetectorModel
skinny85
left a comment
There was a problem hiding this comment.
Looks great @yamatatsu - short and sweet, just the way I like it 😉.
|
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 is about #17711 (but out of the roadmap).
This PR (especially
keyproperty) make it easier to test the features we will implement.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license