Skip to content

Commit 365a7a2

Browse files
Zuulopenstack-gerrit
Zuul
authored and
openstack-gerrit
committed
Merge "Deprecate positional args for 'volume group create'"
2 parents 36ba614 + 0d57f3f commit 365a7a2

File tree

3 files changed

+148
-44
lines changed

3 files changed

+148
-44
lines changed

‎openstackclient/tests/unit/volume/v3/test_volume_group.py‎

Lines changed: 47 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -100,8 +100,8 @@ def test_volume_group_create(self):
100100
api_versions.APIVersion('3.13')
101101

102102
arglist = [
103-
self.fake_volume_group_type.id,
104-
self.fake_volume_type.id,
103+
'--volume-group-type', self.fake_volume_group_type.id,
104+
'--volume-type', self.fake_volume_type.id,
105105
]
106106
verifylist = [
107107
('volume_group_type', self.fake_volume_group_type.id),
@@ -128,12 +128,51 @@ def test_volume_group_create(self):
128128
self.assertEqual(self.columns, columns)
129129
self.assertCountEqual(self.data, data)
130130

131+
def test_volume_group_create__legacy(self):
132+
self.app.client_manager.volume.api_version = \
133+
api_versions.APIVersion('3.13')
134+
135+
arglist = [
136+
self.fake_volume_group_type.id,
137+
self.fake_volume_type.id,
138+
]
139+
verifylist = [
140+
('volume_group_type_legacy', self.fake_volume_group_type.id),
141+
('volume_types_legacy', [self.fake_volume_type.id]),
142+
('name', None),
143+
('description', None),
144+
('availability_zone', None),
145+
]
146+
parsed_args = self.check_parser(self.cmd, arglist, verifylist)
147+
148+
with mock.patch.object(self.cmd.log, 'warning') as mock_warning:
149+
columns, data = self.cmd.take_action(parsed_args)
150+
151+
self.volume_group_types_mock.get.assert_called_once_with(
152+
self.fake_volume_group_type.id)
153+
self.volume_types_mock.get.assert_called_once_with(
154+
self.fake_volume_type.id)
155+
self.volume_groups_mock.create.assert_called_once_with(
156+
self.fake_volume_group_type.id,
157+
self.fake_volume_type.id,
158+
None,
159+
None,
160+
availability_zone=None,
161+
)
162+
self.assertEqual(self.columns, columns)
163+
self.assertCountEqual(self.data, data)
164+
mock_warning.assert_called_once()
165+
self.assertIn(
166+
'Passing volume group type and volume types as positional ',
167+
str(mock_warning.call_args[0][0]),
168+
)
169+
131170
def test_volume_group_create_no_volume_type(self):
132171
self.app.client_manager.volume.api_version = \
133172
api_versions.APIVersion('3.13')
134173

135174
arglist = [
136-
self.fake_volume_group_type.id
175+
'--volume-group-type', self.fake_volume_group_type.id,
137176
]
138177
verifylist = [
139178
('volume_group_type', self.fake_volume_group_type.id),
@@ -148,16 +187,16 @@ def test_volume_group_create_no_volume_type(self):
148187
self.cmd.take_action,
149188
parsed_args)
150189
self.assertIn(
151-
'<volume_types> is a required argument',
190+
'--volume-types is a required argument when creating ',
152191
str(exc))
153192

154193
def test_volume_group_create_with_options(self):
155194
self.app.client_manager.volume.api_version = \
156195
api_versions.APIVersion('3.13')
157196

158197
arglist = [
159-
self.fake_volume_group_type.id,
160-
self.fake_volume_type.id,
198+
'--volume-group-type', self.fake_volume_group_type.id,
199+
'--volume-type', self.fake_volume_type.id,
161200
'--name', 'foo',
162201
'--description', 'hello, world',
163202
'--availability-zone', 'bar',
@@ -192,8 +231,8 @@ def test_volume_group_create_pre_v313(self):
192231
api_versions.APIVersion('3.12')
193232

194233
arglist = [
195-
self.fake_volume_group_type.id,
196-
self.fake_volume_type.id,
234+
'--volume-group-type', self.fake_volume_group_type.id,
235+
'--volume-type', self.fake_volume_type.id,
197236
]
198237
verifylist = [
199238
('volume_group_type', self.fake_volume_group_type.id),

‎openstackclient/volume/v3/volume_group.py‎

Lines changed: 91 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
# License for the specific language governing permissions and limitations
1111
# under the License.
1212

13-
import logging
13+
import argparse
1414

1515
from cinderclient import api_versions
1616
from osc_lib.command import command
@@ -19,8 +19,6 @@
1919

2020
from openstackclient.i18n import _
2121

22-
LOG = logging.getLogger(__name__)
23-
2422

2523
def _format_group(group):
2624
columns = (
@@ -82,19 +80,72 @@ class CreateVolumeGroup(command.ShowOne):
8280

8381
def get_parser(self, prog_name):
8482
parser = super().get_parser(prog_name)
83+
# This is a bit complicated. We accept two patterns: a legacy pattern
84+
#
85+
# volume group create \
86+
# <volume-group-type> <volume-type> [<volume-type>...]
87+
#
88+
# and the modern approach
89+
#
90+
# volume group create \
91+
# --volume-group-type <volume-group-type>
92+
# --volume-type <volume-type>
93+
# [--volume-type <volume-type> ...]
94+
#
95+
# Because argparse doesn't properly support nested exclusive groups, we
96+
# use two groups: one to ensure users don't pass <volume-group-type> as
97+
# both a positional and an option argument and another to ensure users
98+
# don't pass <volume-type> this way. It's a bit weird but it catches
99+
# everything we care about.
85100
source_parser = parser.add_mutually_exclusive_group()
101+
# we use a different name purely so we can issue a deprecation warning
86102
source_parser.add_argument(
87-
'volume_group_type',
103+
'volume_group_type_legacy',
88104
metavar='<volume_group_type>',
89105
nargs='?',
90-
help=_('Name or ID of volume group type to use.'),
106+
help=argparse.SUPPRESS,
91107
)
92-
parser.add_argument(
93-
'volume_types',
108+
volume_types_parser = parser.add_mutually_exclusive_group()
109+
# We need to use a separate dest
110+
# https://github.com/python/cpython/issues/101990
111+
volume_types_parser.add_argument(
112+
'volume_types_legacy',
94113
metavar='<volume_type>',
95114
nargs='*',
96115
default=[],
97-
help=_('Name or ID of volume type(s) to use.'),
116+
help=argparse.SUPPRESS,
117+
)
118+
source_parser.add_argument(
119+
'--volume-group-type',
120+
metavar='<volume_group_type>',
121+
help=_('Volume group type to use (name or ID)'),
122+
)
123+
volume_types_parser.add_argument(
124+
'--volume-type',
125+
metavar='<volume_type>',
126+
dest='volume_types',
127+
action='append',
128+
default=[],
129+
help=_(
130+
'Volume type(s) to use (name or ID) '
131+
'(required with --volume-group-type)'
132+
),
133+
)
134+
source_parser.add_argument(
135+
'--source-group',
136+
metavar='<source-group>',
137+
help=_(
138+
'Existing volume group to use (name or ID) '
139+
'(supported by --os-volume-api-version 3.14 or later)'
140+
),
141+
)
142+
source_parser.add_argument(
143+
'--group-snapshot',
144+
metavar='<group-snapshot>',
145+
help=_(
146+
'Existing group snapshot to use (name or ID) '
147+
'(supported by --os-volume-api-version 3.14 or later)'
148+
),
98149
)
99150
parser.add_argument(
100151
'--name',
@@ -109,59 +160,63 @@ def get_parser(self, prog_name):
109160
parser.add_argument(
110161
'--availability-zone',
111162
metavar='<availability-zone>',
112-
help=_('Availability zone for volume group. '
113-
'(not available if creating group from source)'),
114-
)
115-
source_parser.add_argument(
116-
'--source-group',
117-
metavar='<source-group>',
118-
help=_('Existing volume group (name or ID) '
119-
'(supported by --os-volume-api-version 3.14 or later)'),
120-
)
121-
source_parser.add_argument(
122-
'--group-snapshot',
123-
metavar='<group-snapshot>',
124-
help=_('Existing group snapshot (name or ID) '
125-
'(supported by --os-volume-api-version 3.14 or later)'),
163+
help=_(
164+
'Availability zone for volume group. '
165+
'(not available if creating group from source)'
166+
),
126167
)
127168
return parser
128169

129170
def take_action(self, parsed_args):
130171
volume_client = self.app.client_manager.volume
131172

132-
if parsed_args.volume_group_type:
173+
if parsed_args.volume_group_type_legacy:
174+
msg = _(
175+
"Passing volume group type and volume types as positional "
176+
"arguments is deprecated. Use the --volume-group-type and "
177+
"--volume-type option arguments instead."
178+
)
179+
self.log.warning(msg)
180+
181+
volume_group_type = parsed_args.volume_group_type or \
182+
parsed_args.volume_group_type_legacy
183+
volume_types = parsed_args.volume_types[:]
184+
volume_types.extend(parsed_args.volume_types_legacy)
185+
186+
if volume_group_type:
133187
if volume_client.api_version < api_versions.APIVersion('3.13'):
134188
msg = _(
135189
"--os-volume-api-version 3.13 or greater is required to "
136190
"support the 'volume group create' command"
137191
)
138192
raise exceptions.CommandError(msg)
139-
if not parsed_args.volume_types:
193+
if not volume_types:
140194
msg = _(
141-
"<volume_types> is a required argument when creating a "
195+
"--volume-types is a required argument when creating a "
142196
"group from group type."
143197
)
144198
raise exceptions.CommandError(msg)
145199

146-
volume_group_type = utils.find_resource(
200+
volume_group_type_id = utils.find_resource(
147201
volume_client.group_types,
148-
parsed_args.volume_group_type,
149-
)
150-
volume_types = []
151-
for volume_type in parsed_args.volume_types:
152-
volume_types.append(
202+
volume_group_type,
203+
).id
204+
volume_types_ids = []
205+
for volume_type in volume_types:
206+
volume_types_ids.append(
153207
utils.find_resource(
154208
volume_client.volume_types,
155209
volume_type,
156-
)
210+
).id
157211
)
158212

159213
group = volume_client.groups.create(
160-
volume_group_type.id,
161-
','.join(x.id for x in volume_types),
214+
volume_group_type_id,
215+
','.join(volume_types_ids),
162216
parsed_args.name,
163217
parsed_args.description,
164-
availability_zone=parsed_args.availability_zone)
218+
availability_zone=parsed_args.availability_zone,
219+
)
165220

166221
group = volume_client.groups.get(group.id)
167222
return _format_group(group)
@@ -186,7 +241,7 @@ def take_action(self, parsed_args):
186241
if parsed_args.availability_zone:
187242
msg = _("'--availability-zone' option will not work "
188243
"if creating group from source.")
189-
LOG.warning(msg)
244+
self.log.warning(msg)
190245

191246
source_group = None
192247
if parsed_args.source_group:

‎releasenotes/notes/deprecate-volume-group-create-positional-arguments-89f6b886c0f1f2b5.yaml‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
---
2+
deprecations:
3+
- |
4+
The ``<volume-group-type>`` and ``<volume-type> [<volume-type>...]``
5+
positional arguments for the ``volume group create`` command have been
6+
deprecated in favour of option arguments. For example::
7+
8+
openstack volume group create \
9+
--volume-group-type <volume-group-type>
10+
--volume-type <volume-type> [--volume-type <volume-type> ...]

0 commit comments

Comments
 (0)