compute: Improve 'server create --nic' option parsing

Simplify the parsing of this option by making use of a custom action.

Change-Id: I670ff5109522d533ef4e62a79116e49a35c4e8fa
Signed-off-by: Stephen Finucane <sfinucan@redhat.com>

2 files changed, 185 insertions, 140 deletions

diff --git a/openstackclient/compute/v2/server.py b/openstackclient/compute/v2/server.py
index 7e17d0d0..1277c341 100644
--- a/openstackclient/compute/v2/server.py
+++ b/openstackclient/compute/v2/server.py
@@ -98,16 +98,6 @@ def _get_ip_address(addresses, address_type, ip_address_family):
     )
 
 
-def _prefix_checked_value(prefix):
-    def func(value):
-        if ',' in value or '=' in value:
-            msg = _("Invalid argument %s, "
-                    "characters ',' and '=' are not allowed") % value
-            raise argparse.ArgumentTypeError(msg)
-        return prefix + value
-    return func
-
-
 def _prep_server_detail(compute_client, image_client, server, refresh=True):
     """Prepare the detailed server dict for printing
 
@@ -611,6 +601,81 @@ class AddServerVolume(command.Command):
         )
 
 
+# TODO(stephenfin): Replace with 'MultiKeyValueAction' when we no longer
+# support '--nic=auto' and '--nic=none'
+class NICAction(argparse.Action):
+
+    def __init__(
+        self,
+        option_strings,
+        dest,
+        nargs=None,
+        const=None,
+        default=None,
+        type=None,
+        choices=None,
+        required=False,
+        help=None,
+        metavar=None,
+        key=None,
+    ):
+        self.key = key
+        super().__init__(
+            option_strings=option_strings, dest=dest, nargs=nargs, const=const,
+            default=default, type=type, choices=choices, required=required,
+            help=help, metavar=metavar,
+        )
+
+    def __call__(self, parser, namespace, values, option_string=None):
+        # Make sure we have an empty dict rather than None
+        if getattr(namespace, self.dest, None) is None:
+            setattr(namespace, self.dest, [])
+
+        # Handle the special auto/none cases
+        if values in ('auto', 'none'):
+            getattr(namespace, self.dest).append(values)
+            return
+
+        if self.key:
+            if ',' in values or '=' in values:
+                msg = _(
+                    "Invalid argument %s; characters ',' and '=' are not "
+                    "allowed"
+                )
+                raise argparse.ArgumentTypeError(msg % values)
+
+            values = '='.join([self.key, values])
+
+        info = {
+            'net-id': '',
+            'port-id': '',
+            'v4-fixed-ip': '',
+            'v6-fixed-ip': '',
+        }
+
+        for kv_str in values.split(','):
+            k, sep, v = kv_str.partition("=")
+
+            if k not in info or not v:
+                msg = _(
+                    "Invalid argument %s; argument must be of form "
+                    "'net-id=net-uuid,v4-fixed-ip=ip-addr,v6-fixed-ip=ip-addr,"
+                    "port-id=port-uuid'"
+                )
+                raise argparse.ArgumentTypeError(msg % values)
+
+            info[k] = v
+
+        if info['net-id'] and info['port-id']:
+            msg = _(
+                'Invalid argument %s; either network or port should be '
+                'specified but not both'
+            )
+            raise argparse.ArgumentTypeError(msg % values)
+
+        getattr(namespace, self.dest).append(info)
+
+
 class CreateServer(command.ShowOne):
     _description = _("Create a new server")
 
@@ -701,9 +766,10 @@ class CreateServer(command.ShowOne):
         parser.add_argument(
             '--network',
             metavar="<network>",
-            action='append',
-            dest='nic',
-            type=_prefix_checked_value('net-id='),
+            dest='nics',
+            default=[],
+            action=NICAction,
+            key='net-id',
             # NOTE(RuiChen): Add '\n' to the end of line to improve formatting;
             # see cliff's _SmartHelpFormatter for more details.
             help=_(
@@ -719,9 +785,10 @@ class CreateServer(command.ShowOne):
         parser.add_argument(
             '--port',
             metavar="<port>",
-            action='append',
-            dest='nic',
-            type=_prefix_checked_value('port-id='),
+            dest='nics',
+            default=[],
+            action=NICAction,
+            key='port-id',
             help=_(
                 "Create a NIC on the server and connect it to port. "
                 "Specify option multiple times to create multiple NICs. "
@@ -733,21 +800,28 @@ class CreateServer(command.ShowOne):
         )
         parser.add_argument(
             '--nic',
-            metavar="<net-id=net-uuid,v4-fixed-ip=ip-addr,v6-fixed-ip=ip-addr,"
-                    "port-id=port-uuid,auto,none>",
-            action='append',
+            metavar="<net-id=net-uuid,port-id=port-uuid,v4-fixed-ip=ip-addr,"
+                    "v6-fixed-ip=ip-addr,auto,none>",
+            action=NICAction,
+            dest='nics',
+            default=[],
             help=_(
-                "Create a NIC on the server. "
-                "Specify option multiple times to create multiple NICs. "
-                "Either net-id or port-id must be provided, but not both. "
-                "net-id: attach NIC to network with this UUID, "
-                "port-id: attach NIC to port with this UUID, "
-                "v4-fixed-ip: IPv4 fixed address for NIC (optional), "
-                "v6-fixed-ip: IPv6 fixed address for NIC (optional), "
-                "none: (v2.37+) no network is attached, "
+                "Create a NIC on the server.\n"
+                "NIC in the format:\n"
+                "net-id=<net-uuid>: attach NIC to network with this UUID,\n"
+                "port-id=<port-uuid>: attach NIC to port with this UUID,\n"
+                "v4-fixed-ip=<ip-addr>: IPv4 fixed address for NIC (optional),"
+                "\n"
+                "v6-fixed-ip=<ip-addr>: IPv6 fixed address for NIC (optional),"
+                "\n"
+                "none: (v2.37+) no network is attached,\n"
                 "auto: (v2.37+) the compute service will automatically "
-                "allocate a network. Specifying a --nic of auto or none "
-                "cannot be used with any other --nic value."
+                "allocate a network.\n"
+                "\n"
+                "Specify option multiple times to create multiple NICs.\n"
+                "Specifying a --nic of auto or none cannot be used with any "
+                "other --nic value.\n"
+                "Either net-id or port-id must be provided, but not both."
             ),
         )
         parser.add_argument(
@@ -1103,84 +1177,55 @@ class CreateServer(command.ShowOne):
                 raise exceptions.CommandError(msg)
             block_device_mapping_v2.append(mapping)
 
-        nics = []
-        auto_or_none = False
-        if parsed_args.nic is None:
-            parsed_args.nic = []
-        for nic_str in parsed_args.nic:
-            # Handle the special auto/none cases
-            if nic_str in ('auto', 'none'):
-                auto_or_none = True
-                nics.append(nic_str)
-            else:
-                nic_info = {
-                    'net-id': '',
-                    'v4-fixed-ip': '',
-                    'v6-fixed-ip': '',
-                    'port-id': '',
-                }
-                for kv_str in nic_str.split(","):
-                    k, sep, v = kv_str.partition("=")
-                    if k in nic_info and v:
-                        nic_info[k] = v
-                    else:
-                        msg = _(
-                            "Invalid nic argument '%s'. Nic arguments "
-                            "must be of the form --nic <net-id=net-uuid"
-                            ",v4-fixed-ip=ip-addr,v6-fixed-ip=ip-addr,"
-                            "port-id=port-uuid>."
-                        )
-                        raise exceptions.CommandError(msg % k)
+        nics = parsed_args.nics
 
-                if bool(nic_info["net-id"]) == bool(nic_info["port-id"]):
-                    msg = _(
-                        'Either network or port should be specified '
-                        'but not both'
-                    )
-                    raise exceptions.CommandError(msg)
+        if 'auto' in nics or 'none' in nics:
+            if len(nics) > 1:
+                msg = _(
+                    'Specifying a --nic of auto or none cannot '
+                    'be used with any other --nic, --network '
+                    'or --port value.'
+                )
+                raise exceptions.CommandError(msg)
 
+            nics = nics[0]
+        else:
+            for nic in nics:
                 if self.app.client_manager.is_network_endpoint_enabled():
                     network_client = self.app.client_manager.network
-                    if nic_info["net-id"]:
+
+                    if nic['net-id']:
                         net = network_client.find_network(
-                            nic_info["net-id"], ignore_missing=False)
-                        nic_info["net-id"] = net.id
-                    if nic_info["port-id"]:
+                            nic['net-id'], ignore_missing=False,
+                        )
+                        nic['net-id'] = net.id
+
+                    if nic['port-id']:
                         port = network_client.find_port(
-                            nic_info["port-id"], ignore_missing=False)
-                        nic_info["port-id"] = port.id
+                            nic['port-id'], ignore_missing=False,
+                        )
+                        nic['port-id'] = port.id
                 else:
-                    if nic_info["net-id"]:
-                        nic_info["net-id"] = compute_client.api.network_find(
-                            nic_info["net-id"]
+                    if nic['net-id']:
+                        nic['net-id'] = compute_client.api.network_find(
+                            nic['net-id'],
                         )['id']
-                    if nic_info["port-id"]:
+
+                    if nic['port-id']:
                         msg = _(
                             "Can't create server with port specified "
                             "since network endpoint not enabled"
                         )
                         raise exceptions.CommandError(msg)
 
-                nics.append(nic_info)
-
-        if nics:
-            if auto_or_none:
-                if len(nics) > 1:
-                    msg = _(
-                        'Specifying a --nic of auto or none cannot '
-                        'be used with any other --nic, --network '
-                        'or --port value.'
-                    )
-                    raise exceptions.CommandError(msg)
-                nics = nics[0]
-        else:
+        if not nics:
             # Compute API version >= 2.37 requires a value, so default to
             # 'auto' to maintain legacy behavior if a nic wasn't specified.
             if compute_client.api_version >= api_versions.APIVersion('2.37'):
                 nics = 'auto'
             else:
-                # Default to empty list if nothing was specified, let nova
-                # side to decide the default behavior.
+                # Default to empty list if nothing was specified and let nova
+                # decide the default behavior.
                 nics = []
 
         # Check security group exist and convert ID to name
diff --git a/openstackclient/tests/unit/compute/v2/test_server.py b/openstackclient/tests/unit/compute/v2/test_server.py
index 9a01758c..e8db7f59 100644
--- a/openstackclient/tests/unit/compute/v2/test_server.py
+++ b/openstackclient/tests/unit/compute/v2/test_server.py
@@ -1311,8 +1311,28 @@ class TestServerCreate(TestServer):
         verifylist = [
             ('image', 'image1'),
             ('flavor', 'flavor1'),
-            ('nic', ['net-id=net1', 'net-id=net1,v4-fixed-ip=10.0.0.2',
-                     'port-id=port1', 'net-id=net1', 'port-id=port2']),
+            ('nics', [
+                {
+                    'net-id': 'net1', 'port-id': '',
+                    'v4-fixed-ip': '', 'v6-fixed-ip': '',
+                },
+                {
+                    'net-id': 'net1', 'port-id': '',
+                    'v4-fixed-ip': '10.0.0.2', 'v6-fixed-ip': '',
+                },
+                {
+                    'net-id': '', 'port-id': 'port1',
+                    'v4-fixed-ip': '', 'v6-fixed-ip': '',
+                },
+                {
+                    'net-id': 'net1', 'port-id': '',
+                    'v4-fixed-ip': '', 'v6-fixed-ip': '',
+                },
+                {
+                    'net-id': '', 'port-id': 'port2',
+                    'v4-fixed-ip': '', 'v6-fixed-ip': '',
+                },
+            ]),
             ('config_drive', False),
             ('server_name', self.new_server.name),
         ]
@@ -1413,7 +1433,7 @@ class TestServerCreate(TestServer):
         verifylist = [
             ('image', 'image1'),
             ('flavor', 'flavor1'),
-            ('nic', ['auto']),
+            ('nics', ['auto']),
             ('config_drive', False),
             ('server_name', self.new_server.name),
         ]
@@ -1509,7 +1529,7 @@ class TestServerCreate(TestServer):
         verifylist = [
             ('image', 'image1'),
             ('flavor', 'flavor1'),
-            ('nic', ['none']),
+            ('nics', ['none']),
             ('config_drive', False),
             ('server_name', self.new_server.name),
         ]
@@ -1557,7 +1577,14 @@ class TestServerCreate(TestServer):
         verifylist = [
             ('image', 'image1'),
             ('flavor', 'flavor1'),
-            ('nic', ['none', 'auto', 'port-id=port1']),
+            ('nics', [
+                'none',
+                'auto',
+                {
+                    'net-id': '', 'port-id': 'port1',
+                    'v4-fixed-ip': '', 'v6-fixed-ip': '',
+                },
+            ]),
             ('config_drive', False),
             ('server_name', self.new_server.name),
         ]
@@ -1587,17 +1614,10 @@ class TestServerCreate(TestServer):
             '--nic', 'abcdefgh',
             self.new_server.name,
         ]
-        verifylist = [
-            ('image', 'image1'),
-            ('flavor', 'flavor1'),
-            ('nic', ['abcdefgh']),
-            ('config_drive', False),
-            ('server_name', self.new_server.name),
-        ]
-        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
-
-        self.assertRaises(exceptions.CommandError,
-                          self.cmd.take_action, parsed_args)
+        self.assertRaises(
+            argparse.ArgumentTypeError,
+            self.check_parser,
+            self.cmd, arglist, [])
         self.assertNotCalled(self.servers_mock.create)
 
     def test_server_create_with_invalid_network_key(self):
@@ -1607,17 +1627,10 @@ class TestServerCreate(TestServer):
             '--nic', 'abcdefgh=12324',
             self.new_server.name,
         ]
-        verifylist = [
-            ('image', 'image1'),
-            ('flavor', 'flavor1'),
-            ('nic', ['abcdefgh=12324']),
-            ('config_drive', False),
-            ('server_name', self.new_server.name),
-        ]
-        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
-
-        self.assertRaises(exceptions.CommandError,
-                          self.cmd.take_action, parsed_args)
+        self.assertRaises(
+            argparse.ArgumentTypeError,
+            self.check_parser,
+            self.cmd, arglist, [])
         self.assertNotCalled(self.servers_mock.create)
 
     def test_server_create_with_empty_network_key_value(self):
@@ -1627,17 +1640,10 @@ class TestServerCreate(TestServer):
             '--nic', 'net-id=',
             self.new_server.name,
         ]
-        verifylist = [
-            ('image', 'image1'),
-            ('flavor', 'flavor1'),
-            ('nic', ['net-id=']),
-            ('config_drive', False),
-            ('server_name', self.new_server.name),
-        ]
-        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
-
-        self.assertRaises(exceptions.CommandError,
-                          self.cmd.take_action, parsed_args)
+        self.assertRaises(
+            argparse.ArgumentTypeError,
+            self.check_parser,
+            self.cmd, arglist, [])
         self.assertNotCalled(self.servers_mock.create)
 
     def test_server_create_with_only_network_key(self):
@@ -1647,17 +1653,11 @@ class TestServerCreate(TestServer):
             '--nic', 'net-id',
             self.new_server.name,
         ]
-        verifylist = [
-            ('image', 'image1'),
-            ('flavor', 'flavor1'),
-            ('nic', ['net-id']),
-            ('config_drive', False),
-            ('server_name', self.new_server.name),
-        ]
-        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+        self.assertRaises(
+            argparse.ArgumentTypeError,
+            self.check_parser,
+            self.cmd, arglist, [])
 
-        self.assertRaises(exceptions.CommandError,
-                          self.cmd.take_action, parsed_args)
         self.assertNotCalled(self.servers_mock.create)
 
     @mock.patch.object(common_utils, 'wait_for_status', return_value=True)
@@ -2230,7 +2230,7 @@ class TestServerCreate(TestServer):
         verifylist = [
             ('image_properties', {'hypervisor_type': 'qemu'}),
             ('flavor', 'flavor1'),
-            ('nic', ['none']),
+            ('nics', ['none']),
             ('config_drive', False),
             ('server_name', self.new_server.name),
         ]
@@ -2286,7 +2286,7 @@ class TestServerCreate(TestServer):
             ('image_properties', {'hypervisor_type': 'qemu',
              'hw_disk_bus': 'ide'}),
             ('flavor', 'flavor1'),
-            ('nic', ['none']),
+            ('nics', ['none']),
             ('config_drive', False),
             ('server_name', self.new_server.name),
         ]
@@ -2342,7 +2342,7 @@ class TestServerCreate(TestServer):
             ('image_properties', {'hypervisor_type': 'qemu',
              'hw_disk_bus': 'virtio'}),
             ('flavor', 'flavor1'),
-            ('nic', ['none']),
+            ('nics', ['none']),
             ('config_drive', False),
             ('server_name', self.new_server.name),
         ]
@@ -2374,7 +2374,7 @@ class TestServerCreate(TestServer):
             ('image_properties',
                 {'owner_specified.openstack.object': 'image/cirros'}),
             ('flavor', 'flavor1'),
-            ('nic', ['none']),
+            ('nics', ['none']),
             ('server_name', self.new_server.name),
         ]
         # create a image_info as the side_effect of the fake image_list()