Skip to content

feat: add workflow support for custom operation handlers#3225

Merged
Ruadhri17 merged 7 commits intothin-edge:mainfrom
Ruadhri17:custom-operation-child-device
Nov 27, 2024
Merged

feat: add workflow support for custom operation handlers#3225
Ruadhri17 merged 7 commits intothin-edge:mainfrom
Ruadhri17:custom-operation-child-device

Conversation

@Ruadhri17
Copy link
Copy Markdown
Contributor

@Ruadhri17 Ruadhri17 commented Nov 5, 2024

Proposed changes

TODO:

  • thin-edge command conversion
  • subscribe to command topics for custom operation
  • state change handler
  • main device system test
  • child device system test
  • fix bug with externalID
  • fix codecov (maybe)

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
  • Documentation Update (if none of the other choices apply)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue


Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA (in all commits with git commit -s)
  • I ran cargo fmt as mentioned in CODING_GUIDELINES
  • I used cargo clippy as mentioned in CODING_GUIDELINES
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

@Ruadhri17 Ruadhri17 force-pushed the custom-operation-child-device branch from be44882 to a5a2462 Compare November 5, 2024 21:50
@reubenmiller reubenmiller added the theme:workflows Theme: Workflow engine topics label Nov 6, 2024
@Ruadhri17 Ruadhri17 force-pushed the custom-operation-child-device branch from a5a2462 to baf5a5a Compare November 7, 2024 00:08
@Ruadhri17 Ruadhri17 changed the title feat: add workflow support for child devices feat: add workflow support for custom operation (drop 2) Nov 12, 2024
@Ruadhri17 Ruadhri17 force-pushed the custom-operation-child-device branch from b954d2d to 4624286 Compare November 12, 2024 18:34
@Ruadhri17 Ruadhri17 force-pushed the custom-operation-child-device branch from 4624286 to 615a9e3 Compare November 12, 2024 21:38
@Ruadhri17 Ruadhri17 temporarily deployed to Test Pull Request November 12, 2024 21:38 — with GitHub Actions Inactive
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Nov 12, 2024

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
527 0 2 527 100 1h32m29.931503999s

@Ruadhri17 Ruadhri17 force-pushed the custom-operation-child-device branch from 615a9e3 to bfaa518 Compare November 12, 2024 22:47
@Ruadhri17 Ruadhri17 force-pushed the custom-operation-child-device branch from bfaa518 to bf28c6b Compare November 13, 2024 00:17
@Ruadhri17 Ruadhri17 temporarily deployed to Test Pull Request November 13, 2024 00:17 — with GitHub Actions Inactive
@Ruadhri17 Ruadhri17 marked this pull request as ready for review November 13, 2024 00:19
@Bravo555 Bravo555 self-assigned this Nov 13, 2024
Copy link
Copy Markdown
Member

@rina23q rina23q left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit more work required

@rina23q rina23q dismissed their stale review November 19, 2024 15:24

I don't want to block merging while I'm away

@Bravo555 Bravo555 self-assigned this Nov 19, 2024
.command_name()
.is_some_and(|command_name| command_name.eq(command))
})
.find_map(|template| template.on_fragment())
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

@reubenmiller reubenmiller Nov 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@albinsuresh albinsuresh Nov 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Copy Markdown
Contributor

@albinsuresh albinsuresh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@Bravo555 Bravo555 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Bravo555 Bravo555 removed their assignment Nov 21, 2024
@Ruadhri17 Ruadhri17 force-pushed the custom-operation-child-device branch from a531772 to 9207da0 Compare November 25, 2024 15:14
Copy link
Copy Markdown
Contributor

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. Thank you for taking the time to address all the comments.

Copy link
Copy Markdown
Contributor

@albinsuresh albinsuresh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Ruadhri17 and others added 7 commits November 27, 2024 09:57
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

theme:workflows Theme: Workflow engine topics

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants