Deprecate positional args for 'volume group create'

There are now many ways to create a new volume group, thus the
positional arguments don't make sense. Deprecate them.

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

3 files changed, 148 insertions, 44 deletions

diff --git a/openstackclient/tests/unit/volume/v3/test_volume_group.py b/openstackclient/tests/unit/volume/v3/test_volume_group.py
index a8338a80..78717de8 100644
--- a/openstackclient/tests/unit/volume/v3/test_volume_group.py
+++ b/openstackclient/tests/unit/volume/v3/test_volume_group.py
@@ -100,8 +100,8 @@ class TestVolumeGroupCreate(TestVolumeGroup):
             api_versions.APIVersion('3.13')
 
         arglist = [
-            self.fake_volume_group_type.id,
-            self.fake_volume_type.id,
+            '--volume-group-type', self.fake_volume_group_type.id,
+            '--volume-type', self.fake_volume_type.id,
         ]
         verifylist = [
             ('volume_group_type', self.fake_volume_group_type.id),
@@ -128,12 +128,51 @@ class TestVolumeGroupCreate(TestVolumeGroup):
         self.assertEqual(self.columns, columns)
         self.assertCountEqual(self.data, data)
 
+    def test_volume_group_create__legacy(self):
+        self.app.client_manager.volume.api_version = \
+            api_versions.APIVersion('3.13')
+
+        arglist = [
+            self.fake_volume_group_type.id,
+            self.fake_volume_type.id,
+        ]
+        verifylist = [
+            ('volume_group_type_legacy', self.fake_volume_group_type.id),
+            ('volume_types_legacy', [self.fake_volume_type.id]),
+            ('name', None),
+            ('description', None),
+            ('availability_zone', None),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        with mock.patch.object(self.cmd.log, 'warning') as mock_warning:
+            columns, data = self.cmd.take_action(parsed_args)
+
+        self.volume_group_types_mock.get.assert_called_once_with(
+            self.fake_volume_group_type.id)
+        self.volume_types_mock.get.assert_called_once_with(
+            self.fake_volume_type.id)
+        self.volume_groups_mock.create.assert_called_once_with(
+            self.fake_volume_group_type.id,
+            self.fake_volume_type.id,
+            None,
+            None,
+            availability_zone=None,
+        )
+        self.assertEqual(self.columns, columns)
+        self.assertCountEqual(self.data, data)
+        mock_warning.assert_called_once()
+        self.assertIn(
+            'Passing volume group type and volume types as positional ',
+            str(mock_warning.call_args[0][0]),
+        )
+
     def test_volume_group_create_no_volume_type(self):
         self.app.client_manager.volume.api_version = \
             api_versions.APIVersion('3.13')
 
         arglist = [
-            self.fake_volume_group_type.id
+            '--volume-group-type', self.fake_volume_group_type.id,
         ]
         verifylist = [
             ('volume_group_type', self.fake_volume_group_type.id),
@@ -148,7 +187,7 @@ class TestVolumeGroupCreate(TestVolumeGroup):
             self.cmd.take_action,
             parsed_args)
         self.assertIn(
-            '<volume_types> is a required argument',
+            '--volume-types is a required argument when creating ',
             str(exc))
 
     def test_volume_group_create_with_options(self):
@@ -156,8 +195,8 @@ class TestVolumeGroupCreate(TestVolumeGroup):
             api_versions.APIVersion('3.13')
 
         arglist = [
-            self.fake_volume_group_type.id,
-            self.fake_volume_type.id,
+            '--volume-group-type', self.fake_volume_group_type.id,
+            '--volume-type', self.fake_volume_type.id,
             '--name', 'foo',
             '--description', 'hello, world',
             '--availability-zone', 'bar',
@@ -192,8 +231,8 @@ class TestVolumeGroupCreate(TestVolumeGroup):
             api_versions.APIVersion('3.12')
 
         arglist = [
-            self.fake_volume_group_type.id,
-            self.fake_volume_type.id,
+            '--volume-group-type', self.fake_volume_group_type.id,
+            '--volume-type', self.fake_volume_type.id,
         ]
         verifylist = [
             ('volume_group_type', self.fake_volume_group_type.id),
diff --git a/openstackclient/volume/v3/volume_group.py b/openstackclient/volume/v3/volume_group.py
index 69b18ceb..242ffcd4 100644
--- a/openstackclient/volume/v3/volume_group.py
+++ b/openstackclient/volume/v3/volume_group.py
@@ -10,7 +10,7 @@
 # License for the specific language governing permissions and limitations
 # under the License.
 
-import logging
+import argparse
 
 from cinderclient import api_versions
 from osc_lib.command import command
@@ -19,8 +19,6 @@ from osc_lib import utils
 
 from openstackclient.i18n import _
 
-LOG = logging.getLogger(__name__)
-
 
 def _format_group(group):
     columns = (
@@ -82,19 +80,72 @@ class CreateVolumeGroup(command.ShowOne):
 
     def get_parser(self, prog_name):
         parser = super().get_parser(prog_name)
+        # This is a bit complicated. We accept two patterns: a legacy pattern
+        #
+        #   volume group create \
+        #     <volume-group-type> <volume-type> [<volume-type>...]
+        #
+        # and the modern approach
+        #
+        #   volume group create \
+        #     --volume-group-type <volume-group-type>
+        #     --volume-type <volume-type>
+        #    [--volume-type <volume-type> ...]
+        #
+        # Because argparse doesn't properly support nested exclusive groups, we
+        # use two groups: one to ensure users don't pass <volume-group-type> as
+        # both a positional and an option argument and another to ensure users
+        # don't pass <volume-type> this way. It's a bit weird but it catches
+        # everything we care about.
         source_parser = parser.add_mutually_exclusive_group()
+        # we use a different name purely so we can issue a deprecation warning
         source_parser.add_argument(
-            'volume_group_type',
+            'volume_group_type_legacy',
             metavar='<volume_group_type>',
             nargs='?',
-            help=_('Name or ID of volume group type to use.'),
+            help=argparse.SUPPRESS,
         )
-        parser.add_argument(
-            'volume_types',
+        volume_types_parser = parser.add_mutually_exclusive_group()
+        # We need to use a separate dest
+        # https://github.com/python/cpython/issues/101990
+        volume_types_parser.add_argument(
+            'volume_types_legacy',
             metavar='<volume_type>',
             nargs='*',
             default=[],
-            help=_('Name or ID of volume type(s) to use.'),
+            help=argparse.SUPPRESS,
+        )
+        source_parser.add_argument(
+            '--volume-group-type',
+            metavar='<volume_group_type>',
+            help=_('Volume group type to use (name or ID)'),
+        )
+        volume_types_parser.add_argument(
+            '--volume-type',
+            metavar='<volume_type>',
+            dest='volume_types',
+            action='append',
+            default=[],
+            help=_(
+                'Volume type(s) to use (name or ID) '
+                '(required with --volume-group-type)'
+            ),
+        )
+        source_parser.add_argument(
+            '--source-group',
+            metavar='<source-group>',
+            help=_(
+                'Existing volume group to use (name or ID) '
+                '(supported by --os-volume-api-version 3.14 or later)'
+            ),
+        )
+        source_parser.add_argument(
+            '--group-snapshot',
+            metavar='<group-snapshot>',
+            help=_(
+                'Existing group snapshot to use (name or ID) '
+                '(supported by --os-volume-api-version 3.14 or later)'
+            ),
         )
         parser.add_argument(
             '--name',
@@ -109,59 +160,63 @@ class CreateVolumeGroup(command.ShowOne):
         parser.add_argument(
             '--availability-zone',
             metavar='<availability-zone>',
-            help=_('Availability zone for volume group. '
-                   '(not available if creating group from source)'),
-        )
-        source_parser.add_argument(
-            '--source-group',
-            metavar='<source-group>',
-            help=_('Existing volume group (name or ID) '
-                   '(supported by --os-volume-api-version 3.14 or later)'),
-        )
-        source_parser.add_argument(
-            '--group-snapshot',
-            metavar='<group-snapshot>',
-            help=_('Existing group snapshot (name or ID) '
-                   '(supported by --os-volume-api-version 3.14 or later)'),
+            help=_(
+                'Availability zone for volume group. '
+                '(not available if creating group from source)'
+            ),
         )
         return parser
 
     def take_action(self, parsed_args):
         volume_client = self.app.client_manager.volume
 
-        if parsed_args.volume_group_type:
+        if parsed_args.volume_group_type_legacy:
+            msg = _(
+                "Passing volume group type and volume types as positional "
+                "arguments is deprecated. Use the --volume-group-type and "
+                "--volume-type option arguments instead."
+            )
+            self.log.warning(msg)
+
+        volume_group_type = parsed_args.volume_group_type or \
+            parsed_args.volume_group_type_legacy
+        volume_types = parsed_args.volume_types[:]
+        volume_types.extend(parsed_args.volume_types_legacy)
+
+        if volume_group_type:
             if volume_client.api_version < api_versions.APIVersion('3.13'):
                 msg = _(
                     "--os-volume-api-version 3.13 or greater is required to "
                     "support the 'volume group create' command"
                 )
                 raise exceptions.CommandError(msg)
-            if not parsed_args.volume_types:
+            if not volume_types:
                 msg = _(
-                    "<volume_types> is a required argument when creating a "
+                    "--volume-types is a required argument when creating a "
                     "group from group type."
                 )
                 raise exceptions.CommandError(msg)
 
-            volume_group_type = utils.find_resource(
+            volume_group_type_id = utils.find_resource(
                 volume_client.group_types,
-                parsed_args.volume_group_type,
-            )
-            volume_types = []
-            for volume_type in parsed_args.volume_types:
-                volume_types.append(
+                volume_group_type,
+            ).id
+            volume_types_ids = []
+            for volume_type in volume_types:
+                volume_types_ids.append(
                     utils.find_resource(
                         volume_client.volume_types,
                         volume_type,
-                    )
+                    ).id
                 )
 
             group = volume_client.groups.create(
-                volume_group_type.id,
-                ','.join(x.id for x in volume_types),
+                volume_group_type_id,
+                ','.join(volume_types_ids),
                 parsed_args.name,
                 parsed_args.description,
-                availability_zone=parsed_args.availability_zone)
+                availability_zone=parsed_args.availability_zone,
+            )
 
             group = volume_client.groups.get(group.id)
             return _format_group(group)
@@ -186,7 +241,7 @@ class CreateVolumeGroup(command.ShowOne):
             if parsed_args.availability_zone:
                 msg = _("'--availability-zone' option will not work "
                         "if creating group from source.")
-                LOG.warning(msg)
+                self.log.warning(msg)
 
             source_group = None
             if parsed_args.source_group:
diff --git a/releasenotes/notes/deprecate-volume-group-create-positional-arguments-89f6b886c0f1f2b5.yaml b/releasenotes/notes/deprecate-volume-group-create-positional-arguments-89f6b886c0f1f2b5.yaml
new file mode 100644
index 00000000..ee3a6843
--- /dev/null
+++ b/releasenotes/notes/deprecate-volume-group-create-positional-arguments-89f6b886c0f1f2b5.yaml
@@ -0,0 +1,10 @@
+---
+deprecations:
+  - |
+    The ``<volume-group-type>`` and ``<volume-type> [<volume-type>...]``
+    positional arguments for the ``volume group create`` command have been
+    deprecated in favour of option arguments. For example::
+
+        openstack volume group create \
+          --volume-group-type <volume-group-type>
+          --volume-type <volume-type> [--volume-type <volume-type> ...]