Add support for commands with multiple arguments in custom operations #2602

Ruadhri17 · 2024-01-18T20:44:18Z

Proposed changes

This PR adds support for arguments/subcommands when running command in custom operation. Command accepts string with arguments separated by whitespaces, e.g:

command = "python /etc/tedge/operations/command.py"

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

Allow arguments/subcommands in custom operations #2568

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-18T21:06:35Z

Codecov Report

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

Comparison is base (fb82765) 75.8% compared to head (df6e1ab) 75.9%.
Report is 8 commits behind head on main.

❗ Current head df6e1ab differs from pull request most recent head b821fdf. Consider uploading reports for the commit b821fdf to get more accurate results

Additional details and impacted files

Files	Coverage Δ
crates/core/plugin_sm/src/plugin.rs	`11.8% <0.0%> (-0.1%)`	⬇️
crates/extensions/c8y_mapper_ext/src/converter.rs	`81.6% <16.6%> (-0.3%)`	⬇️
crates/common/logged_command/src/logged_command.rs	`79.2% <52.9%> (-2.1%)`	⬇️

... and 9 files with indirect coverage changes

github-actions · 2024-01-18T21:48:36Z

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
382	0	3	382	100	52m38.941s

albinsuresh

LGTM

...ustom_operation/custom_operation_multiple_args/operation_multiple_arguments_in_command.robot

rina23q

Panic must be avoided. Others look fine.

crates/common/logged_command/src/logged_command.rs

crates/extensions/c8y_mapper_ext/src/converter.rs

Bravo555

blocker: update any relevant documentation (example 1)

Besides the panic that Rina mentioned, I think it would be the best to address the documentation as part of this PR. We need to keep in mind what are the limitations of shell_words:

The result is exactly the same as one obtained from Unix shell as long as those unsupported features are not present in input: no operators, no variable assignments, no tilde expansion, no parameter expansion, no command substitution, no arithmetic expansion, no pathname expansion.

In case those unsupported shell features are present, the syntax that introduce them is interpreted literally.

This can potentially confusing to users, as error message Operation execution failed: No such file or directory (os error 2). Command: ~/../etc/tedge/operations/command_1.sh. Operation name: c8y_Command one might think that the file doesn't exist instead of the tilde character not getting expanded. Granted most users shouldn't do that kind of thing anyway, so I'd say error messages are enough, but this behaviour needs to be described in the documentation.

crates/common/logged_command/src/logged_command.rs

didier-wenzek · 2024-01-22T09:00:26Z

crates/extensions/c8y_mapper_ext/src/converter.rs

+        if !args.is_empty() {
+            logged.args(&args);
+        }
+
        logged.arg(&payload);


Beware, that the behavior is not obvious and has to be documented: the smart rest payload is always added as the very last argument.

The command = "python /etc/tedge/operations/command.py" actually leads to the execution of python /etc/tedge/operations/command.py $SMART_REST_PAYLOAD.

didier-wenzek

The changes can be simpler by moving the logic to parse the command line into the logged_command crate.

Instead of introducing an args method to mutate a LoggedCommand that tries to rebuild the command line from where the arguments have been extracted, it would be far simpler to pass the command line to LoggedCommand::new() and do the split there.

crates/common/logged_command/src/logged_command.rs

didier-wenzek

Approved

Bravo555 · 2024-01-26T12:20:27Z

crates/common/logged_command/src/logged_command.rs

