Merge "network: Accept name as positional arg for FWaaS create commands"

Author: Zuul

openstackclient/network/v2/fwaas/group.py

@@ -60,7 +60,6 @@ def human_readable(self) -> str:
 def _get_common_parser(
     parser: argparse.ArgumentParser,
 ) -> argparse.ArgumentParser:
-    parser.add_argument('--name', help=_('Name for the firewall group'))
     parser.add_argument(
         '--description',
         metavar='<description>',
@@ -184,6 +183,23 @@ class CreateFirewallGroup(command.ShowOne):
     def get_parser(self, prog_name: str) -> argparse.ArgumentParser:
         parser = super().get_parser(prog_name)
         _get_common_parser(parser)
+        # TODO(slaweq): Remove the deprecated --name option and make the
+        # positional name argument required (remove nargs='?') once the
+        # deprecation period is over.
+        parser.add_argument(
+            'positional_name',
+            nargs='?',
+            metavar='<name>',
+            default=None,
+            help=_('Name for the firewall group'),
+        )
+        parser.add_argument(
+            '--name',
+            help=_(
+                '(Deprecated, please pass name as a positional argument) '
+                'Name for the firewall group'
+            ),
+        )
         identity_utils.add_project_owner_option_to_parser(parser)
         port_group = parser.add_mutually_exclusive_group()
         port_group.add_argument(
@@ -207,6 +223,22 @@ def take_action(
         self, parsed_args: argparse.Namespace
     ) -> tuple[Sequence[str], Iterable[Any]]:
         client = self.app.client_manager.network
+        # TODO(slaweq): Remove this --name deprecation handling once the
+        # deprecation period is over.
+        if parsed_args.positional_name and parsed_args.name:
+            msg = _(
+                "Cannot specify name as both a positional argument "
+                "and with the --name option."
+            )
+            raise exceptions.CommandError(msg)
+        if parsed_args.name:
+            LOG.warning(
+                'The --name option is deprecated for the "firewall group '
+                'create" command, please pass the name as a positional '
+                'argument instead.'
+            )
+        elif parsed_args.positional_name:
+            parsed_args.name = parsed_args.positional_name
         attrs = _get_common_attrs(self.app.client_manager, parsed_args)
         if 'project' in parsed_args and parsed_args.project is not None:
             attrs['project_id'] = identity_common.find_project(
@@ -300,6 +332,7 @@ def get_parser(self, prog_name: str) -> argparse.ArgumentParser:
             metavar='<firewall-group>',
             help=_('Firewall group to update (name or ID)'),
         )
+        parser.add_argument('--name', help=_('Name for the firewall group'))
         parser.add_argument(
             '--port',
             metavar='<port>',

openstackclient/network/v2/fwaas/rule.py

@@ -74,9 +74,6 @@ def _convert_to_lowercase(string: str) -> str:
 def _get_common_parser(
     parser: argparse.ArgumentParser,
 ) -> argparse.ArgumentParser:
-    parser.add_argument(
-        '--name', metavar='<name>', help=_('Name of the firewall rule')
-    )
     parser.add_argument(
         '--description',
         metavar='<description>',
@@ -288,13 +285,47 @@ class CreateFirewallRule(command.ShowOne):
     def get_parser(self, prog_name: str) -> argparse.ArgumentParser:
         parser = super().get_parser(prog_name)
         _get_common_parser(parser)
+        # TODO(slaweq): Remove the deprecated --name option and make the
+        # positional name argument required (remove nargs='?') once the
+        # deprecation period is over.
+        parser.add_argument(
+            'positional_name',
+            nargs='?',
+            metavar='<name>',
+            default=None,
+            help=_('Name of the firewall rule'),
+        )
+        parser.add_argument(
+            '--name',
+            metavar='<name>',
+            help=_(
+                '(Deprecated, please pass name as a positional argument) '
+                'Name of the firewall rule'
+            ),
+        )
         identity_utils.add_project_owner_option_to_parser(parser)
         return parser
 
     def take_action(
         self, parsed_args: argparse.Namespace
     ) -> tuple[Sequence[str], Iterable[Any]]:
         client = self.app.client_manager.network
+        # TODO(slaweq): Remove this --name deprecation handling once the
+        # deprecation period is over.
+        if parsed_args.positional_name and parsed_args.name:
+            msg = _(
+                "Cannot specify name as both a positional argument "
+                "and with the --name option."
+            )
+            raise exceptions.CommandError(msg)
+        if parsed_args.name:
+            LOG.warning(
+                'The --name option is deprecated for the "firewall group '
+                'rule create" command, please pass the name as a positional '
+                'argument instead.'
+            )
+        elif parsed_args.positional_name:
+            parsed_args.name = parsed_args.positional_name
         attrs = _get_common_attrs(self.app.client_manager, parsed_args)
         if 'project' in parsed_args and parsed_args.project is not None:
             attrs['project_id'] = identity_common.find_project(
@@ -415,6 +446,11 @@ def get_parser(self, prog_name: str) -> argparse.ArgumentParser:
             metavar='<firewall-rule>',
             help=_('Firewall rule to set (name or ID)'),
         )
+        parser.add_argument(
+            '--name',
+            metavar='<name>',
+            help=_('Name of the firewall rule'),
+        )
         return parser
 
     def take_action(self, parsed_args: argparse.Namespace) -> None:

openstackclient/tests/unit/network/v2/fwaas/test_group.py

@@ -32,6 +32,7 @@
     'egress_firewall_policy': 'egress_firewall_policy_id',
     'no_ingress_firewall_policy': 'ingress_firewall_policy_id',
     'no_egress_firewall_policy': 'egress_firewall_policy_id',
+    'positional_name': 'name',
     'project': 'project_id',
     'port': 'ports',
 }
@@ -247,7 +248,6 @@ def _mock_find(*args, **kwargs):
         self.network_client.find_port.side_effect = _mock_find
         project_id = 'my-project'
         arglist = [
-            '--name',
             name,
             '--description',
             description,
@@ -263,7 +263,7 @@ def _mock_find(*args, **kwargs):
             '--disable',
         ]
         verifylist = [
-            ('name', name),
+            ('positional_name', name),
             ('description', description),
             ('ingress_firewall_policy', ingress_policy),
             ('egress_firewall_policy', egress_policy),
@@ -279,6 +279,32 @@ def _mock_find(*args, **kwargs):
 
         self.check_results(headers, data, request)
 
+    def test_create_with_name_option_deprecated(self):
+        name = 'my-name'
+        arglist = ['--name', name]
+        verifylist = [('name', name)]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+        with mock.patch.object(fwaas_group.LOG, 'warning') as mock_warning:
+            headers, _data = self.cmd.take_action(parsed_args)
+            mock_warning.assert_called_once_with(
+                'The --name option is deprecated for the "firewall group '
+                'create" command, please pass the name as a positional '
+                'argument instead.'
+            )
+        self.assertEqual(self.ordered_headers, tuple(sorted(headers)))
+
+    def test_create_with_both_positional_and_option_name(self):
+        name = 'my-name'
+        arglist = [name, '--name', 'other-name']
+        verifylist = [
+            ('positional_name', name),
+            ('name', 'other-name'),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+        self.assertRaises(
+            exceptions.CommandError, self.cmd.take_action, parsed_args
+        )
+
     def test_create_with_shared_and_no_share(self):
         arglist = [
             '--share',

openstackclient/tests/unit/network/v2/fwaas/test_rule.py

@@ -29,6 +29,7 @@
 
 
 CONVERT_MAP = {
+    'positional_name': 'name',
     'project': 'project_id',
 }
 
@@ -222,10 +223,9 @@ def _set_all_params(self, args={}):
         destination_port = args.get('destination_port') or '0:65535'
         project_id = args.get('project_id') or 'my-tenant'
         arglist = [
+            name,
             '--description',
             description,
-            '--name',
-            name,
             '--protocol',
             protocol,
             '--ip-version',
@@ -247,7 +247,7 @@ def _set_all_params(self, args={}):
         ]
 
         verifylist = [
-            ('name', name),
+            ('positional_name', name),
             ('description', description),
             ('shared', True),
             ('protocol', protocol),
@@ -320,6 +320,31 @@ def test_create_with_all_params_protocol_upper_capitalized(self):
                 verifylist,
             )
 
+    def test_create_with_name_option_deprecated(self):
+        name = 'my-name'
+        arglist = ['--name', name]
+        verifylist = [('name', name)]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+        with mock.patch.object(fwaas_rule.LOG, 'warning') as mock_warning:
+            _headers, _data = self.cmd.take_action(parsed_args)
+            mock_warning.assert_called_once_with(
+                'The --name option is deprecated for the "firewall group '
+                'rule create" command, please pass the name as a positional '
+                'argument instead.'
+            )
+
+    def test_create_with_both_positional_and_option_name(self):
+        name = 'my-name'
+        arglist = [name, '--name', 'other-name']
+        verifylist = [
+            ('positional_name', name),
+            ('name', 'other-name'),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+        self.assertRaises(
+            exceptions.CommandError, self.cmd.take_action, parsed_args
+        )
+
 
 class TestListFirewallRule(TestFirewallRule):
     def _setup_summary(self, expect=None):