feat: Trigger tedge multicall on symlinks as well as with an explicit subcommand parameter by didier-wenzek · Pull Request #3311 · thin-edge/thin-edge.io

didier-wenzek · 2024-12-31T17:46:40Z

Proposed changes

~~remove main.rs for the agent and the mapper, and all the plugins that can be launched using a symlink on tedge~~
- This has already been done by d16f09b
- With the exception of tegde_write, which makes sense as tegde_write is used in combination with sudo.
trigger tedge multicall on symlinks as well as with an explicit subcommand parameters

The following are now equivalent:

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

The former tedge cli can be invoked as before or with the new sub-command approach:

tedge cert show
tedge cli cert show
tedge-cli cert show where tedge-cli is a copy or a symlink of tedge

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

The following are also accepted (as a side effect of using aliases for the sub-command names):

tedge tedge-cli cert show
cli cert show where cli is a copy or a symlink of tedge
tedge tedge-mapper c8y
mapper c8y where mapper is a copy or a symlink of tedge

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 or with the new sub-command approach: - `tedge cert show` - `tedge cli cert show` - `tedge-cli cert show` where `tedge-cli` is a copy or a symlink of `tedge` Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

codecov · 2024-12-31T17:57:30Z

Codecov Report

Attention: Patch coverage is 47.61905% with 11 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
crates/core/tedge/src/main.rs	50.00%	10 Missing ⚠️
crates/core/tedge/src/cli/init.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

📢 Thoughts on this report? Let us know!

didier-wenzek · 2024-12-31T17:59:27Z

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

+    #[clap(alias = "cli")]
+    TedgeCli(CliOpt),
+
+    #[clap(alias = "mapper")]


The following would improve tedge --help using the short names without the tedge- prefix, which is more convenient on the command line (but might misguide users as the prefix is desirable for tedge subcommand symlinks).

Suggested change

#[clap(alias = "mapper")]

#[clap(name = "mapper", alias = "tedge-mapper")]

The following would improve tedge --help using the short names without the tedge- prefix

Actually I'm seeing the exact opposite when I use the help:

root@666b6db5f6ae:/setup# tedge mapper --help tedge-mapper is the mapper that translates thin-edge.io data model to c8y/az data model. Usage: tedge tedge-mapper [OPTIONS] <COMMAND>

As you can see, the subcommand also has the tedge- prefix.

Maybe my comment was not clear enough.

By applying the above suggestion (i.e. name = "mapper", alias = "tedge-mapper"), the help will be:

$ tedge mapper help tedge-mapper is the mapper that translates thin-edge.io data model to c8y/az data model. Usage: tedge mapper [OPTIONS] <COMMAND>

didier-wenzek · 2024-12-31T18:00:49Z

crates/core/tedge/tests/main.rs


        let mut get_device_id_cmd =
-            tedge_command_with_test_home(["--config-dir", home_dir, "config", "get", "device.id"])?;
+            tedge_command_with_test_home(["config", "get", "device.id", "--config-dir", home_dir])?;


I have for now no nice solution to accept the old syntax with --config-dir set before the subcommand.

See clap::Arg::global

I'm actually confused with that doc. The following statement: "In effect, this means one should define all global arguments at the top level, however it doesn’t matter where the user uses the global argument." seems to indicate that the options marked as global can be used anywhere, which is exactly the old behaviour that we want. So, why isn't that working I wonder.

github-actions · 2024-12-31T18:07:45Z

Robot Results

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

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

albinsuresh

If using the global parameter isn't working as desired, I still wonder if reverting one change that removed the nested Tedge struct would restore the old behaviour.

albinsuresh · 2025-01-03T06:54:30Z

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

-    Tedge {
-        #[clap(subcommand)]
-        cmd: TEdgeOpt,
+    #[clap(alias = "cli")]
+    TedgeCli(CliOpt),

-        #[command(flatten)]
-        common: CommonArgs,
-    },
-
-    #[clap(flatten)]
-    Component(Component),
-}


Removing this additional nested level is what caused the global argument ordering issue, right? Was that necessary? Couldn't we have achieved the same by retaining this "flattened" Component struct here, with CliOpt being the new enum variant in it?

Removing this additional nested level is what caused the global argument ordering issue, right?

No, the behavior is exactly the same with two enum levels, the inner being flatten.

However, having two levels helps when the first one is a struct:

