Merge #2102

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]>

Author: bors[bot]

include/multipass/cli/client_platform.h

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2018-2019 Canonical, Ltd.
+ * Copyright (C) 2018-2021 Canonical, Ltd.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -32,7 +32,7 @@ namespace platform
 void parse_transfer_entry(const QString& entry, QString& path, QString& instance_name);
 int getuid();
 int getgid();
-void open_multipass_shell(const QString& instance_name);
+void open_multipass_shell(const QString& instance_name); // precondition: requires a valid instance name
 QStringList gui_tray_notification_strings();
 }
 }

src/client/cli/cmd/launch.cpp

@@ -185,12 +185,15 @@ mp::ParseCode cmd::Launch::parse_args(mp::ArgParser* parser)
                                            "in bytes, or with K, M, G suffix.\nMinimum: {}, default: {}.",
                                            min_memory_size, default_memory_size)),
         "mem", QString::fromUtf8(default_memory_size)); // In MB's
-    QCommandLineOption nameOption(
-        {"n", "name"},
-        QString{"Name for the instance. If it is '%1' (the configured primary instance name), the user's home "
-                "directory is mounted inside the newly launched instance, in '%2'."}
-            .arg(petenv_name, mp::home_automount_dir),
-        "name");
+
+    const auto name_option_desc =
+        petenv_name.isEmpty()
+            ? QString{"Name for the instance."}
+            : QString{"Name for the instance. If it is '%1' (the configured primary instance name), the user's home "
+                      "directory is mounted inside the newly launched instance, in '%2'."}
+                  .arg(petenv_name, mp::home_automount_dir);
+
+    QCommandLineOption nameOption({"n", "name"}, name_option_desc, "name");
     QCommandLineOption cloudInitOption("cloud-init", "Path to a user-data cloud-init configuration, or '-' for stdin",
                                        "file");
     QCommandLineOption networkOption("network",

src/client/cli/cmd/restart.cpp

@@ -82,11 +82,17 @@ QString cmd::Restart::description() const
 mp::ParseCode cmd::Restart::parse_args(mp::ArgParser* parser)
 {
     const auto petenv_name = MP_SETTINGS.get(petenv_key);
-    parser->addPositionalArgument(
-        "name",
-        QString{"Names of instances to restart. If omitted, and without the --all option, '%1' will be assumed."}.arg(
-            petenv_name),
-        "[<name> ...]");
+
+    const auto& [description, syntax] =
+        petenv_name.isEmpty()
+            ? std::make_pair(QString{"Names of instances to restart."}, QString{"<name> [<name> ...]"})
+            : std::make_pair(
+                  QString{
+                      "Names of instances to restart. If omitted, and without the --all option, '%1' will be assumed."}
+                      .arg(petenv_name),
+                  QString{"[<name> ...]"});
+
+    parser->addPositionalArgument("name", description, syntax);
 
     QCommandLineOption all_option(all_option_name, "Restart all instances");
     parser->addOption(all_option);
@@ -108,9 +114,14 @@ mp::ParseCode cmd::Restart::parse_args(mp::ArgParser* parser)
         return ParseCode::CommandLineError;
     }
 
-    auto parse_code = check_for_name_and_all_option_conflict(parser, cerr, /*allow_empty=*/true);
+    auto parse_code = check_for_name_and_all_option_conflict(parser, cerr, /*allow_empty=*/!petenv_name.isEmpty());
     if (parse_code != ParseCode::Ok)
+    {
+        if (petenv_name.isEmpty() && parser->positionalArguments().isEmpty())
+            fmt::print(cerr, "Note: the primary instance is disabled.\n");
+
         return parse_code;
+    }
 
     request.mutable_instance_names()->CopyFrom(add_instance_names(parser, /*default_name=*/petenv_name.toStdString()));
 

src/client/cli/cmd/shell.cpp

@@ -128,13 +128,16 @@ QString cmd::Shell::description() const
 
 mp::ParseCode cmd::Shell::parse_args(mp::ArgParser* parser)
 {
-    parser->addPositionalArgument(
-        "name",
-        QString{
-            "Name of the instance to open a shell on. If omitted, '%1' (the configured primary instance name) will be "
-            "assumed. If the instance is not running, an attempt is made to start it (see `start` for more info)."}
-            .arg(petenv_name),
-        "[<name>]");
+    const auto& [description, syntax] =
+        petenv_name.isEmpty()
+            ? std::make_pair(QString{"Name of instance to open a shell on."}, QString{"<name>"})
+            : std::make_pair(QString{"Name of the instance to open a shell on. If omitted, '%1' (the configured "
+                                     "primary instance name) will be assumed. If the instance is not running, an "
+                                     "attempt is made to start it (see `start` for more info)."}
+                                 .arg(petenv_name),
+                             QString{"[<name>]"});
+
+    parser->addPositionalArgument("name", description, syntax);
 
     mp::cmd::add_timeout(parser);
 
@@ -162,10 +165,15 @@ mp::ParseCode cmd::Shell::parse_args(mp::ArgParser* parser)
         cerr << "Too many arguments given\n";
         status = ParseCode::CommandLineError;
     }
-    else
+    else if (auto instance = num_args ? pos_args.first() : petenv_name; !instance.isEmpty())
     {
         auto entry = request.add_instance_name();
-        entry->append(num_args ? pos_args.first().toStdString() : petenv_name.toStdString());
+        entry->append(instance.toStdString());
+    }
+    else
+    {
+        fmt::print(cerr, "The primary instance is disabled, please provide an instance name.\n");
+        return ParseCode::CommandLineError;
     }
 
     return status;

src/client/cli/cmd/start.cpp

@@ -155,13 +155,17 @@ QString cmd::Start::description() const
 
 mp::ParseCode cmd::Start::parse_args(mp::ArgParser* parser)
 {
-    parser->addPositionalArgument(
-        "name",
-        QString{"Names of instances to start. If omitted, and without the --all option, '%1' (the configured primary "
-                "instance name) will be assumed. If '%1' does not exist but is included in a successful start command "
-                "(either implicitly or explicitly), it is launched automatically (see `launch` for more info)."}
-            .arg(petenv_name),
-        "[<name> ...]");
+    const auto& [description, syntax] =
+        petenv_name.isEmpty()
+            ? std::make_pair(QString{"Names of instances to start."}, QString{"<name> [<name> ...]"})
+            : std::make_pair(QString{"Names of instances to start. If omitted, and without the --all option, '%1' (the "
+                                     "configured primary instance name) will be assumed. If '%1' does not exist but is "
+                                     "included in a successful start command either implicitly or explicitly), it is "
+                                     "launched automatically (see `launch` for more info)."}
+                                 .arg(petenv_name),
+                             QString{"[<name> ...]"});
+
+    parser->addPositionalArgument("name", description, syntax);
 
     QCommandLineOption all_option(all_option_name, "Start all instances");
     parser->addOption(all_option);
@@ -173,9 +177,14 @@ mp::ParseCode cmd::Start::parse_args(mp::ArgParser* parser)
     if (status != ParseCode::Ok)
         return status;
 
-    auto parse_code = check_for_name_and_all_option_conflict(parser, cerr, /*allow_empty=*/true);
+    auto parse_code = check_for_name_and_all_option_conflict(parser, cerr, /*allow_empty=*/!petenv_name.isEmpty());
     if (parse_code != ParseCode::Ok)
+    {
+        if (petenv_name.isEmpty() && parser->positionalArguments().isEmpty())
+            fmt::print(cerr, "Note: the primary instance is disabled.\n");
+
         return parse_code;
+    }
 
     try
     {

src/client/cli/cmd/stop.cpp

@@ -68,11 +68,16 @@ QString cmd::Stop::description() const
 mp::ParseCode cmd::Stop::parse_args(mp::ArgParser* parser)
 {
     const auto petenv_name = MP_SETTINGS.get(petenv_key);
-    parser->addPositionalArgument(
-        "name",
-        QString{"Names of instances to stop. If omitted, and without the --all option, '%1' will be assumed"}.arg(
-            petenv_name),
-        "[<name> ...]");
+
+    const auto& [description, syntax] =
+        petenv_name.isEmpty()
+            ? std::make_pair(QString{"Names of instances to stop."}, QString{"<name> [<name> ...]"})
+            : std::make_pair(
+                  QString{"Names of instances to stop. If omitted, and without the --all option, '%1' will be assumed."}
+                      .arg(petenv_name),
+                  QString{"[<name> ...]"});
+
+    parser->addPositionalArgument("name", description, syntax);
 
     QCommandLineOption all_option(all_option_name, "Stop all instances");
     QCommandLineOption time_option({"t", "time"}, "Time from now, in minutes, to delay shutdown of the instance",
@@ -84,9 +89,14 @@ mp::ParseCode cmd::Stop::parse_args(mp::ArgParser* parser)
     if (status != ParseCode::Ok)
         return status;
 
-    auto parse_code = check_for_name_and_all_option_conflict(parser, cerr, /*allow_empty=*/true);
+    auto parse_code = check_for_name_and_all_option_conflict(parser, cerr, /*allow_empty=*/!petenv_name.isEmpty());
     if (parse_code != ParseCode::Ok)
+    {
+        if (petenv_name.isEmpty() && parser->positionalArguments().isEmpty())
+            fmt::print(cerr, "Note: the primary instance is disabled.\n");
+
         return parse_code;
+    }
 
     if (parser->isSet(time_option) && parser->isSet(cancel_option))
     {

src/client/cli/cmd/suspend.cpp

@@ -68,11 +68,17 @@ QString cmd::Suspend::description() const
 mp::ParseCode cmd::Suspend::parse_args(mp::ArgParser* parser)
 {
     const auto petenv_name = MP_SETTINGS.get(petenv_key);
-    parser->addPositionalArgument(
-        "name",
-        QString{"Names of instances to suspend. If omitted, and without the --all option, '%1' will be assumed."}.arg(
-            petenv_name),
-        "[<name> ...]");
+
+    const auto& [description, syntax] =
+        petenv_name.isEmpty()
+            ? std::make_pair(QString{"Names of instances to suspend."}, QString{"<name> [<name> ...]"})
+            : std::make_pair(
+                  QString{
+                      "Names of instances to suspend. If omitted, and without the --all option, '%1' will be assumed."}
+                      .arg(petenv_name),
+                  QString{"[<name> ...]"});
+
+    parser->addPositionalArgument("name", description, syntax);
 
     QCommandLineOption all_option("all", "Suspend all instances");
     parser->addOptions({all_option});
@@ -81,9 +87,14 @@ mp::ParseCode cmd::Suspend::parse_args(mp::ArgParser* parser)
     if (status != ParseCode::Ok)
         return status;
 
-    auto parse_code = check_for_name_and_all_option_conflict(parser, cerr, /*allow_empty=*/true);
+    auto parse_code = check_for_name_and_all_option_conflict(parser, cerr, /*allow_empty=*/!petenv_name.isEmpty());
     if (parse_code != ParseCode::Ok)
+    {
+        if (petenv_name.isEmpty() && parser->positionalArguments().isEmpty())
+            fmt::print(cerr, "Note: the primary instance is disabled.\n");
+
         return parse_code;
+    }
 
     request.mutable_instance_names()->CopyFrom(add_instance_names(parser, /*default_name=*/petenv_name.toStdString()));
 

src/client/gui/gui_cmd.cpp

@@ -104,7 +104,12 @@ mp::ReturnCode cmd::GuiCmd::run(mp::ArgParser* parser)
     }
 
     update_hotkey();
-    QObject::connect(&hotkey, &QHotkey::activated, qApp, [&]() { mp::cli::platform::open_multipass_shell(QString()); });
+    QObject::connect(&hotkey, &QHotkey::activated, qApp,
+                     [&]()
+                     {
+                         if (!current_petenv_name.empty())
+                             mp::cli::platform::open_multipass_shell(QString::fromStdString(current_petenv_name));
+                     });
 
     create_actions();
     create_menu();
@@ -158,7 +163,7 @@ void cmd::GuiCmd::create_actions()
                                  {&petenv_start_action, &petenv_shell_action, &petenv_stop_action});
 
     QObject::connect(&petenv_shell_action, &QAction::triggered,
-                     [] { mp::cli::platform::open_multipass_shell(QString()); });
+                     [this] { mp::cli::platform::open_multipass_shell(QString::fromStdString(current_petenv_name)); });
     QObject::connect(&petenv_stop_action, &QAction::triggered, [this] {
         future_synchronizer.addFuture(QtConcurrent::run(this, &GuiCmd::stop_instance_for, current_petenv_name));
     });
@@ -235,6 +240,12 @@ void cmd::GuiCmd::update_menu()
     {
         about_separator->setVisible(true);
     }
+
+    const bool petenv_visibility = !current_petenv_name.empty();
+    petenv_actions_separator->setVisible(petenv_visibility);
+    petenv_start_action.setVisible(petenv_visibility);
+    petenv_shell_action.setVisible(petenv_visibility);
+    petenv_stop_action.setVisible(petenv_visibility);
 }
 
 void cmd::GuiCmd::update_about_menu()

src/client/gui/gui_cmd.h

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2019-2020 Canonical, Ltd.
+ * Copyright (C) 2019-2021 Canonical, Ltd.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by

src/platform/client/client_platform_linux.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2019 Canonical, Ltd.
+ * Copyright (C) 2019-2021 Canonical, Ltd.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -24,6 +24,7 @@ namespace mcp = multipass::cli::platform;
 
 void mcp::open_multipass_shell(const QString& instance_name)
 {
+    assert(!instance_name.isEmpty() && "Instance name cannot be empty");
     QProcess::startDetached(
         "xterm", {"-title", instance_name, "-j", "-e", QString("multipass shell %1 || read").arg(instance_name)});
 }