Limit dynamic operation dir APIs to main device #2409 by albinsuresh · Pull Request #2614 · thin-edge/thin-edge.io

albinsuresh · 2024-01-23T06:22:18Z

Proposed changes

Add operations support for nested child devices by keeping their supported operations in the same /etc/tedge/operations/c8y directory where the main device and immediate child devices keep their operation files.
Stop interpreting all directories under the operations directory as immediate child devices, and registering them on startup, resulting in the reported bug.
Limit the dynamic update of the supported operations list via file updates to this directory only to the main device. The supported list of child devices are updated only when they declare their supported operations via MQTT.

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

#2409

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

codecov · 2024-01-23T06:32:20Z

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (00c14eb) 75.9% compared to head (0ece474) 75.8%.

Additional details and impacted files

Files	Coverage Δ
crates/extensions/c8y_mapper_ext/src/config.rs	`45.7% <100.0%> (+1.4%)`	⬆️
crates/extensions/c8y_mapper_ext/src/tests.rs	`91.8% <96.0%> (+0.1%)`	⬆️
crates/extensions/c8y_mapper_ext/src/converter.rs	`81.0% <68.7%> (-0.7%)`	⬇️
crates/extensions/c8y_mapper_ext/src/actor.rs	`80.3% <53.3%> (+1.6%)`	⬆️

... and 4 files with indirect coverage changes

github-actions · 2024-01-23T08:33:38Z

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
385	0	3	385	100	57m37.867999999s

rina23q

Looks good, however, I want to review one more round after I get more insights about handling of custom operations.

P.S. I think this PR also needs to update user guide. For example, https://thin-edge.github.io/thin-edge.io/operate/c8y/supported_operations/#adding-new-operations is stating as below:

To add a new operation to a child device, create a new file in /etc/tedge/operations/c8y/ directory as below.
sudo -u tedge touch /etc/tedge/operations/c8y//c8y_Restart
Now the new operation will be automatically added to the list of child supported operations and will be sent to the cloud.

This is no longer applicable.

tests/RobotFramework/tests/cumulocity/firmware/firmware_operation_child_device.robot

crates/extensions/c8y_mapper_ext/src/tests.rs

crates/extensions/c8y_mapper_ext/src/actor.rs

didier-wenzek · 2024-01-24T17:58:35Z

crates/extensions/c8y_mapper_ext/src/actor.rs

