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(
- '<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 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 \
+ # <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 0000000000..ee3a684377
--- /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> ...]