Skip to content

Commit

Permalink
Merge #1958
Browse files Browse the repository at this point in the history 
1958: Rename 'id' to 'name' in launch command r=ricab a=luis4a0

`multipass launch --network` accepts `id=` as the key to the interface name argument. This must be changed to `name=` in order to avoid confusion with the network listing, on which interface names are listed using 'name' as field name.

Co-authored-by: Luis Peñaranda <[email protected]>
Co-authored-by: Ricardo Abreu <[email protected]>
  • Loading branch information
@bors @luis4a0
@ricab @Saviq
3 people authored and Saviq committed Feb 8, 2021
1 parent 9c63746 commit 9f27f43
Show file tree
Hide file tree
Showing 4 changed files with 36 additions and 35 deletions.
12 changes: 6 additions & 6 deletions src/client/cli/cmd/launch.cpp
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (C) 2017-2020 Canonical, Ltd.
* Copyright (C) 2017-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
Expand Down Expand Up @@ -85,7 +85,7 @@ auto net_digest(const QString& options)

const auto& key = key_value_split[0].toLower();
const auto& val = key_value_split[1];
if (key == "id")
if (key == "name")
net.set_id(val.toStdString());
else if (key == "mode")
net.set_mode(checked_mode(val.toLower().toStdString()));
Expand All @@ -95,7 +95,7 @@ auto net_digest(const QString& options)
throw NetworkDefinitionException{fmt::format("Bad network field: {}", key)};
}

// Interpret as "id" the argument when there are no ',' and no '='.
// Interpret as "name" the argument when there are no ',' and no '='.
else if (key_value_split.size() == 1 && split.size() == 1)
net.set_id(key_value_split[0].toStdString());

Expand All @@ -104,7 +104,7 @@ auto net_digest(const QString& options)
}

if (net.id().empty())
throw NetworkDefinitionException{fmt::format("Bad network definition, need at least an ID field")};
throw NetworkDefinitionException{fmt::format("Bad network definition, need at least a 'name' field")};

return net;
}
Expand Down Expand Up @@ -198,11 +198,11 @@ mp::ParseCode cmd::Launch::parse_args(mp::ArgParser* parser)
QCommandLineOption networkOption("network",
"Add a network interface to the instance, where <spec> is in the "
"\"key=value,key=value\" format, with the following keys available:\n"
" id: the network to connect to (required), use the networks command for a "
" name: the network to connect to (required), use the networks command for a "
"list of possible values\n"
" mode: auto|manual (default: auto)\n"
" mac: hardware address (default: random).\n"
"You can also use a shortcut of \"<id>\" to mean \"id=<id>\".",
"You can also use a shortcut of \"<name>\" to mean \"name=<name>\".",
"spec");

parser->addOptions({cpusOption, diskOption, memOption, nameOption, cloudInitOption, networkOption});
Expand Down
2 changes: 1 addition & 1 deletion src/daemon/daemon.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -426,7 +426,7 @@ std::vector<mp::NetworkInterface> validate_extra_interfaces(const mp::LaunchRequ

if (result == factory_networks->cend())
{
mpl::log(mpl::Level::warning, category, fmt::format("Invalid network id \"{}\"", net.id()));
mpl::log(mpl::Level::warning, category, fmt::format("Invalid network name \"{}\"", net.id()));
option_errors.add_error_codes(mp::LaunchError::INVALID_NETWORK);
}

Expand Down
16 changes: 8 additions & 8 deletions tests/test_cli_client.cpp
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (C) 2017-2020 Canonical, Ltd.
* Copyright (C) 2017-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
Expand Down Expand Up @@ -675,8 +675,8 @@ INSTANTIATE_TEST_SUITE_P(Client, TestInvalidNetworkOptions,
std::vector<std::string>{"--network"},
std::vector<std::string>{"--network", "mode=manual"},
std::vector<std::string>{"--network", "mode=manual=auto"},
std::vector<std::string>{"--network", "id=eth0,mode=man"},
std::vector<std::string>{"--network", "id=eth1,mac=0a"},
std::vector<std::string>{"--network", "name=eth0,mode=man"},
std::vector<std::string>{"--network", "name=eth1,mac=0a"},
std::vector<std::string>{"--network", "eth2", "--network"}));

struct TestValidNetworkOptions : Client, WithParamInterface<std::vector<std::string>>
Expand All @@ -695,11 +695,11 @@ TEST_P(TestValidNetworkOptions, launch_cmd_return)