+        let mut args = shell_words::split(&command_line)?;
+
+        let mut command = match args.len() {
+            0 => return Err(shell_words::ParseError),


The Display impl for shell_words::ParseError is "missing closing quote" which is very much not the case here. We should use our own error with a correct message.

Also one more thing, need to add time feature to logged_command/Cargo.toml, because without it it's impossible to run tests for the package.

Signed-off-by: Krzysztof Piotrowski <krzysztof.piotrowski@inetum.com>

rina23q

Approved

rina23q · 2024-01-29T10:25:01Z

crates/common/logged_command/src/logged_command.rs

@@ -148,23 +148,40 @@ impl std::fmt::Display for LoggedCommand {
 }

 impl LoggedCommand {
-    pub fn new(program: impl AsRef<OsStr>) -> LoggedCommand {
+    pub fn new(program: impl AsRef<OsStr>) -> Result<LoggedCommand, std::io::Error> {


Very minor: I think try_new() is a better name as the function now returns a Result.

Ruadhri17 had a problem deploying to Test Pull Request January 18, 2024 20:51 — with GitHub Actions Failure

Ruadhri17 force-pushed the custom-operation-subcommands branch from e12852d to b194649 Compare January 18, 2024 20:57

Ruadhri17 temporarily deployed to Test Pull Request January 18, 2024 21:04 — with GitHub Actions Inactive

Ruadhri17 had a problem deploying to Test Auto January 18, 2024 21:36 — with GitHub Actions Failure

Ruadhri17 force-pushed the custom-operation-subcommands branch from 4cf2a0f to 834aea9 Compare January 18, 2024 22:00

Ruadhri17 temporarily deployed to Test Pull Request January 18, 2024 22:07 — with GitHub Actions Inactive

Ruadhri17 temporarily deployed to Test Auto January 19, 2024 07:34 — with GitHub Actions Inactive

Ruadhri17 marked this pull request as ready for review January 19, 2024 09:55

Ruadhri17 requested review from reubenmiller, Bravo555, didier-wenzek, albinsuresh, jarhodes314, rina23q and a team as code owners January 19, 2024 09:55

Bravo555 self-assigned this Jan 19, 2024

albinsuresh approved these changes Jan 19, 2024

View reviewed changes

...ustom_operation/custom_operation_multiple_args/operation_multiple_arguments_in_command.robot Show resolved Hide resolved

rina23q requested changes Jan 19, 2024

View reviewed changes

crates/common/logged_command/src/logged_command.rs Outdated Show resolved Hide resolved

crates/extensions/c8y_mapper_ext/src/converter.rs Outdated Show resolved Hide resolved

Bravo555 requested changes Jan 19, 2024

View reviewed changes

reubenmiller reviewed Jan 19, 2024

View reviewed changes

crates/common/logged_command/src/logged_command.rs Outdated Show resolved Hide resolved

didier-wenzek reviewed Jan 22, 2024

View reviewed changes

Ruadhri17 force-pushed the custom-operation-subcommands branch 3 times, most recently from e961441 to c6fc170 Compare January 25, 2024 22:24

Ruadhri17 had a problem deploying to Test Pull Request January 25, 2024 22:32 — with GitHub Actions Failure

Ruadhri17 force-pushed the custom-operation-subcommands branch from c6fc170 to 30d2e66 Compare January 25, 2024 22:41

Ruadhri17 temporarily deployed to Test Pull Request January 25, 2024 22:48 — with GitHub Actions Inactive

Ruadhri17 temporarily deployed to Test Auto January 25, 2024 23:10 — with GitHub Actions Inactive

didier-wenzek reviewed Jan 26, 2024

View reviewed changes

crates/common/logged_command/src/logged_command.rs Outdated Show resolved Hide resolved

Bravo555 removed their assignment Jan 26, 2024

Ruadhri17 force-pushed the custom-operation-subcommands branch from 30d2e66 to f3d7b8e Compare January 26, 2024 10:28

Ruadhri17 temporarily deployed to Test Pull Request January 26, 2024 10:35 — with GitHub Actions Inactive

Ruadhri17 temporarily deployed to Test Auto January 26, 2024 10:39 — with GitHub Actions Inactive

didier-wenzek approved these changes Jan 26, 2024

View reviewed changes

didier-wenzek requested review from Bravo555 and rina23q January 26, 2024 10:53

Bravo555 requested changes Jan 26, 2024

View reviewed changes

Ruadhri17 force-pushed the custom-operation-subcommands branch from f3d7b8e to df6e1ab Compare January 28, 2024 15:58

Ruadhri17 temporarily deployed to Test Pull Request January 28, 2024 16:07 — with GitHub Actions Inactive

Ruadhri17 temporarily deployed to Test Auto January 28, 2024 16:36 — with GitHub Actions Inactive

Bravo555 approved these changes Jan 29, 2024

View reviewed changes

Ruadhri17 added 4 commits January 29, 2024 10:43

Add support for multiple arguments when creating LoggedCommand

1588c2e

Signed-off-by: Krzysztof Piotrowski <krzysztof.piotrowski@inetum.com>

Support multiple arguments when executing the command

a01c6d7

Signed-off-by: Krzysztof Piotrowski <krzysztof.piotrowski@inetum.com>

Add system test that check multiple arguments in custom operation

94a5eed

Signed-off-by: Krzysztof Piotrowski <krzysztof.piotrowski@inetum.com>

Add documentation part about arguments in custom operations' command

b821fdf

Signed-off-by: Krzysztof Piotrowski <krzysztof.piotrowski@inetum.com>

Ruadhri17 force-pushed the custom-operation-subcommands branch from df6e1ab to b821fdf Compare January 29, 2024 10:44

rina23q approved these changes Jan 29, 2024

View reviewed changes

Ruadhri17 temporarily deployed to Test Pull Request January 29, 2024 10:51 — with GitHub Actions Inactive

Ruadhri17 temporarily deployed to Test Auto January 29, 2024 11:08 — with GitHub Actions Inactive

didier-wenzek added this pull request to the merge queue Jan 29, 2024

Merged via the queue into thin-edge:main with commit 952f0d1 Jan 29, 2024
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for commands with multiple arguments in custom operations #2602

Add support for commands with multiple arguments in custom operations #2602

Ruadhri17 commented Jan 18, 2024 •

edited

Loading

codecov bot commented Jan 18, 2024 •

edited

Loading

github-actions bot commented Jan 18, 2024 •

edited

Loading

albinsuresh left a comment

rina23q left a comment •

edited

Loading

Bravo555 left a comment

didier-wenzek Jan 22, 2024

didier-wenzek left a comment

didier-wenzek left a comment

Bravo555 Jan 26, 2024

Bravo555 Jan 26, 2024 •

edited

Loading

rina23q left a comment

rina23q Jan 29, 2024

Add support for commands with multiple arguments in custom operations #2602

Add support for commands with multiple arguments in custom operations #2602

Conversation

Ruadhri17 commented Jan 18, 2024 • edited Loading

Proposed changes

Types of changes

Paste Link to the issue

Checklist

Further comments

codecov bot commented Jan 18, 2024 • edited Loading

Codecov Report

github-actions bot commented Jan 18, 2024 • edited Loading

Robot Results

albinsuresh left a comment

Choose a reason for hiding this comment

rina23q left a comment • edited Loading

Choose a reason for hiding this comment

Bravo555 left a comment

Choose a reason for hiding this comment

didier-wenzek Jan 22, 2024

Choose a reason for hiding this comment

didier-wenzek left a comment

Choose a reason for hiding this comment

didier-wenzek left a comment

Choose a reason for hiding this comment

Bravo555 Jan 26, 2024

Choose a reason for hiding this comment

Bravo555 Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

rina23q left a comment

Choose a reason for hiding this comment

rina23q Jan 29, 2024

Choose a reason for hiding this comment

Ruadhri17 commented Jan 18, 2024 •

edited

Loading

codecov bot commented Jan 18, 2024 •

edited

Loading

github-actions bot commented Jan 18, 2024 •

edited

Loading

rina23q left a comment •

edited

Loading

Bravo555 Jan 26, 2024 •

edited

Loading