Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

albinsuresh
Copy link
Contributor

@albinsuresh albinsuresh commented Jan 23, 2024

Proposed changes

  1. 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.
  2. Stop interpreting all directories under the operations directory as immediate child devices, and registering them on startup, resulting in the reported bug.
  3. 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

Copy link

codecov bot commented Jan 23, 2024

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

Copy link
Contributor

github-actions bot commented Jan 23, 2024

Robot Results

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

Copy link
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.

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.

crates/extensions/c8y_mapper_ext/src/actor.rs Outdated Show resolved Hide resolved
self.mqtt_publisher
.send(
self.converter
.process_operation_update_message(discovered_ops),
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
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.

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
Copy link
Contributor Author

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.

Copy link
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.

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

Copy link
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

@albinsuresh 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
20 checks passed
@albinsuresh albinsuresh deleted the fix/2409/limit-operations-dir-apis branch February 2, 2024 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants