feat: Trigger tedge multicall with symlinks as well as sub-commands by didier-wenzek · Pull Request #3313 · thin-edge/thin-edge.io

didier-wenzek · 2025-01-03T16:36:19Z

Proposed changes

Simplify the way tedge-mapper, tedge-agent and tedge-write are launched: it's no more mandatory to create symlinks to tedge with appropriate names.

The following are now equivalent (for the mapper, but also the agent and all tedge services and plugins ):

tedge run tedge-mapper c8y
tedge-mapper c8y where tedge-mapper is a copy or a symlink of tedge

The former tedge cli can be invoked as before:

$ tedge help
tedge is the cli tool for thin-edge.io

Usage: tedge [OPTIONS] <COMMAND>

Commands:
  init             Initialize Thin Edge
  cert             Create and manage device certificate
  ...
  run              Run thin-edge services and plugins
  ...

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

#2696

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

This PR supersedes #3311

codecov · 2025-01-03T16:49:54Z

Codecov Report

Attention: Patch coverage is 72.91667% with 13 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
crates/core/tedge/src/main.rs	76.08%	8 Missing and 3 partials ⚠️
crates/core/tedge/src/cli/mod.rs	0.00%	1 Missing ⚠️
plugins/tedge_apt_plugin/src/lib.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

📢 Thoughts on this report? Let us know!

github-actions · 2025-01-03T16:57:34Z

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
551	0	2	551	100	1h27m53.879757s

didier-wenzek · 2025-01-03T16:57:55Z

crates/core/tedge/src/main.rs

