openstack
/
python-openstackclient
Code Issues Proposed changes

volume: Deprecate '--detailed' options 

We use flags, not options with boolean-like values, for boolean
parameters. The affected commands were only introduced recently so this
should have minimal fallout.

Change-Id: I700733e750bff539806af167c9d47cec769c3343
Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
This commit is contained in:
Stephen Finucane 2023-06-04 13:29:31 +01:00
parent 08faf81d0d
commit 8735b862c5
3 changed files with 303 additions and 156 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

369
openstackclient/tests/unit/volume/v3/test_block_storage_manage.py
View File

@ -9,7 +9,8 @@
# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
# License for the specific language governing permissions and limitations
# under the License.
#
from unittest import mock
from cinderclient import api_versions
from osc_lib import exceptions
@ -49,12 +50,11 @@ class TestBlockStorageVolumeManage(TestBlockStorageManage):
self.app.client_manager.volume.api_version = api_versions.APIVersion(
'3.8'
)
host = 'fake_host'
arglist = [
host,
'fake_host',
]
verifylist = [
('host', host),
('host', 'fake_host'),
]
parsed_args = self.check_parser(self.cmd, arglist, verifylist)
@ -64,48 +64,37 @@ class TestBlockStorageVolumeManage(TestBlockStorageManage):
'reference',
'size',
'safe_to_manage',
'reason_not_safe',
'cinder_id',
'extra_info',
]
# confirming if all expected columns are present in the result.
self.assertEqual(expected_columns, columns)
datalist = []
for volume_record in self.volume_manage_list:
manage_details = (
volume_record.reference,
volume_record.size,
volume_record.safe_to_manage,
volume_record.reason_not_safe,
volume_record.cinder_id,
volume_record.extra_info,
)
datalist.append(manage_details)
datalist = tuple(datalist)
# confirming if all expected values are present in the result.
self.assertEqual(expected_columns, columns)
self.assertEqual(datalist, tuple(data))
# checking if proper call was made to get volume manageable list
self.volumes_mock.list_manageable.assert_called_with(
host=parsed_args.host,
detailed=parsed_args.detailed,
marker=parsed_args.marker,
limit=parsed_args.limit,
offset=parsed_args.offset,
sort=parsed_args.sort,
cluster=parsed_args.cluster,
host='fake_host',
detailed=False,
marker=None,
limit=None,
offset=None,
sort=None,
cluster=None,
)
def test_block_storage_volume_manage_pre_38(self):
host = 'fake_host'
def test_block_storage_volume_manage_list__pre_v38(self):
arglist = [
host,
'fake_host',
]
verifylist = [
('host', host),
('host', 'fake_host'),
]
parsed_args = self.check_parser(self.cmd, arglist, verifylist)
@ -116,17 +105,16 @@ class TestBlockStorageVolumeManage(TestBlockStorageManage):
'--os-volume-api-version 3.8 or greater is required', str(exc)
)
def test_block_storage_volume_manage_pre_317(self):
def test_block_storage_volume_manage_list__pre_v317(self):
self.app.client_manager.volume.api_version = api_versions.APIVersion(
'3.16'
)
cluster = 'fake_cluster'
arglist = [
'--cluster',
cluster,
'fake_cluster',
]
verifylist = [
('cluster', cluster),
('cluster', 'fake_cluster'),
]
parsed_args = self.check_parser(self.cmd, arglist, verifylist)
@ -138,20 +126,18 @@ class TestBlockStorageVolumeManage(TestBlockStorageManage):
)
self.assertIn('--cluster', str(exc))
def test_block_storage_volume_manage_host_and_cluster(self):
def test_block_storage_volume_manage_list__host_and_cluster(self):
self.app.client_manager.volume.api_version = api_versions.APIVersion(
'3.17'
)
host = 'fake_host'
cluster = 'fake_cluster'
arglist = [
host,
'fake_host',
'--cluster',
cluster,
'fake_cluster',
]
verifylist = [
('host', host),
('cluster', cluster),
('host', 'fake_host'),
('cluster', 'fake_cluster'),
]
exc = self.assertRaises(
tests_utils.ParserException,
@ -164,40 +150,28 @@ class TestBlockStorageVolumeManage(TestBlockStorageManage):
'argument --cluster: not allowed with argument <host>', str(exc)
)
def test_block_storage_volume_manage_list_all_args(self):
def test_block_storage_volume_manage_list__detailed(self):
"""This option is deprecated."""
self.app.client_manager.volume.api_version = api_versions.APIVersion(
'3.8'
)
host = 'fake_host'
detailed = True
marker = 'fake_marker'
limit = '5'
offset = '3'
sort = 'size:asc'
arglist = [
host,
'--detailed',
str(detailed),
'--marker',
marker,
'--limit',
limit,
'--offset',
offset,
'--sort',
sort,
'True',
'fake_host',
]
verifylist = [
('host', host),
('detailed', str(detailed)),
('marker', marker),
('limit', limit),
('offset', offset),
('sort', sort),
('host', 'fake_host'),
('detailed', 'True'),
('marker', None),
('limit', None),
('offset', None),
('sort', None),
]
parsed_args = self.check_parser(self.cmd, arglist, verifylist)
columns, data = self.cmd.take_action(parsed_args)
with mock.patch.object(self.cmd.log, 'warning') as mock_warning:
columns, data = self.cmd.take_action(parsed_args)
expected_columns = [
'reference',
@ -207,10 +181,6 @@ class TestBlockStorageVolumeManage(TestBlockStorageManage):
'cinder_id',
'extra_info',
]
# confirming if all expected columns are present in the result.
self.assertEqual(expected_columns, columns)
datalist = []
for volume_record in self.volume_manage_list:
manage_details = (
@ -224,18 +194,87 @@ class TestBlockStorageVolumeManage(TestBlockStorageManage):
datalist.append(manage_details)
datalist = tuple(datalist)
# confirming if all expected values are present in the result.
self.assertEqual(expected_columns, columns)
self.assertEqual(datalist, tuple(data))
# checking if proper call was made to get volume manageable list
self.volumes_mock.list_manageable.assert_called_with(
host=host,
detailed=detailed,
marker=marker,
limit=limit,
offset=offset,
sort=sort,
cluster=parsed_args.cluster,
host='fake_host',
detailed=True,
marker=None,
limit=None,
offset=None,
sort=None,
cluster=None,
)
mock_warning.assert_called_once()
self.assertIn(
"The --detailed option has been deprecated.",
str(mock_warning.call_args[0][0]),
)
def test_block_storage_volume_manage_list__all_args(self):
self.app.client_manager.volume.api_version = api_versions.APIVersion(
'3.8'
)
arglist = [
'fake_host',
'--long',
'--marker',
'fake_marker',
'--limit',
'5',
'--offset',
'3',
'--sort',
'size:asc',
]
verifylist = [
('host', 'fake_host'),
('detailed', None),
('long', True),
('marker', 'fake_marker'),
('limit', '5'),
('offset', '3'),
('sort', 'size:asc'),
]
parsed_args = self.check_parser(self.cmd, arglist, verifylist)
columns, data = self.cmd.take_action(parsed_args)
expected_columns = [
'reference',
'size',
'safe_to_manage',
'reason_not_safe',
'cinder_id',
'extra_info',
]
datalist = []
for volume_record in self.volume_manage_list:
manage_details = (
volume_record.reference,
volume_record.size,
volume_record.safe_to_manage,
volume_record.reason_not_safe,
volume_record.cinder_id,
volume_record.extra_info,
)
datalist.append(manage_details)
datalist = tuple(datalist)
self.assertEqual(expected_columns, columns)
self.assertEqual(datalist, tuple(data))
# checking if proper call was made to get volume manageable list
self.volumes_mock.list_manageable.assert_called_with(
host='fake_host',
detailed=True,
marker='fake_marker',
limit='5',
offset='3',
sort='size:asc',
cluster=None,
)
@ -258,12 +297,11 @@ class TestBlockStorageSnapshotManage(TestBlockStorageManage):
self.app.client_manager.volume.api_version = api_versions.APIVersion(
'3.8'
)
host = 'fake_host'
arglist = [
host,
'fake_host',
]
verifylist = [
('host', host),
('host', 'fake_host'),
]
parsed_args = self.check_parser(self.cmd, arglist, verifylist)
@ -274,14 +312,7 @@ class TestBlockStorageSnapshotManage(TestBlockStorageManage):
'size',
'safe_to_manage',
'source_reference',
'reason_not_safe',
'cinder_id',
'extra_info',
]
# confirming if all expected columns are present in the result.
self.assertEqual(expected_columns, columns)
datalist = []
for snapshot_record in self.snapshot_manage_list:
manage_details = (
@ -289,34 +320,30 @@ class TestBlockStorageSnapshotManage(TestBlockStorageManage):
snapshot_record.size,
snapshot_record.safe_to_manage,
snapshot_record.source_reference,
snapshot_record.reason_not_safe,
snapshot_record.cinder_id,
snapshot_record.extra_info,
)
datalist.append(manage_details)
datalist = tuple(datalist)
# confirming if all expected values are present in the result.
self.assertEqual(expected_columns, columns)
self.assertEqual(datalist, tuple(data))
# checking if proper call was made to get snapshot manageable list
self.snapshots_mock.list_manageable.assert_called_with(
host=parsed_args.host,
detailed=parsed_args.detailed,
marker=parsed_args.marker,
limit=parsed_args.limit,
offset=parsed_args.offset,
sort=parsed_args.sort,
cluster=parsed_args.cluster,
host='fake_host',
detailed=False,
marker=None,
limit=None,
offset=None,
sort=None,
cluster=None,
)
def test_block_storage_volume_manage_pre_38(self):
host = 'fake_host'
def test_block_storage_snapshot_manage_list__pre_v38(self):
arglist = [
host,
'fake_host',
]
verifylist = [
('host', host),
('host', 'fake_host'),
]
parsed_args = self.check_parser(self.cmd, arglist, verifylist)
@ -327,17 +354,16 @@ class TestBlockStorageSnapshotManage(TestBlockStorageManage):
'--os-volume-api-version 3.8 or greater is required', str(exc)
)
def test_block_storage_volume_manage_pre_317(self):
def test_block_storage_snapshot_manage_list__pre_v317(self):
self.app.client_manager.volume.api_version = api_versions.APIVersion(
'3.16'
)
cluster = 'fake_cluster'
arglist = [
'--cluster',
cluster,
'fake_cluster',
]
verifylist = [
('cluster', cluster),
('cluster', 'fake_cluster'),
]
parsed_args = self.check_parser(self.cmd, arglist, verifylist)
@ -349,20 +375,18 @@ class TestBlockStorageSnapshotManage(TestBlockStorageManage):
)
self.assertIn('--cluster', str(exc))
def test_block_storage_volume_manage_host_and_cluster(self):
def test_block_storage_snapshot_manage_list__host_and_cluster(self):
self.app.client_manager.volume.api_version = api_versions.APIVersion(
'3.17'
)
host = 'fake_host'
cluster = 'fake_cluster'
arglist = [
host,
'fake_host',
'--cluster',
cluster,
'fake_cluster',
]
verifylist = [
('host', host),
('cluster', cluster),
('host', 'fake_host'),
('cluster', 'fake_cluster'),
]
exc = self.assertRaises(
tests_utils.ParserException,
@ -375,40 +399,28 @@ class TestBlockStorageSnapshotManage(TestBlockStorageManage):
'argument --cluster: not allowed with argument <host>', str(exc)
)
def test_block_storage_volume_manage_list_all_args(self):
def test_block_storage_snapshot_manage_list__detailed(self):
"""This option is deprecated."""
self.app.client_manager.volume.api_version = api_versions.APIVersion(
'3.8'
)
host = 'fake_host'
detailed = True
marker = 'fake_marker'
limit = '5'
offset = '3'
sort = 'size:asc'
arglist = [
host,
'--detailed',
str(detailed),
'--marker',
marker,
'--limit',
limit,
'--offset',
offset,
'--sort',
sort,
'True',
'fake_host',
]
verifylist = [
('host', host),
('detailed', str(detailed)),
('marker', marker),
('limit', limit),
('offset', offset),
('sort', sort),
('host', 'fake_host'),
('detailed', 'True'),
('marker', None),
('limit', None),
('offset', None),
('sort', None),
]
parsed_args = self.check_parser(self.cmd, arglist, verifylist)
columns, data = self.cmd.take_action(parsed_args)
with mock.patch.object(self.cmd.log, 'warning') as mock_warning:
columns, data = self.cmd.take_action(parsed_args)
expected_columns = [
'reference',
@ -419,10 +431,6 @@ class TestBlockStorageSnapshotManage(TestBlockStorageManage):
'cinder_id',
'extra_info',
]
# confirming if all expected columns are present in the result.
self.assertEqual(expected_columns, columns)
datalist = []
for snapshot_record in self.snapshot_manage_list:
manage_details = (
@ -437,16 +445,87 @@ class TestBlockStorageSnapshotManage(TestBlockStorageManage):
datalist.append(manage_details)
datalist = tuple(datalist)
# confirming if all expected values are present in the result.
self.assertEqual(expected_columns, columns)
self.assertEqual(datalist, tuple(data))
# checking if proper call was made to get snapshot manageable list
self.snapshots_mock.list_manageable.assert_called_with(
host=host,
detailed=detailed,
marker=marker,
limit=limit,
offset=offset,
sort=sort,
cluster=parsed_args.cluster,
host='fake_host',
detailed=True,
marker=None,
limit=None,
offset=None,
sort=None,
cluster=None,
)
mock_warning.assert_called_once()
self.assertIn(
"The --detailed option has been deprecated.",
str(mock_warning.call_args[0][0]),
)
def test_block_storage_snapshot_manage_list__all_args(self):
self.app.client_manager.volume.api_version = api_versions.APIVersion(
'3.8'
)
arglist = [
'--long',
'--marker',
'fake_marker',
'--limit',
'5',
'--offset',
'3',
'--sort',
'size:asc',
'fake_host',
]
verifylist = [
('host', 'fake_host'),
('detailed', None),
('long', True),
('marker', 'fake_marker'),
('limit', '5'),
('offset', '3'),
('sort', 'size:asc'),
]
parsed_args = self.check_parser(self.cmd, arglist, verifylist)
columns, data = self.cmd.take_action(parsed_args)
expected_columns = [
'reference',
'size',
'safe_to_manage',
'source_reference',
'reason_not_safe',
'cinder_id',
'extra_info',
]
datalist = []
for snapshot_record in self.snapshot_manage_list:
manage_details = (
snapshot_record.reference,
snapshot_record.size,
snapshot_record.safe_to_manage,
snapshot_record.source_reference,
snapshot_record.reason_not_safe,
snapshot_record.cinder_id,
snapshot_record.extra_info,
)
datalist.append(manage_details)
datalist = tuple(datalist)
self.assertEqual(expected_columns, columns)
self.assertEqual(datalist, tuple(data))
# checking if proper call was made to get snapshot manageable list
self.snapshots_mock.list_manageable.assert_called_with(
host='fake_host',
detailed=True,
marker='fake_marker',
limit='5',
offset='3',
sort='size:asc',
cluster=None,
)

83
openstackclient/volume/v3/block_storage_manage.py
View File

@ -13,11 +13,12 @@
"""Block Storage Volume/Snapshot Management implementations"""
import argparse
from cinderclient import api_versions
from osc_lib.command import command
from osc_lib import exceptions
from osc_lib import utils
from oslo_utils import strutils
from openstackclient.i18n import _
@ -52,11 +53,18 @@ class BlockStorageManageVolumes(command.Lister):
'(supported by --os-volume-api-version 3.17 or later)'
),
)
parser.add_argument(
'--long',
action='store_true',
default=False,
help=_('List additional fields in output'),
)
# TODO(stephenfin): Remove this in a future major version bump
parser.add_argument(
'--detailed',
metavar='<detailed>',
default=True,
help=_('Returns detailed information (Default=True).'),
default=None,
help=argparse.SUPPRESS,
)
parser.add_argument(
'--marker',
@ -121,8 +129,30 @@ class BlockStorageManageVolumes(command.Lister):
)
raise exceptions.CommandError(msg)
detailed = strutils.bool_from_string(parsed_args.detailed)
cluster = getattr(parsed_args, 'cluster', None)
detailed = parsed_args.long
if parsed_args.detailed is not None:
detailed = parsed_args.detailed.lower().strip() in {
'1',
't',
'true',
'on',
'y',
'yes',
}
if detailed:
# if the user requested e.g. '--detailed true' then they should
# not request '--long'
msg = _(
"The --detailed option has been deprecated. "
"Use --long instead."
)
self.log.warning(msg)
else:
# if the user requested e.g. '--detailed false' then they
# should simply stop requesting this since the default has
# changed
msg = _("The --detailed option has been deprecated. Unset it.")
self.log.warning(msg)
columns = [
'reference',
@ -145,7 +175,7 @@ class BlockStorageManageVolumes(command.Lister):
limit=parsed_args.limit,
offset=parsed_args.offset,
sort=parsed_args.sort,
cluster=cluster,
cluster=parsed_args.cluster,
)
return (
@ -187,11 +217,18 @@ class BlockStorageManageSnapshots(command.Lister):
'(supported by --os-volume-api-version 3.17 or later)'
),
)
parser.add_argument(
'--long',
action='store_true',
default=False,
help=_('List additional fields in output'),
)
# TODO(stephenfin): Remove this in a future major version bump
parser.add_argument(
'--detailed',
metavar='<detailed>',
default=True,
help=_('Returns detailed information (Default=True).'),
default=None,
help=argparse.SUPPRESS,
)
parser.add_argument(
'--marker',
@ -258,8 +295,32 @@ class BlockStorageManageSnapshots(command.Lister):
)
raise exceptions.CommandError(msg)
detailed = strutils.bool_from_string(parsed_args.detailed)
cluster = getattr(parsed_args, 'cluster', None)
detailed = parsed_args.long
if parsed_args.detailed is not None:
detailed = parsed_args.detailed.lower().strip() in {
'1',
't',
'true',
'on',
'y',
'yes',
}
if detailed:
# if the user requested e.g. '--detailed true' then they should
# not request '--long'
msg = _(
"The --detailed option has been deprecated. "
"Use --long instead."
)
self.log.warning(msg)
else:
# if the user requested e.g. '--detailed false' then they
# should simply stop requesting this since the default has
# changed
msg = _(
"The --detailed option has been deprecated. " "Unset it."
)
self.log.warning(msg)
columns = [
'reference',
@ -283,7 +344,7 @@ class BlockStorageManageSnapshots(command.Lister):
limit=parsed_args.limit,
offset=parsed_args.offset,
sort=parsed_args.sort,
cluster=cluster,
cluster=parsed_args.cluster,
)
return (

7
releasenotes/notes/block-storage-x-manageable-list-long-option-a16a4641acfcf781.yaml Normal file
View File

@ -0,0 +1,7 @@
---
upgrade:
- |
The ``--detailed`` option of the ``block storage volume manageable list``
and ``block storage snapshot manageable list`` commands has been deprecated
in favour of a ``--long`` option. These commands will no longer default to
detailed output by default.