[cli] Toggle Primary Instance (petenv) #2102

surahman · 2021-05-12T00:03:17Z

Issue #1138:
I have added support to handle the client.primary-name= command. I am not sure about the mock tests I have set up and they would require some review. When stepping through the tests with a debugger I was unable to hit breakpoints on set and set_aux within the utils/settings.cpp file - which is typically an indicator of an issue. Manual argument passing tests work on the cmdl.

ricab

Hi @surahman, thanks for the PR. I am afraid it doesn't cut it yet though.

The logic for disabling primary would need to go in the commands that deal with it (along with the "if" condition below).

tests/test_mock_settings.cpp

src/utils/settings.cpp

src/client/cli/cmd/set.cpp

surahman · 2021-05-12T15:33:46Z

The logic for disabling primary would need to go in the commands that deal with it (along with the "if" condition below).

~~I will correct the issue with the error being thrown about the missing settings key parameter.~~
The positional arg client.primary-name= is to being handled with and without the instance name. Are you referring to handling further downstream once the command line param is parsed and clears checks?

ricab · 2021-05-13T09:13:10Z

Hey @surahman, yeah I mean the places where the primary instance gets special treatment:

$ grep -r petenv_key src/client/ include/
src/client/gui/gui_cmd.cpp:    auto petenv_name = MP_SETTINGS.get(petenv_key).toStdString();
src/client/cli/cmd/restart.cpp:    const auto petenv_name = MP_SETTINGS.get(petenv_key);
src/client/cli/cmd/stop.cpp:    const auto petenv_name = MP_SETTINGS.get(petenv_key);
src/client/cli/cmd/suspend.cpp:    const auto petenv_name = MP_SETTINGS.get(petenv_key);
src/client/cli/cmd/launch.cpp:    petenv_name = MP_SETTINGS.get(petenv_key);
src/client/cli/cmd/shell.cpp:    petenv_name = MP_SETTINGS.get(petenv_key);
src/client/cli/cmd/start.cpp:    petenv_name = MP_SETTINGS.get(petenv_key);
include/multipass/constants.h:constexpr auto petenv_key = "client.primary-name";     // This will eventually be moved to some dynamic settings schema
include/multipass/cli/format_utils.h:    const auto petenv_name = MP_SETTINGS.get(petenv_key).toStdString();

They would have to be modified to have the primary instance disabled. Otherwise this just sets the name to the empty string.

(Note that, for historic reasons, the code often uses the term petenv, short for "pet environment", to designate the primary instance.)

src/utils/settings.cpp

[tests] mock tests need to be double checked. [utils] handling <"client.primary-name=">. [utils] refactoring. [tests] relocated mock tests to <settings> fixtures. [tests] resetting file. [utils] cleaning up code. [utils] typo with empty string check. [utils] reverting.

surahman · 2021-05-15T17:35:16Z

$ grep -r petenv_key src/client/ include/
src/client/gui/gui_cmd.cpp:    auto petenv_name = MP_SETTINGS.get(petenv_key).toStdString();
src/client/cli/cmd/restart.cpp:    const auto petenv_name = MP_SETTINGS.get(petenv_key);
src/client/cli/cmd/stop.cpp:    const auto petenv_name = MP_SETTINGS.get(petenv_key);
src/client/cli/cmd/suspend.cpp:    const auto petenv_name = MP_SETTINGS.get(petenv_key);
src/client/cli/cmd/launch.cpp:    petenv_name = MP_SETTINGS.get(petenv_key);
src/client/cli/cmd/shell.cpp:    petenv_name = MP_SETTINGS.get(petenv_key);
src/client/cli/cmd/start.cpp:    petenv_name = MP_SETTINGS.get(petenv_key);
include/multipass/constants.h:constexpr auto petenv_key = "client.primary-name";     // This will eventually be moved to some dynamic settings schema
include/multipass/cli/format_utils.h:    const auto petenv_name = MP_SETTINGS.get(petenv_key).toStdString();

@ricab thanks for the information. How would you like to have the primary is disabled situation dealt with? Is the correct of dealing with this to just return from the function with a success (potentially a silent error) or a failure (input command/request invalid) code?