+fn redirect_if_multicall(cmd: TEdgeOpt, common: CommonArgs) -> TEdgeOptMulticall {
+    match cmd {
+        TEdgeOpt::Mapper(opt) => {
+            TEdgeOptMulticall::Component(Component::TedgeMapper(opt.with_common_args(common)))


We have to be more cautious here: there is a risk of confusion as --tedge-config is provided twice to clap.

I assume you mean --config-dir

When a given global argument (as --config-dir) is used twice by the command struct (as here where common can be accessed directly (common) or via the mapper options (opt.common), clap correctly duplicate the user provided info to all the inner components.

Hence, there is no need to explicitly copy to common arg.

As this clap behavior is not documented, tests have been added, see b735319

There is one caveat: clap fails to notice that an argument is provided twice.

crates/core/tedge/src/cli/mod.rs

reubenmiller · 2025-01-06T13:25:39Z

@didier-wenzek Overall the changes look very nice, though I'm just wondering whether the tedge subcommands are being polluted too much by some of the more internal(ish) commands like tedge write...If we have to add a tedge read in the future, then will this just cause unnecessary noise for the user when they look at the --help command?

Do we need to put some of the commands under a some subcommand like utils (though the name is not perfect):

tedge utils write

reubenmiller · 2025-01-07T08:45:49Z

@didier-wenzek I think it would be useful if we make all of the binaries which can be access via symlinks also available via subcommands, but since there are a few tricker cases (e.g. c8y-firmware-plugin, c8y-remote-access-plugin), so here is the suggested structure (and the associated symlink equivalent in brackets).

tedge
- agent (tedge-agent)
- mapper (tedge-mapper)
- watchdog (tedge-watchdog)
- sm-plugins
  - apt (tedge-apt-plugin)
- plugins
  - c8y-remote-access-plugin (c8y-remote-access-plugin)
  - c8y-firmware-plugin (c8y-firmware-plugin)
- utils
  - write (tedge-write)

Alternatively, if we could just do a flat structure and just place all of the binary names under one subcommand:

tedge
- run
  - tedge-agent
  - tedge-mapper
  - tedge-watchdog
  - tedge-apt-plugin
  - c8y-remote-access-plugin
  - c8y-firmware-plugin
  - tedge-write

didier-wenzek · 2025-01-07T09:21:35Z

@didier-wenzek I think it would be useful if we make all of the binaries which can be access via symlinks also available via subcommands, but since there are a few tricker cases (e.g. c8y-firmware-plugin, c8y-remote-access-plugin), so here is the suggested structure (and the associated symlink equivalent in brackets).
* tedge
  
  * agent (tedge-agent)
  * mapper (tedge-mapper)
  * watchdog (tedge-watchdog)
  * sm-plugins
    
    * apt (tedge-apt-plugin)
  * plugins
    
    * c8y-remote-access-plugin (c8y-remote-access-plugin)
    * c8y-firmware-plugin (c8y-firmware-plugin)
  * utils
    
    * write (tedge-write)
Alternatively, if we could just do a flat structure and just place all of the binary names under one subcommand:
* tedge
  
  * run
    
    * tedge-agent
    * tedge-mapper
    * tedge-watchdog
    * tedge-apt-plugin
    * c8y-remote-access-plugin
    * c8y-firmware-plugin
    * tedge-write

The second approach with tedge run xxx is simpler. I will go with that one.

didier-wenzek · 2025-01-07T11:13:20Z

The second approach with tedge run xxx is simpler. I will go with that one.

Done with 2bada64
the docs being updated by 2c73956

reubenmiller

Just two minor points:

tedge-apt-plugin is missing under tedge run
Can we sort the subcommands under tedge run? I don't mind having help at the end, however the current order seems to be manually defined.

didier-wenzek · 2025-01-08T10:44:28Z

1. `tedge-apt-plugin` is missing under `tedge run`

For some reasons I have to understand better, tedge-apt-plugin is a special case: https://github.com/thin-edge/thin-edge.io/blob/main/crates/core/tedge/src/main.rs#L29

2. Can we sort the subcommands under `tedge run`? I don't mind having `help` at the end, however the current order seems to be manually defined.

Indeed, these sub-commands are listed here: https://github.com/thin-edge/thin-edge.io/blob/main/crates/core/tedge/src/cli/mod.rs#L52

Any order in mind?

reubenmiller · 2025-01-08T10:59:02Z

1. `tedge-apt-plugin` is missing under `tedge run`
For some reasons I have to understand better, tedge-apt-plugin is a special case: https://github.com/thin-edge/thin-edge.io/blob/main/crates/core/tedge/src/main.rs#L29
2. Can we sort the subcommands under `tedge run`? I don't mind having `help` at the end, however the current order seems to be manually defined.
Indeed, these sub-commands are listed here: https://github.com/thin-edge/thin-edge.io/blob/main/crates/core/tedge/src/cli/mod.rs#L52

Any order in mind?

alphabetical is fine

didier-wenzek · 2025-01-08T11:11:53Z

1. `tedge-apt-plugin` is missing under `tedge run`
For some reasons I have to understand better, tedge-apt-plugin is a special case: https://github.com/thin-edge/thin-edge.io/blob/main/crates/core/tedge/src/main.rs#L29

This is to exit with a status code of 1 and not 2 when the command line cannot be parsed:
https://github.com/thin-edge/thin-edge.io/blob/main/plugins/tedge_apt_plugin/src/lib.rs#L354

I will handle that case

Bravo555

Nice improvement, I won't have to manually create dummy binary files for different components to do cargo run anymore!

That said, looking at tedge run --help there are some inconsistencies that could be fixed:

Run thin-edge services and plugins

Usage: tedge run [OPTIONS] <COMMAND>

Commands:
  tedge-mapper              tedge-mapper is the mapper that translates thin-edge.io data model to c8y/az data model.
  tedge-agent               tedge-agent interacts with a Cloud Mapper and one or more Software Plugins
  c8y-firmware-plugin       Thin-edge device firmware management for Cumulocity
  tedge-watchdog            tedge-watchdog checks the health of all the thin-edge.io components/services.
  c8y-remote-access-plugin  Thin-edge.io plugin for the Cumulocity IoT's Cloud Remote Access feature
  tedge-write               tee-like helper for writing to files which `tedge` user does not have write permissions to
  help                      Print this message or the help of the given subcommand(s)

snip...

tedge-mapper mentions c8y/az, but not aws
the style is inconsistent (capitalization, if the sentence is terminated by a .)

These descriptions are taken from package.description field of Cargo.toml of these crates, so they should either be fixed there, or we could provide the descriptions directly in code by adding doc comments to the Component enum variants; however I think updating Cargo.tomls would be preferable.

Doing this however would be best in another PR, so approving this one.

didier-wenzek · 2025-01-08T13:59:14Z

Just two minor points:

1. `tedge-apt-plugin` is missing under `tedge run`

2. Can we sort the subcommands under `tedge run`? I don't mind having `help` at the end, however the current order seems to be manually defined.

Addressed by 1bdac43

crates/core/tedge/src/main.rs

reubenmiller

Approved

The following are now equivalent: - `tedge mapper c8y` - `tedge-mapper c8y` where `tedge-mapper` is a copy or a symlink of `tedge` The former `tedge` cli can be invoked as before: ``` $ tedge help CLI arguments that should be handled by all thin-edge components Usage: tedge [OPTIONS] <COMMAND> Commands: init Initialize Thin Edge cert Create and manage device certificate ... mapper Launch a cloud mapper agent Run the agent write Write standard input to a target file ... ``` Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

Since the introduction of common arguments, the header of `tedge help` was wrongly taken by clap from the `CommonArgs` doc comments (i.e. "CLI arguments that should be handled by all thin-edge components") Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

This allows running the tedge binary via `cargo run` without having to specify `--bin tedge`

tedge-apt-plugin specific treatment in tedge/main.rs was only for exiting with 1 and not 2 when the command line cannot be parsed. Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

didier-wenzek requested review from albinsuresh, jarhodes314 and rina23q as code owners January 3, 2025 16:36

didier-wenzek temporarily deployed to Test Pull Request January 3, 2025 16:36 — with GitHub Actions Inactive

didier-wenzek mentioned this pull request Jan 3, 2025

feat: Trigger tedge multicall on symlinks as well as with an explicit subcommand parameter #3311

Closed

13 tasks

didier-wenzek added theme:installation Theme: Installation topics theme:cli Theme: cli related topics labels Jan 3, 2025

didier-wenzek temporarily deployed to Test Auto January 3, 2025 16:42 — with GitHub Actions Inactive

didier-wenzek temporarily deployed to Test Pull Request January 3, 2025 16:52 — with GitHub Actions Inactive

didier-wenzek temporarily deployed to Test Auto January 3, 2025 16:58 — with GitHub Actions Inactive

didier-wenzek commented Jan 3, 2025

View reviewed changes

didier-wenzek temporarily deployed to Test Pull Request January 6, 2025 16:19 — with GitHub Actions Inactive

didier-wenzek temporarily deployed to Test Auto January 6, 2025 16:26 — with GitHub Actions Inactive

didier-wenzek temporarily deployed to Test Pull Request January 7, 2025 11:11 — with GitHub Actions Inactive

didier-wenzek temporarily deployed to Test Auto January 7, 2025 11:17 — with GitHub Actions Inactive

reubenmiller requested changes Jan 8, 2025

View reviewed changes

reubenmiller temporarily deployed to Test Pull Request January 8, 2025 11:15 — with GitHub Actions Inactive

reubenmiller temporarily deployed to Test Auto January 8, 2025 12:14 — with GitHub Actions Inactive

Bravo555 approved these changes Jan 8, 2025

View reviewed changes

Bravo555 mentioned this pull request Jan 8, 2025

fix: Update component crates descriptions #3317

Merged

11 tasks

didier-wenzek temporarily deployed to Test Pull Request January 8, 2025 13:48 — with GitHub Actions Inactive

didier-wenzek temporarily deployed to Test Auto January 8, 2025 13:54 — with GitHub Actions Inactive

reubenmiller reviewed Jan 8, 2025

View reviewed changes

crates/core/tedge/src/main.rs Show resolved Hide resolved

reubenmiller approved these changes Jan 8, 2025

View reviewed changes

didier-wenzek and others added 5 commits January 8, 2025 16:59

Fix tedge help header

a272b60

Since the introduction of common arguments, the header of `tedge help` was wrongly taken by clap from the `CommonArgs` doc comments (i.e. "CLI arguments that should be handled by all thin-edge components") Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

Update docs for tedge cli

468c943

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

set cargo default-run to the tedge binary

db8c920

This allows running the tedge binary via `cargo run` without having to specify `--bin tedge`

Align tedge-apt-plugin with tedge services/plugins

c80b3d0

tedge-apt-plugin specific treatment in tedge/main.rs was only for exiting with 1 and not 2 when the command line cannot be parsed. Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

didier-wenzek force-pushed the feat/simplify-tedge-multicall branch from 1bdac43 to c80b3d0 Compare January 8, 2025 16:13

didier-wenzek temporarily deployed to Test Pull Request January 8, 2025 16:13 — with GitHub Actions Inactive

didier-wenzek enabled auto-merge January 8, 2025 16:15

didier-wenzek temporarily deployed to Test Auto January 8, 2025 16:20 — with GitHub Actions Inactive

didier-wenzek added this pull request to the merge queue Jan 8, 2025

Merged via the queue into thin-edge:main with commit bd06fcb Jan 8, 2025

didier-wenzek deleted the feat/simplify-tedge-multicall branch January 8, 2025 16:58

Conversation

didier-wenzek commented Jan 3, 2025 • 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 3, 2025 • 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 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Robot Results

Uh oh!

didier-wenzek Jan 3, 2025

Choose a reason for hiding this comment

Uh oh!

reubenmiller Jan 6, 2025

Choose a reason for hiding this comment

Uh oh!

didier-wenzek Jan 6, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

reubenmiller commented Jan 6, 2025

Uh oh!

reubenmiller commented Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

didier-wenzek commented Jan 7, 2025

Uh oh!

didier-wenzek commented Jan 7, 2025

Uh oh!

reubenmiller left a comment

Choose a reason for hiding this comment

Uh oh!

didier-wenzek commented Jan 8, 2025

Uh oh!

reubenmiller commented Jan 8, 2025

Uh oh!

didier-wenzek commented Jan 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bravo555 left a comment

Choose a reason for hiding this comment

Uh oh!

didier-wenzek commented Jan 8, 2025

Uh oh!

Uh oh!

reubenmiller left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

didier-wenzek commented Jan 3, 2025 •

edited

Loading

codecov bot commented Jan 3, 2025 •

edited

Loading

github-actions bot commented Jan 3, 2025 •

edited

Loading

reubenmiller commented Jan 7, 2025 •

edited

Loading

didier-wenzek commented Jan 8, 2025 •

edited

Loading