pub struct TEdgeOptMulticall { #[command(flatten)] pub common: CommonArgs, #[clap(subcommand)] pub component: TEdgeComponent, } #[derive(clap::Parser, Debug)] pub enum TEdgeComponent { #[clap(flatten)] Tedge(TEdgeOpt), #[clap(alias = "mapper")] TedgeMapper(MapperOpt), // .. }

This requires some other minor changes (to move out the common args from TedgeOpt, MapperOpt ... to EdgeOptMulticall), but then all the following variants are accepted:

tedge mapper c8y

tedge --config-dir /tmp mapper c8y

tedge mapper --config-dir /tmp c8y

tedge mapper c8y --config-dir /tmp

Unfortunately this not working when used à la busybox:

$ cp tedge tedge-mapper $ tedge-mapper c8y thread 'main' panicked at /home/didier/.cargo/registry/src/index.crates.io-6f17d22bba15001f/clap_builder-4.4.18/src/builder/debug_asserts.rs:60:9: Command : Arguments like config_dir cannot be set on a multicall command

The error is really surprising: the config_dir argument is rejected even if not provided.

Unfortunately this not working when used à la busybox

Indeed not supported yet: clap-rs/clap#4521

The error is really surprising: the config_dir argument is rejected even if not provided.

Even if not directly related, this comment clap-rs/clap#5525 (comment) explains why this is rejected at runtime and not compile time: "The challenge with #[command(flatten, from_global)] is that proc macros can't talk to each other at compile time. We'd have to implement this at runtime through the code we generate."

albinsuresh · 2025-01-03T06:57:40Z

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

+    #[clap(subcommand)]
+    pub cmd: TEdgeOpt,
+
+    #[command(flatten)]


I thought you were gonna mark this as global to get around the common argument ordering issue as the clap doc describes: "In effect, this means one should define all global arguments at the top level, however it doesn’t matter where the user uses the global argument"

This is not necessary and even rejected by clap, global being set for all the arguments of the flatten struct.

albinsuresh · 2025-01-03T07:28:37Z

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

+    #[clap(alias = "cli")]
+    TedgeCli(CliOpt),
+
+    #[clap(alias = "mapper")]


The following would improve tedge --help using the short names without the tedge- prefix

Actually I'm seeing the exact opposite when I use the help:

root@666b6db5f6ae:/setup# tedge mapper --help tedge-mapper is the mapper that translates thin-edge.io data model to c8y/az data model. Usage: tedge tedge-mapper [OPTIONS] <COMMAND>

As you can see, the subcommand also has the tedge- prefix.

albinsuresh · 2025-01-03T07:35:19Z

crates/core/tedge/tests/main.rs


        let mut get_device_id_cmd =
-            tedge_command_with_test_home(["--config-dir", home_dir, "config", "get", "device.id"])?;
+            tedge_command_with_test_home(["config", "get", "device.id", "--config-dir", home_dir])?;


I'm actually confused with that doc. The following statement: "In effect, this means one should define all global arguments at the top level, however it doesn’t matter where the user uses the global argument." seems to indicate that the options marked as global can be used anywhere, which is exactly the old behaviour that we want. So, why isn't that working I wonder.

didier-wenzek · 2025-01-03T16:38:21Z

This PR is a dead end and has been superseded by #3313

didier-wenzek added theme:installation Theme: Installation topics theme:cli Theme: cli related topics labels Dec 31, 2024

didier-wenzek requested review from albinsuresh, jarhodes314 and rina23q as code owners December 31, 2024 17:46

didier-wenzek temporarily deployed to Test Pull Request December 31, 2024 17:46 — with GitHub Actions Inactive

didier-wenzek had a problem deploying to Test Auto December 31, 2024 17:52 — with GitHub Actions Failure

fixup! Trigger tedge multicall on symlinks as well as explicitly

04531b1

didier-wenzek commented Dec 31, 2024

View reviewed changes

fixup! fixup! Trigger tedge multicall on symlinks as well as explicitly

4e0fcd7

didier-wenzek requested a review from a team as a code owner December 31, 2024 18:11

didier-wenzek temporarily deployed to Test Pull Request December 31, 2024 18:11 — with GitHub Actions Inactive

didier-wenzek temporarily deployed to Test Auto December 31, 2024 18:17 — with GitHub Actions Inactive

Fix typos in comments

a23796e

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

didier-wenzek temporarily deployed to Test Pull Request January 2, 2025 14:25 — with GitHub Actions Inactive

didier-wenzek temporarily deployed to Test Auto January 2, 2025 14:31 — with GitHub Actions Inactive

rina23q mentioned this pull request Jan 2, 2025

fix: Fix typos #3312

Merged

11 tasks

albinsuresh reviewed Jan 3, 2025

View reviewed changes

didier-wenzek mentioned this pull request Jan 3, 2025

feat: Trigger tedge multicall with symlinks as well as sub-commands #3313

Merged

11 tasks

didier-wenzek closed this Jan 3, 2025

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

	#[clap(alias = "mapper")]
	#[clap(name = "mapper", alias = "tedge-mapper")]

Conversation

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

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Robot Results

Uh oh!

albinsuresh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

didier-wenzek Jan 3, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

didier-wenzek commented Jan 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

didier-wenzek commented Dec 31, 2024 •

edited

Loading

codecov bot commented Dec 31, 2024 •

edited

Loading

github-actions bot commented Dec 31, 2024 •

edited

Loading

didier-wenzek Jan 3, 2025 •

edited

Loading