From 0d57f3f367163dc4d3b57ae574a825563494f133 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Thu, 16 Feb 2023 19:12:31 +0000 Subject: [PATCH] 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 --- .../tests/unit/volume/v3/test_volume_group.py | 55 ++++++-- openstackclient/volume/v3/volume_group.py | 127 +++++++++++++----- ...positional-arguments-89f6b886c0f1f2b5.yaml | 10 ++ 3 files changed, 148 insertions(+), 44 deletions(-) create mode 100644 releasenotes/notes/deprecate-volume-group-create-positional-arguments-89f6b886c0f1f2b5.yaml diff --git a/openstackclient/tests/unit/volume/v3/test_volume_group.py b/openstackclient/tests/unit/volume/v3/test_volume_group.py index a8338a8007..78717de851 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( - ' 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 69b18cebf4..242ffcd49f 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 \ + # [...] + # + # and the modern approach + # + # volume group create \ + # --volume-group-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 as + # both a positional and an option argument and another to ensure users + # don't pass 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='', 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='', nargs='*', default=[], - help=_('Name or ID of volume type(s) to use.'), + help=argparse.SUPPRESS, + ) + source_parser.add_argument( + '--volume-group-type', + metavar='', + help=_('Volume group type to use (name or ID)'), + ) + volume_types_parser.add_argument( + '--volume-type', + metavar='', + 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='', + 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='', + 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='', - help=_('Availability zone for volume group. ' - '(not available if creating group from source)'), - ) - source_parser.add_argument( - '--source-group', - metavar='', - help=_('Existing volume group (name or ID) ' - '(supported by --os-volume-api-version 3.14 or later)'), - ) - source_parser.add_argument( - '--group-snapshot', - metavar='', - 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 = _( - " 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 0000000000..ee3a684377 --- /dev/null +++ b/releasenotes/notes/deprecate-volume-group-create-positional-arguments-89f6b886c0f1f2b5.yaml @@ -0,0 +1,10 @@ +--- +deprecations: + - | + The ```` and `` [...]`` + 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-type [--volume-type ...]