+                            self.mqtt_publisher
+                                .send(
+                                    self.converter
+                                        .process_operation_update_message(discovered_ops),


There is something a bit weird/overly complex/useless here.

The process_inotify_events method carefully build a DiscoverOp with the details of the operation update,
but the process_operation_update_message calls try_process_operation_update_messaDiscoverOpge which ignores all the message: DiscoverOp content except for the message.ops_dir which is always equals to the converter.ops_dir.

rina23q

Code and tests look good.

However, I think the doc is still not clear enough for the users who want to add a custom operation for child devices. I think we should mention
a. they can create a custom operation file for child devices
b. however, they need to declare its capability via MQTT message (probably link to the capability doc is enough)

albinsuresh · 2024-01-29T16:21:54Z

Code and tests look good.

However, I think the doc is still not clear enough for the users who want to add a custom operation for child devices. I think we should mention a. they can create a custom operation file for child devices b. however, they need to declare its capability via MQTT message (probably link to the capability doc is enough)

Even though we support that, I intentionally left that out from the doc as the goal is to eventually deprecate this whole file-system based API and replace it completely with workflows. So, I didn't want to highlight a feature in the 1.0 doc that'll be deprecated soon. Especially since the existing Adding new custom operation section does not explicitly mention support for child devices.

didier-wenzek

The code is doing what is expected. However, I have comments related to docs & tests.

docs/src/operate/c8y/supported_operations.md

tests/RobotFramework/tests/cumulocity/firmware/firmware_operation_child_device.robot

tests/RobotFramework/tests/cumulocity/bootstrap.robot

didier-wenzek

Approved

… operation [1] disabled dynamic operation reload for child devices because it needed to support nested child devices as well. For this reason, when receiving an MQTT command metadata message and registering that operation, we still need to read the operation directory and register and send all the new operations. [1]: thin-edge#2614 Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>

albinsuresh temporarily deployed to Test Pull Request January 23, 2024 06:29 — with GitHub Actions Inactive

albinsuresh had a problem deploying to Test Auto January 23, 2024 06:30 — with GitHub Actions Failure

albinsuresh temporarily deployed to Test Pull Request January 23, 2024 07:17 — with GitHub Actions Inactive

albinsuresh had a problem deploying to Test Auto January 23, 2024 07:54 — with GitHub Actions Failure

albinsuresh temporarily deployed to Test Pull Request January 23, 2024 16:12 — with GitHub Actions Inactive

albinsuresh had a problem deploying to Test Auto January 23, 2024 16:15 — with GitHub Actions Failure

albinsuresh force-pushed the fix/2409/limit-operations-dir-apis branch from 21d4e4c to 09b489c Compare January 24, 2024 05:16

albinsuresh temporarily deployed to Test Pull Request January 24, 2024 05:23 — with GitHub Actions Inactive

albinsuresh temporarily deployed to Test Auto January 24, 2024 05:34 — with GitHub Actions Inactive

albinsuresh marked this pull request as ready for review January 24, 2024 05:48

albinsuresh requested review from a team, didier-wenzek, jarhodes314 and rina23q as code owners January 24, 2024 05:48

rina23q reviewed Jan 24, 2024

View reviewed changes

tests/RobotFramework/tests/cumulocity/firmware/firmware_operation_child_device.robot Outdated Show resolved Hide resolved

crates/extensions/c8y_mapper_ext/src/tests.rs Show resolved Hide resolved

didier-wenzek reviewed Jan 24, 2024

View reviewed changes

albinsuresh force-pushed the fix/2409/limit-operations-dir-apis branch from 09b489c to a0e11a1 Compare January 29, 2024 13:50

albinsuresh had a problem deploying to Test Pull Request January 29, 2024 13:57 — with GitHub Actions Failure

albinsuresh force-pushed the fix/2409/limit-operations-dir-apis branch from a0e11a1 to e6890b5 Compare January 29, 2024 14:34

albinsuresh requested a review from reubenmiller as a code owner January 29, 2024 14:34

albinsuresh temporarily deployed to Test Pull Request January 29, 2024 14:41 — with GitHub Actions Inactive

albinsuresh temporarily deployed to Test Auto January 29, 2024 14:42 — with GitHub Actions Inactive

rina23q reviewed Jan 29, 2024

View reviewed changes

didier-wenzek reviewed Jan 29, 2024

View reviewed changes

docs/src/operate/c8y/supported_operations.md Outdated Show resolved Hide resolved

tests/RobotFramework/tests/cumulocity/firmware/firmware_operation_child_device.robot Show resolved Hide resolved

tests/RobotFramework/tests/cumulocity/bootstrap.robot Show resolved Hide resolved

albinsuresh had a problem deploying to Test Pull Request January 30, 2024 15:03 — with GitHub Actions Failure

albinsuresh temporarily deployed to Test Pull Request January 30, 2024 15:47 — with GitHub Actions Inactive

albinsuresh had a problem deploying to Test Auto January 30, 2024 16:16 — with GitHub Actions Failure

albinsuresh temporarily deployed to Test Pull Request January 31, 2024 09:30 — with GitHub Actions Inactive

albinsuresh had a problem deploying to Test Auto January 31, 2024 09:30 — with GitHub Actions Failure

Limit dynamic operation dir APIs to main device thin-edge#2409

0ece474

albinsuresh force-pushed the fix/2409/limit-operations-dir-apis branch from 06f1230 to 0ece474 Compare January 31, 2024 10:20

albinsuresh temporarily deployed to Test Pull Request January 31, 2024 10:28 — with GitHub Actions Inactive

albinsuresh had a problem deploying to Test Auto January 31, 2024 10:34 — with GitHub Actions Failure

albinsuresh temporarily deployed to Test Pull Request January 31, 2024 11:56 — with GitHub Actions Inactive

albinsuresh temporarily deployed to Test Auto January 31, 2024 11:56 — with GitHub Actions Inactive

didier-wenzek approved these changes Jan 31, 2024

View reviewed changes

albinsuresh added this pull request to the merge queue Jan 31, 2024

Merged via the queue into thin-edge:main with commit 9dbd1ad Jan 31, 2024

albinsuresh deleted the fix/2409/limit-operations-dir-apis branch February 2, 2024 09:36

Bravo555 mentioned this pull request Nov 15, 2024

fix: Don't fail if only some operations could not be loaded #3236

Merged

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit dynamic operation dir APIs to main device #2409#2614

Limit dynamic operation dir APIs to main device #2409#2614
albinsuresh merged 1 commit intothin-edge:mainfrom
albinsuresh:fix/2409/limit-operations-dir-apis

albinsuresh commented Jan 23, 2024 •

edited

Loading

Uh oh!

codecov bot commented Jan 23, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Jan 23, 2024 •

edited

Loading

Uh oh!

rina23q left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

didier-wenzek Jan 24, 2024

Uh oh!

rina23q left a comment

Uh oh!

albinsuresh commented Jan 29, 2024

Uh oh!

didier-wenzek left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

didier-wenzek left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

albinsuresh commented Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Types of changes

Paste Link to the issue

Checklist

Further comments

Uh oh!

codecov bot commented Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Robot Results

Uh oh!

rina23q left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

didier-wenzek Jan 24, 2024

Choose a reason for hiding this comment

Uh oh!

rina23q left a comment

Choose a reason for hiding this comment

Uh oh!

albinsuresh commented Jan 29, 2024

Uh oh!

didier-wenzek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

didier-wenzek left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

albinsuresh commented Jan 23, 2024 •

edited

Loading

codecov bot commented Jan 23, 2024 •

edited

Loading

github-actions bot commented Jan 23, 2024 •

edited

Loading

rina23q left a comment •

edited

Loading