For now, I have set a check for disabled primary instances and returned a CommandFailed status where applicable. ~~I will add a check to ensure key is set to client.primary-name, but it currently only checks for name set to empty.~~ petenv_key is a constant, as pointed out earlier.

[tests] clang-formatted.

ricab · 2021-05-17T11:09:46Z

Hi @surahman, this is quite tricky. if the primary instance is disabled, multipass needs to disable any special treatment for it and error out in commands that need an instance but get none. So, if the user said for instance multipass start, without an instance name, multipass would indeed need to error out. Commands like start, stop, shell... would require at least one instance argument. Pretty much like a multipass delete would, but perhaps mentioning that the primary instance is disabled in the error message. The help messages would also need to be updated, where they mention primary today.

ricab · 2021-05-17T11:11:18Z

BTW, I haven't looked at your new code yet. Please let me know when you feel it's ready for another review pass.

[cli] clang-format reversions.

codecov · 2021-05-20T03:19:07Z

Codecov Report

Merging #2102 (4cb066e) into main (2126b95) will increase coverage by 0.06%.
The diff coverage is 98.21%.

@@            Coverage Diff             @@
##             main    #2102      +/-   ##
==========================================
+ Coverage   82.91%   82.98%   +0.06%     
==========================================
  Files         186      186              
  Lines        9695     9730      +35     
==========================================
+ Hits         8039     8074      +35     
  Misses       1656     1656

Impacted Files	Coverage Δ
src/utils/settings.cpp	`22.09% <0.00%> (ø)`
src/client/cli/cmd/launch.cpp	`78.22% <100.00%> (+0.08%)`	⬆️
src/client/cli/cmd/restart.cpp	`92.00% <100.00%> (+1.30%)`	⬆️
src/client/cli/cmd/shell.cpp	`84.41% <100.00%> (+1.55%)`	⬆️
src/client/cli/cmd/start.cpp	`97.56% <100.00%> (+0.19%)`	⬆️
src/client/cli/cmd/stop.cpp	`94.44% <100.00%> (+0.82%)`	⬆️
src/client/cli/cmd/suspend.cpp	`92.50% <100.00%> (+1.59%)`	⬆️

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 2126b95...4cb066e. Read the comment docs.

surahman · 2021-05-20T03:28:19Z

@ricab I have gone ahead and updated the tests to include parameters. ATM start errors out if a command is passed without a parameter.

The other commands are also set to error out if the petenv is disabled. I gather that you would like a message to be sent to cerr and the command to return ok instead? ~~Are there any example tests on how to capture the messages sent to the streams?~~ Found it.

I will look through the help messages to locate instances of primary.

ricab · 2021-05-20T10:42:03Z

Hmm, I don't quite follow, but primary is an instance that is automatically launched, started, stoped, etc. when you use the corresponding commands without arguments. And its name can change, so that after multipass set client.primary_name=heyo, multipass start # no args launches an instance named heyo, multipass stop would stop it, etc. BTW, have you seen https://multipass.run/docs/primary-instance ?

So, to disable it, stop, start, etc. should now error out, since they don't know what instance to work with. So that's what the tests should check, for instance:

    EXPECT_CALL(mock_settings, get(Eq(mp::petenv_key))).WillRepeatedly(Return(""));
    EXPECT_THAT(send_command({"start" /* no further args */ }), Eq(mp::ReturnCode::CommandFail));

while

    EXPECT_CALL(mock_settings, get(Eq(mp::petenv_key))).WillRepeatedly(Return("something"));
    EXPECT_THAT(send_command({"start" /* no further args */ }), Eq(mp::ReturnCode::CommandSuccess));

The launch command is different, because it expects an image name rather than an instance name. So multipass launch # no args is unrelated to primary.

BTW, the help messages are dynamically generated, so you should look for petenv_key rather than primary.

surahman · 2021-05-20T15:08:30Z

My adjustments to start seem to be up to spec, all the other tests for start are passing including start_cmd_considers_configured_petenv (Jarjar Binks is happy):

    const auto custom_petenv = "";
    EXPECT_CALL(mock_settings, get(Eq(mp::petenv_key))).WillRepeatedly(Return(custom_petenv));
    EXPECT_THAT(send_command({"start"}), Eq(mp::ReturnCode::CommandFail));

