From 8735b862c52daaafffd3c6d91c14be21891ac74b Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Sun, 4 Jun 2023 13:29:31 +0100 Subject: [PATCH] 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 --- .../volume/v3/test_block_storage_manage.py | 369 +++++++++++------- .../volume/v3/block_storage_manage.py | 83 +++- ...ble-list-long-option-a16a4641acfcf781.yaml | 7 + 3 files changed, 303 insertions(+), 156 deletions(-) create mode 100644 releasenotes/notes/block-storage-x-manageable-list-long-option-a16a4641acfcf781.yaml diff --git a/openstackclient/tests/unit/volume/v3/test_block_storage_manage.py b/openstackclient/tests/unit/volume/v3/test_block_storage_manage.py index 27a7bf358..fc3a0029f 100644 --- a/openstackclient/tests/unit/volume/v3/test_block_storage_manage.py +++ b/openstackclient/tests/unit/volume/v3/test_block_storage_manage.py @@ -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 ', 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 ', 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, ) diff --git a/openstackclient/volume/v3/block_storage_manage.py b/openstackclient/volume/v3/block_storage_manage.py index 6810e5c84..5b76b53cb 100644 --- a/openstackclient/volume/v3/block_storage_manage.py +++ b/openstackclient/volume/v3/block_storage_manage.py @@ -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='', - 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='', - 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 ( diff --git a/releasenotes/notes/block-storage-x-manageable-list-long-option-a16a4641acfcf781.yaml b/releasenotes/notes/block-storage-x-manageable-list-long-option-a16a4641acfcf781.yaml new file mode 100644 index 000000000..60566ba57 --- /dev/null +++ b/releasenotes/notes/block-storage-x-manageable-list-long-option-a16a4641acfcf781.yaml @@ -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.