Add command aliases to the CLI client #1986

luis4a0 · 2021-02-26T18:37:50Z

This PR adds the ability to create aliases to be executed in determined instances.

Private bits are in https://github.com/canonical/multipass-private/pull/386

codecov · 2021-02-26T18:50:49Z

Codecov Report

Merging #1986 (e07e504) into main (a24cc2c) will increase coverage by 0.67%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1986      +/-   ##
==========================================
+ Coverage   83.30%   83.97%   +0.67%     
==========================================
  Files         190      203      +13     
  Lines        9756    10108     +352     
==========================================
+ Hits         8127     8488     +361     
+ Misses       1629     1620       -9

Impacted Files	Coverage Δ
include/multipass/platform.h	`100.00% <ø> (ø)`
include/multipass/utils.h	`100.00% <ø> (ø)`
src/daemon/default_vm_image_vault.cpp	`85.51% <ø> (ø)`
include/multipass/cli/alias_definition.h	`100.00% <100.00%> (ø)`
include/multipass/cli/alias_dict.h	`100.00% <100.00%> (ø)`
include/multipass/cli/argparser.h	`100.00% <100.00%> (ø)`
include/multipass/cli/format_utils.h	`100.00% <100.00%> (ø)`
include/multipass/cli/formatter.h	`100.00% <100.00%> (ø)`
src/client/cli/argparser.cpp	`96.91% <100.00%> (+2.21%)`	⬆️
src/client/cli/client.cpp	`100.00% <100.00%> (ø)`
... and 40 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a24cc2c...e07e504. Read the comment docs.

ricab

Hi @luis4a0, I did a high-level, superficial review pass. It looks very nice overall! 👏

The one thing that concerns me a bit, from an architectural point of view, is the multiple AliasDict objects. They are meant to always represent the same mapping, but the multiple copies open a space for conflicts. They need to be loaded and saved multiple times, which increases the potential for concurrency issues. Admittedly I have only just considered this, but it seems to me that it would be better to have a single, central object owned by the client, that the relevant commands operated on via a reference. At that point, I am not sure the separate type is beneficial (relatively to a map). Then again, perhaps it's worth keeping it so that it could be reused in the GUI client. But please let me know if there are other things I am not considering.