INSTANTIATE_TEST_SUITE_P(Client, TestValidNetworkOptions,
Values(std::vector<std::string>{"--network", "eth3"},
std::vector<std::string>{"--network", "id=eth4", "--network", "eth5"},
std::vector<std::string>{"--network", "id=eth6,mac=01:23:45:67:89:ab"},
std::vector<std::string>{"--network", "id=eth7,mode=manual"},
std::vector<std::string>{"--network", "id=eth8,mode=auto"},
std::vector<std::string>{"--network", "id=eth9", "--network", "id=eth9"}));
std::vector<std::string>{"--network", "name=eth4", "--network", "eth5"},
std::vector<std::string>{"--network", "name=eth6,mac=01:23:45:67:89:ab"},
std::vector<std::string>{"--network", "name=eth7,mode=manual"},
std::vector<std::string>{"--network", "name=eth8,mode=auto"},
std::vector<std::string>{"--network", "name=eth9", "--network", "name=eth9"}));

// purge cli tests
TEST_F(Client, purge_cmd_ok_no_args)
Expand Down
41 changes: 21 additions & 20 deletions tests/test_daemon.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -784,13 +784,13 @@ std::vector<std::string> make_args(const std::vector<std::string>& args)

INSTANTIATE_TEST_SUITE_P(
Daemon, LaunchWithNoNetworkCloudInit,
Values(make_args({}), make_args({"xenial"}), make_args({"xenial", "--network", "id=eth0,mode=manual"}),
make_args({"groovy"}), make_args({"groovy", "--network", "id=eth0,mode=manual"}),
make_args({"--network", "id=eth0,mode=manual"}), make_args({"devel"}),
make_args({"hirsute", "--network", "id=eth0,mode=manual"}), make_args({"daily:21.04"}),
make_args({"daily:21.04", "--network", "id=eth0,mode=manual"}),
make_args({"appliance:openhab", "--network", "id=eth0,mode=manual"}), make_args({"appliance:nextcloud"}),
make_args({"snapcraft:core18", "--network", "id=eth0,mode=manual"}), make_args({"snapcraft:core20"})));
Values(make_args({}), make_args({"xenial"}), make_args({"xenial", "--network", "name=eth0,mode=manual"}),
make_args({"groovy"}), make_args({"groovy", "--network", "name=eth0,mode=manual"}),
make_args({"--network", "name=eth0,mode=manual"}), make_args({"devel"}),
make_args({"hirsute", "--network", "name=eth0,mode=manual"}), make_args({"daily:21.04"}),
make_args({"daily:21.04", "--network", "name=eth0,mode=manual"}),
make_args({"appliance:openhab", "--network", "name=eth0,mode=manual"}), make_args({"appliance:nextcloud"}),
make_args({"snapcraft:core18", "--network", "name=eth0,mode=manual"}), make_args({"snapcraft:core20"})));