For the others, I will need to handle the disabled primary in the parse_args to check for additional passed parameter counts when necessary. Still drop an error message on cerr?

I will update the descriptions to include behavior for a disabled primary.

ricab · 2021-08-04T09:16:51Z

bors try

bors · 2021-08-04T09:16:52Z

try

Already running a review

Saviq · 2021-08-04T09:34:48Z

bors try-

Saviq · 2021-08-04T09:35:05Z

bors try

ricab · 2021-08-04T09:53:39Z

bors cancel

ricab · 2021-08-04T09:54:56Z

bors try-

ricab · 2021-08-04T09:55:35Z

bors try

bors · 2021-08-04T11:21:50Z

try

Build failed:

CI (Windows, hyperv)

ricab · 2021-08-04T12:01:36Z

@surahman I believe you have dropped some test code when fixing the conflicts. I didn't want to force-push into your repo (probably couldn't anyway) so I have pushed a fixed merge in the branch primary-null-proposed.

You should be able to move this PR to it with the following steps:

git checkout set_client.primary-name=null
git fetch <canonicals_multipass_remote>
git reset --hard <canonicals_multipass_remote>/primary-null-proposed
git push --force (after confirming it's pointing at the new merge commit)

surahman · 2021-08-04T14:42:34Z

@ricab sorry about that, thanks for the fix 👍🏽.

ricab · 2021-08-04T14:44:10Z

Alright, let's try again.

bors try

bors · 2021-08-04T16:06:03Z

try

Build succeeded:

ricab

Alright, let's merge this. Yeyyy 🥳 Thanks again for your effort getting this done @surahman

bors merge

2102: [cli] Toggle Primary Instance (petenv) r=ricab a=surahman **_Issue #1138:_** I have added support to handle the `client.primary-name=` command. I am not sure about the mock tests I have set up and they would require some review. When stepping through the tests with a debugger I was unable to hit breakpoints on `set` and `set_aux` within the `utils/settings.cpp` file - which is typically an indicator of an issue. Manual argument passing tests work on the `cmdl`. Co-authored-by: Saad Ur Rahman <[email protected]>

surahman · 2021-08-04T17:11:45Z

@ricab you are most welcome! Thanks for all your patience, guidance, and assistance with this.

bors · 2021-08-04T17:50:46Z

Build failed:

CI (macOS, hyperkit)

ricab · 2021-08-04T18:05:36Z

bors retry

2102: [cli] Toggle Primary Instance (petenv) r=ricab a=surahman **_Issue #1138:_** I have added support to handle the `client.primary-name=` command. I am not sure about the mock tests I have set up and they would require some review. When stepping through the tests with a debugger I was unable to hit breakpoints on `set` and `set_aux` within the `utils/settings.cpp` file - which is typically an indicator of an issue. Manual argument passing tests work on the `cmdl`. Co-authored-by: Saad Ur Rahman <[email protected]>

bors · 2021-08-04T18:40:07Z

Build failed:

Windows

ricab · 2021-08-05T08:59:27Z

bors retry

bors · 2021-08-05T10:36:59Z

Build succeeded:

surahman changed the title ~~[utils] Set "client.primary_name="~~ [utils] Setting "client.primary_name=" May 12, 2021

ricab requested changes May 12, 2021

View reviewed changes

tests/test_mock_settings.cpp Outdated Show resolved Hide resolved

src/utils/settings.cpp Outdated Show resolved Hide resolved

src/client/cli/cmd/set.cpp Outdated Show resolved Hide resolved

ricab reviewed May 13, 2021

View reviewed changes

src/utils/settings.cpp Show resolved Hide resolved

surahman added 3 commits May 15, 2021 16:54

[gui] returning <CommandFail> on disabled primary instance ops.

5ea96a6

[cli] returning <CommandFail> on disabled primary instance ops.

2a862a2

[tests] <start> <stop> <suspend> <restart> <launch> <shell>.

9707135

[tests] clang-formatted.

surahman added 4 commits May 19, 2021 22:43

[gui] resetting formatting on file.

5cd9b0e

[cli] resetting formatting.

591bbb8

[tests] start fails on disabled petenv.

d210229

[cli] clang-format reversions.

1e1671a

[cli] clang-format reversions.

[tests] updated to pass parameters.

22d7736

surahman added 8 commits May 20, 2021 13:14

[cli] restart handling disabled primary.

1fa0f58

[cli] stop handling disabled primary.

c5117d1

[cli] suspend handling disabled primary.

0905169

[cli] shell and launch no longer concerned with disabled primary.

08b3b7b

[tests] removed unneeded mock instance from start.

31e66a2

[cli] shell handles disabled primary.

cf33431

[tests] fixed launch w/ disabled primary.

55933d0

[tests] clang-format fix.

5981ade

surahman requested a review from ricab May 20, 2021 19:12

bors bot added a commit that referenced this pull request Aug 4, 2021

Try #2102:

80cc250

ricab closed this Aug 4, 2021

ricab reopened this Aug 4, 2021

bors bot added a commit that referenced this pull request Aug 4, 2021

Try #2102:

845d778

ricab closed this Aug 4, 2021

ricab reopened this Aug 4, 2021

Merge branch 'main' into set_client.primary-name=null

4cb066e

bors bot added a commit that referenced this pull request Aug 4, 2021

Try #2102:

69bb289

ricab approved these changes Aug 4, 2021

View reviewed changes

bors bot merged commit a130207 into canonical:main Aug 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[cli] Toggle Primary Instance (petenv) #2102

[cli] Toggle Primary Instance (petenv) #2102

surahman commented May 12, 2021

ricab left a comment

surahman commented May 12, 2021 •

edited

Loading

ricab commented May 13, 2021

surahman commented May 15, 2021 •

edited

Loading

ricab commented May 17, 2021

ricab commented May 17, 2021

codecov bot commented May 20, 2021 •

edited

Loading

surahman commented May 20, 2021 •

edited

Loading

ricab commented May 20, 2021 •

edited

Loading

surahman commented May 20, 2021 •

edited

Loading

ricab commented Aug 4, 2021

bors bot commented Aug 4, 2021

Saviq commented Aug 4, 2021

Saviq commented Aug 4, 2021

ricab commented Aug 4, 2021

ricab commented Aug 4, 2021

ricab commented Aug 4, 2021

bors bot commented Aug 4, 2021

ricab commented Aug 4, 2021

surahman commented Aug 4, 2021

ricab commented Aug 4, 2021

bors bot commented Aug 4, 2021

ricab left a comment

surahman commented Aug 4, 2021

bors bot commented Aug 4, 2021

ricab commented Aug 4, 2021

bors bot commented Aug 4, 2021

ricab commented Aug 5, 2021

bors bot commented Aug 5, 2021

[cli] Toggle Primary Instance (petenv) #2102

[cli] Toggle Primary Instance (petenv) #2102

Conversation

surahman commented May 12, 2021

ricab left a comment

Choose a reason for hiding this comment

surahman commented May 12, 2021 • edited Loading

ricab commented May 13, 2021

surahman commented May 15, 2021 • edited Loading

ricab commented May 17, 2021

ricab commented May 17, 2021

codecov bot commented May 20, 2021 • edited Loading

Codecov Report

surahman commented May 20, 2021 • edited Loading

ricab commented May 20, 2021 • edited Loading

surahman commented May 20, 2021 • edited Loading

ricab commented Aug 4, 2021

bors bot commented Aug 4, 2021

try

Saviq commented Aug 4, 2021

Saviq commented Aug 4, 2021

ricab commented Aug 4, 2021

ricab commented Aug 4, 2021

ricab commented Aug 4, 2021

bors bot commented Aug 4, 2021

try

ricab commented Aug 4, 2021

surahman commented Aug 4, 2021

ricab commented Aug 4, 2021

bors bot commented Aug 4, 2021

try

ricab left a comment

Choose a reason for hiding this comment

surahman commented Aug 4, 2021

bors bot commented Aug 4, 2021

ricab commented Aug 4, 2021

bors bot commented Aug 4, 2021

ricab commented Aug 5, 2021

bors bot commented Aug 5, 2021

surahman commented May 12, 2021 •

edited

Loading

surahman commented May 15, 2021 •

edited

Loading

codecov bot commented May 20, 2021 •

edited

Loading

surahman commented May 20, 2021 •

edited

Loading

ricab commented May 20, 2021 •

edited

Loading

surahman commented May 20, 2021 •

edited

Loading