I suspect you went with the multiple objects because the Command objects are created with Client::add_command, which takes no arguments, while a central dict would have to be passed only to a few commands. But you can modify Client::add_command to receive and pass on whatever arguments each Command's ctor takes, with a technique called perfect forwarding. It's what things like emplace methods or make_unique use, and it can even move rvalues as appropriate. I will submit a PR that you can use if you want. (Done: #2032.)

I didn't quite get the ClientFormatter separate from the existing Formatter, but I confess I didn't look closely. I think we discussed something related to that, but I forget what the issue was. We can discuss further if you want.

luis4a0 · 2021-04-05T16:59:38Z

Hey @ricab, thanks a lot for the review!

About the multiple AliasDict objects, my intention was to read the file exactly once per session, and to write to it at most once. I thought an alternative when having a many objects would be making it a singleton, like done for Settings. OTOH, the solution you propose to add taking parameters to add_command is very elegant. I'll think a bit more on this.

About formatters, we already discussed on the topic. The current formatters work with RPC objects, while the new formatter work with a CLI-only thing, which is the alias dictionary. Implementing the dict formatters inside the current formatters would imply to link against RPC all things which use formatters, which is a big refactor. I opted to separate both, i.e., adding a "client" formatter, which formats data structures defined in the client only.

ricab · 2021-04-06T10:05:26Z

Hey @luis4a0, about AliasDict, I'll let you think about it, but let me explain my reasoning a bit better. My take is that it is better to have a direct correspondence between resource ownership and an object to represent it. It's harder to guarantee invariants with shared ownership. Classes should be designed to be reusable (e.g. to employ in a full-blown GUI), but multiple objects representing the same resource would be harder to synchronize. Assuming that only one client will use the class at a time is also not very self contained.

With a single AliasDict representing a persistent set of aliases, we'd get a single place to encapsulate synchronization and preserve invariants. Entities that only need reading would only get read access by default (e.g. with a const reference), write access being restricted to select entities (for now only the alias cmd, I think). New AliasDicts could be created if we ever needed separate alias sets.

A singleton would not allow that read/write separation. It's advantages would not be very useful here: not something to be used in arbitrary parts of the code (like settings or std paths); not something that avoids deep parameter passing all over (AliasDict param passed only in one level of the call tree); not a wrapper of otherwise free functions that we need to mock. The one advantage here would be enforcing a single object, but I don't think that's worth the "glorified global".

For the formatters, right, the dependency on the RPC... thanks for reminding me. I wish there was a way to isolate the dependency and otherwise homogenize things, but I don't have a proposal right now. I guess we could always revisit.

luis4a0 · 2021-04-06T13:01:30Z

Thanks for the explanation @ricab ! Now it makes complete sense to me what you proposed.

luis4a0 · 2021-04-06T13:02:05Z

Sorry, wrong button :X

Saviq

Hey, looks good overall! A handful of comments inline.

include/multipass/cli/format_utils.h

src/client/cli/argparser.cpp

Saviq · 2021-04-07T11:48:09Z

src/client/cli/cmd/alias.cpp

+    parser->addPositionalArgument("definition", "Alias definition in the form <instance>:'<command> [<args>]'",
+                                  "<definition>");
+
+    auto status = parser->commandParse(this);
+    if (status != ParseCode::Ok)
+        return status;
+
+    if (parser->positionalArguments().count() != 2)
+    {
+        cerr << "Wrong number of arguments given\n";
+        return ParseCode::CommandLineError;
+    }


We've not thought this through. We don't want to be splitting the args on space, or we're into escape sequences and things.

There's two options we have here, both involving the -- separator:

<name> <instance>:<command> [--] [<args> ...]

<name> <instance> <command> [--] [<args> ...]

The colon is probably overkill here, as <instance> is mandatory (as opposed to in transfer that works both ways). Option 2 follows multipass exec closest, so that'd be my preference.

@ricab, @townsend2010, WDYT?

Yeah, totally agree about not splitting args on spaces and I think option 2 is the best choice.

Thanks! About option 2: is -- alone optional? Shouldn't it be <name> <instance> <command> [-- <args> ...]?

It is optional, you only need it if any of the <args> starts with a -. All positional arguments beyond <command> are arguments to the alias, so there's no need for it.

+1 for no spaces in cmd, preferring no colon, and matching exec — this problem was already solved for that cmd, which works quite well. The only thing that surprised me is the placement of the [--]. Shouldn't it come before <command>?

So I think I'm changing my vote to option 3:

multipass alias <instance>:<command> [<alias>], where <alias> defaults to <command>

And in case we skip arguments and stick to 3, who wants arguments can create a script on the instance. But in case we got requests to add arguments to aliases, I think we'll have to add a new syntax to define an alias with commands, thus complicating the interface.

Hmm, not allowing arguments and requiring a script/alias/fn inside the instance would be quite limiting. But the arguments would still be allowed if quoted with option 3, right? So the mismatch with exec worries me more.

The awkwardness in ln isn't intrinsic, is it? Other commands have the target first (e.g. tar). It's often the case when an equals sign is implicit (e.g. git config --global alias.co checkout). With ln, I think it's rather the mismatch with cp and mv that makes it awkward. I fear we could be causing the same feel for someone coming from exec:

# if you're used to multipass -v exec foo -- ip -br -c a # it's perhaps less awkward to say multipass -v alias fooip foo -- ip -br -c -a # then multipass -v alias foo:"ip -br -c -a" fooip # idk

Let me summarize what we talked about:

ssh (the client, not the protocol) is weird (aka broken) in that:
$ ssh localhost mkdir foo\ bar $ ssh localhost ls foo bar $ ssh localhost echo \$HOME /home/ubuntu
I.e. it always has a shell interpret the command line. So the user has to be very careful on how to quote things to become separate arguments.

multipass exec is not using a shell:
$ multipass exec primary mkdir foo\ bar $ multipass exec primary ls 'foo bar' $ multipass exec primary echo \$HOME $HOME
We definitely want to maintain that in the alias feature, that exactly the arguments provided are passed through to the <command> executable in the instance. If we support arguments, you could still have a shell interpret the command with sh -c "...". If <command> includes special characters, the executable has to be called exactly that.

alias from exec is different in that alias is creating an entity (the alias), and it's prevalent in CLIs that the thing you create is last on the command line:
$ cp <src> <tgt> $ mv <src> [<src> ...] <tgt> $ ln <target> [<link>] $ snap alias <snap.app> [<alias>]

There will still be even more complex use cases that are probably out of scope, e.g. templating/placeholders.

The most common use case of aliases will be exporting a command from inside the instance to the host, and so I'd like to optimize for this:
multipass alias <instance>:<command> [<alias>]

While awkward, it's still possible to allow, with time, the addition of arguments, possibly like so:
multipass alias <instance>:<command> [<alias>] [-- <arg> …]

Note this still supports arguments when invoking the alias:
multipass <alias> [<arg> ...] <alias> [<arg> ...]
Equivalent to:
multipass exec <instance> -- <command> [<arg> ...]

I vote for implementing multipass alias <instance>:<command> [<alias>] at this stage.

Hey, thanks for the comments! I've been considering all this. Now I'm convinced that, in terms of usability and simplicity, multipass alias <instance>:<command> [<aliasname>] would be the best choice at the moment. I don't think adding parameters after -- would be a big issue. In fact, since we have three positional arguments being one of them optional, we can't avoid using --. Circumventing the use of -- would imply to either make one argument mandatory, thus losing the simplicity of the approach.

Saviq · 2021-04-07T11:48:32Z

src/client/cli/cmd/alias.cpp

+
+    std::string instance = definition.left(colon_pos).toStdString();
+    QString full_command = definition.right(definition.size() - colon_pos - 1);
+    QStringList full_command_split = full_command.split(' ', QString::SkipEmptyParts);


Suggested change

QStringList full_command_split = full_command.split(' ', QString::SkipEmptyParts);

🔥 🔥 🔥

Yeah, fire in the hole! This was now removed because we removed arguments from the interface.

src/client/cli/cmd/unalias.cpp

src/client/common/alias_dict.cpp

src/client/common/client_formatter.cpp

src/client/common/alias_dict.cpp

townsend2010

Ok, let's do this! Whoop! Whoop!

bors merge

bors · 2021-09-09T20:30:55Z

Build failed:

macOS

Base automatically changed from master to main March 3, 2021 13:41

luis4a0 force-pushed the aliases branch from 399421b to 294dd9e Compare March 3, 2021 14:46

luis4a0 force-pushed the aliases branch from 294dd9e to aa1afd7 Compare March 11, 2021 18:31

luis4a0 force-pushed the aliases branch 2 times, most recently from 5fed3ae to 772b5b9 Compare March 19, 2021 13:31

luis4a0 marked this pull request as ready for review March 19, 2021 13:32

luis4a0 force-pushed the aliases branch 4 times, most recently from 43791bb to e4d10db Compare March 26, 2021 12:37

luis4a0 force-pushed the aliases branch 2 times, most recently from 5d39c93 to 51f84cd Compare March 31, 2021 13:07

ricab reviewed Apr 1, 2021

View reviewed changes

ricab mentioned this pull request Apr 1, 2021

[cli] Allow different params in Command ctors #2032

Closed

luis4a0 closed this Apr 6, 2021

luis4a0 reopened this Apr 6, 2021

luis4a0 force-pushed the aliases branch from 545d57f to 31edebd Compare April 6, 2021 17:23

Saviq requested changes Apr 7, 2021

View reviewed changes

Saviq reviewed Apr 8, 2021

View reviewed changes

src/client/common/alias_dict.cpp Outdated Show resolved Hide resolved

src/client/common/alias_dict.cpp Show resolved Hide resolved

luis4a0 force-pushed the aliases branch 5 times, most recently from ecc58f1 to 4da9193 Compare April 16, 2021 17:50

luis4a0 force-pushed the aliases branch from 4da9193 to 1848f49 Compare April 28, 2021 13:50

luis4a0 added 20 commits September 6, 2021 14:47

[cli] Error out if cannot open aliases file.

7eba93c

[cli] Show error when if not removing alias script.

aaffd2a

[cli] Error out if creating alias file fails.

df648da

[cli] Use file_ops to ease testing.

3283104

[cli] Check instance exists on alias command.

f9b49c0

Use clang-format 12.

7e6b2fd

[tests] Fix strict mock hiding useless mocks.

1e9b715

[tests] Avoid using extra_assertions.h.

afe61e1

[cli] Remove unused capture in lambda.

1a771be

[cli] Format includes following clang-format-12.

b218904

[tests] Avoid race condition in send_commands().

6b55b6c

[cli] Use info instead of list in alias cmd.

c29707b

[tests] Check asking for info in alias cmd.

4446c16

[json_writer] Use file_ops to ease testing.

73912c9

[cli] Don't throw in the AliasDict destructor.

38cfe10

[tests] Check throwing if opening alias file fails

2c9d838

[cli] Make AliasDict inherit the term from Client.

99ce134

[tests] Check AliasDict inheriting terminal.

ca8fba4

[tests] Check file ops in aliases config.

189e8f3

[platform] Remove unneeded include.

789a554

luis4a0 force-pushed the aliases branch from 4e6c582 to 789a554 Compare September 7, 2021 11:26

Fix formatting.

1341be5

luis4a0 force-pushed the aliases branch from a1bf60d to 1341be5 Compare September 7, 2021 11:37

luis4a0 added 2 commits September 7, 2021 09:37

[linux] Quote aliases executable.

a73f1d4

[cli] Move path message to platform.

e07e504

townsend2010 approved these changes Sep 9, 2021

View reviewed changes

townsend2010 merged commit 44e7b02 into main Sep 10, 2021

bors bot deleted the aliases branch September 10, 2021 01:22

Saviq added this to the v1.8.0 milestone Oct 18, 2021

Add command aliases to the CLI client #1986

Add command aliases to the CLI client #1986

Uh oh!

Conversation

luis4a0 commented Feb 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Feb 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ricab 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!

luis4a0 commented Apr 5, 2021

Uh oh!

ricab commented Apr 6, 2021

Uh oh!

luis4a0 commented Apr 6, 2021

Uh oh!

luis4a0 commented Apr 6, 2021

Uh oh!

Saviq left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Saviq Apr 7, 2021 • 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!

luis4a0 Apr 7, 2021 • 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!

Saviq Apr 8, 2021 • 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!

Saviq Apr 8, 2021 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

townsend2010 left a comment

Choose a reason for hiding this comment

Uh oh!

bors bot commented Sep 9, 2021

Uh oh!

Uh oh!

luis4a0 commented Feb 26, 2021 •

edited

Loading

codecov bot commented Feb 26, 2021 •

edited

Loading

ricab left a comment •

edited

Loading

Saviq Apr 7, 2021 •

edited

Loading

luis4a0 Apr 7, 2021 •

edited

Loading

Saviq Apr 8, 2021 •

edited

Loading

Saviq Apr 8, 2021 •

edited

Loading