TEST_P(LaunchWithBridges, creates_network_cloud_init_iso)
{
Expand Down Expand Up @@ -857,10 +857,10 @@ typedef typename std::pair<std::vector<std::tuple<std::string, std::string, std:
INSTANTIATE_TEST_SUITE_P(
Daemon, LaunchWithBridges,
Values(BridgeTestArgType({{"eth0", "extra0", "52:54:00:"}}, {"extra1"}),
BridgeTestArgType({{"id=eth0,mac=01:23:45:ab:cd:ef,mode=auto", "extra0", "01:23:45:ab:cd:ef"},
BridgeTestArgType({{"name=eth0,mac=01:23:45:ab:cd:ef,mode=auto", "extra0", "01:23:45:ab:cd:ef"},
{"wlan0", "extra1", "52:54:00:"}},
{"extra2"}),
BridgeTestArgType({{"id=eth0,mode=manual", "", ""}, {"id=wlan0", "extra1", "52:54:00:"}},
BridgeTestArgType({{"name=eth0,mode=manual", "", ""}, {"name=wlan0", "extra1", "52:54:00:"}},
{"extra0", "extra2"})));

TEST_P(MinSpaceRespectedSuite, accepts_launch_with_enough_explicit_memory)
Expand Down Expand Up @@ -1230,7 +1230,7 @@ TEST_F(Daemon, prevents_repetition_of_loaded_mac_addresses)

std::stringstream stream;
EXPECT_CALL(*mock_factory, create_virtual_machine).Times(0); // expect *no* call
send_command({"launch", "--network", fmt::format("id=eth0,mac={}", repeated_mac)}, std::cout, stream);
send_command({"launch", "--network", fmt::format("name=eth0,mac={}", repeated_mac)}, std::cout, stream);
EXPECT_THAT(stream.str(), AllOf(HasSubstr("fail"), HasSubstr("Repeated MAC"), HasSubstr(repeated_mac)));
}

Expand All @@ -1248,7 +1248,7 @@ TEST_F(Daemon, does_not_hold_on_to_repeated_mac_addresses_when_loading)
mp::Daemon daemon{config_builder.build()};

EXPECT_CALL(*mock_factory, create_virtual_machine);
send_command({"launch", "--network", fmt::format("id=eth0,mac={}", mac_addr)});
send_command({"launch", "--network", fmt::format("name=eth0,mac={}", mac_addr)});
}

TEST_F(Daemon, does_not_hold_on_to_macs_when_loading_fails)
Expand All @@ -1269,7 +1269,7 @@ TEST_F(Daemon, does_not_hold_on_to_macs_when_loading_fails)
mp::Daemon daemon{config_builder.build()};

for (const auto& mac : {mac1, mac2})
send_command({"launch", "--network", fmt::format("id=eth0,mac={}", mac)});
send_command({"launch", "--network", fmt::format("name=eth0,mac={}", mac)});
}

TEST_F(Daemon, does_not_hold_on_to_macs_when_image_preparation_fails)
Expand All @@ -1282,7 +1282,7 @@ TEST_F(Daemon, does_not_hold_on_to_macs_when_image_preparation_fails)
EXPECT_CALL(*mock_factory, prepare_instance_image).WillOnce(Throw(std::exception{})).WillOnce(DoDefault());
EXPECT_CALL(*mock_factory, create_virtual_machine).Times(1);

auto cmd = std::vector<std::string>{"launch", "--network", "mac=52:54:00:73:76:28,id=wlan0"};
auto cmd = std::vector<std::string>{"launch", "--network", "mac=52:54:00:73:76:28,name=wlan0"};
send_command(cmd); // we cause this one to fail
send_command(cmd); // and confirm we can repeat the same mac
}
Expand All @@ -1294,7 +1294,7 @@ TEST_F(Daemon, releases_macs_when_launch_fails)

EXPECT_CALL(*mock_factory, create_virtual_machine).WillOnce(Throw(std::exception{})).WillOnce(DoDefault());

auto cmd = std::vector<std::string>{"launch", "--network", "mac=52:54:00:73:76:28,id=wlan0"};
auto cmd = std::vector<std::string>{"launch", "--network", "mac=52:54:00:73:76:28,name=wlan0"};
send_command(cmd); // we cause this one to fail
send_command(cmd); // and confirm we can repeat the same mac
}
Expand All @@ -1314,16 +1314,17 @@ TEST_F(Daemon, releases_macs_of_purged_instances_but_keeps_the_rest)
EXPECT_CALL(*mock_factory, create_virtual_machine(mac_matcher(mac2), _)).Times(1);
EXPECT_CALL(*mock_factory, create_virtual_machine(mac_matcher(mac3), _)).Times(2); // this one gets reused

send_command({"launch", "--network", fmt::format("id=eth0,mac={}", mac1), "--name", "vm1"});
send_command({"launch", "--network", fmt::format("id=eth0,mac={}", mac2), "--name", "vm2"});
send_command({"launch", "--network", fmt::format("id=eth0,mac={}", mac3), "--name", "vm3"});
send_command({"launch", "--network", fmt::format("name=eth0,mac={}", mac1), "--name", "vm1"});
send_command({"launch", "--network", fmt::format("name=eth0,mac={}", mac2), "--name", "vm2"});
send_command({"launch", "--network", fmt::format("name=eth0,mac={}", mac3), "--name", "vm3"});

send_command({"delete", "vm1"});
send_command({"delete", "--purge", "vm3"}); // so that mac3 can be reused

send_command({"launch", "--network", fmt::format("id=eth0,mac={}", mac1)}); // repeated mac is rejected
send_command({"launch", "--network", fmt::format("id=eth0,mac={}", mac2)}); // idem
send_command({"launch", "--network", fmt::format("id=eth0,mac={}", mac3)}); // mac is free after purge, so accepted
send_command({"launch", "--network", fmt::format("name=eth0,mac={}", mac1)}); // repeated mac is rejected
send_command({"launch", "--network", fmt::format("name=eth0,mac={}", mac2)}); // idem
send_command(
{"launch", "--network", fmt::format("name=eth0,mac={}", mac3)}); // mac is free after purge, so accepted
}

} // namespace

0 comments on commit 9f27f43

Please sign in to comment.