feat: add workflow support for custom operation handlers#3225
feat: add workflow support for custom operation handlers#3225Ruadhri17 merged 7 commits intothin-edge:mainfrom
Conversation
be44882 to
a5a2462
Compare
a5a2462 to
baf5a5a
Compare
b954d2d to
4624286
Compare
4624286 to
615a9e3
Compare
Robot Results
|
615a9e3 to
bfaa518
Compare
bfaa518 to
bf28c6b
Compare
I don't want to block merging while I'm away
| .command_name() | ||
| .is_some_and(|command_name| command_name.eq(command)) | ||
| }) | ||
| .find_map(|template| template.on_fragment()) |
There was a problem hiding this comment.
This makes me wonder if the on_fragment filed is redundant, when the name of the operation file should have been be the same. Are there any cases in C8y where the operation name and this fragment name could be different?
There was a problem hiding this comment.
I wouldn't assume that the fragment and the name of the operation are the same. These are custom operations which means that the customer is free to configure them however they want, so being more explicit would be safer here with the downside of a bit of boilerplate.
There was a problem hiding this comment.
Okay. It would have been better if we could drop that additional level of indirection with the name and then the fragment name. Because that design enables users to define multiple operation files with the same on_fragment value in them. But, when the actual c8y operation is triggered, only one of them can be used, as we can't trigger multiple tedge commands in response to a single c8y operation.
But, if it is still desired, we need to add some additional validations to prevent conflicting operation files with different names, but with the same on_fragment value. Current behaviour is that it picks the first operation file in the alphabetic order. It'd be better to add some additional logs to highlight to the user as to what is picked and the others are ignored.
There was a problem hiding this comment.
@Ruadhri17 if you're also wondering why any user would want to declare one name in the supported operations list (the operation file name) and use an entirely different name for his operation fragment, here is the use-case: The declared supported operation name is typically used to enable/disable UI components in C8y. The developer of that UI component dictates the expected operation name. And it is a possibility that the custom operation developer may not be able to (or don't want to) reuse the same name for his operation as well. In such cases, this design would help.
328da68 to
a079897
Compare
a079897 to
6297e58
Compare
.../tests/cumulocity/custom_operation/custom_operation_workflow/custom_operation_workflow.robot
Show resolved
Hide resolved
.../tests/cumulocity/custom_operation/custom_operation_workflow/custom_operation_workflow.robot
Outdated
Show resolved
Hide resolved
albinsuresh
left a comment
There was a problem hiding this comment.
Happy to approve once #3225 (comment) is also resolved. If that on_fragment key is redundant (always same as the operation name itself), we can get rid of it.
Bravo555
left a comment
There was a problem hiding this comment.
https://github.com/thin-edge/thin-edge.io/pull/3225/files#r1851457754
Thanks to Albin's comment I now know that the checks are indeed correct, so I don't have any other blockers.
The code and the test suite could use some more documentation comments, to make it easier why we do the things we do without retracing the original tickets, but I don't want to block this PR anymore.
a531772 to
9207da0
Compare
didier-wenzek
left a comment
There was a problem hiding this comment.
Approved. Thank you for taking the time to address all the comments.
Signed-off-by: Krzysztof Piotrowski <krzysztof.piotrowski@inetum.com>
Signed-off-by: Krzysztof Piotrowski <krzysztof.piotrowski@inetum.com>
Signed-off-by: Krzysztof Piotrowski <krzysztof.piotrowski@inetum.com>
Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>
Signed-off-by: Krzysztof Piotrowski <krzysztof.piotrowski@inetum.com>
Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>
Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>
Proposed changes
TODO:
externalIDTypes of changes
Paste Link to the issue
Checklist
cargo fmtas mentioned in CODING_GUIDELINEScargo clippyas mentioned in CODING_GUIDELINESFurther comments