Merge "volume: Deprecate '--detailed' options"

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,
+            host='fake_host',
-            detailed=parsed_args.detailed,
+            detailed=False,
-            marker=parsed_args.marker,
+            marker=None,
-            limit=parsed_args.limit,
+            limit=None,
-            offset=parsed_args.offset,
+            offset=None,
-            sort=parsed_args.sort,
+            sort=None,
-            cluster=parsed_args.cluster,
+            cluster=None,
         )
 
-    def test_block_storage_volume_manage_pre_38(self):
+    def test_block_storage_volume_manage_list__pre_v38(self):
-        host = 'fake_host'
         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),
+            ('host', 'fake_host'),
-            ('cluster', cluster),
+            ('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),
+            'True',
-            '--marker',
+            'fake_host',
-            marker,
-            '--limit',
-            limit,
-            '--offset',
-            offset,
-            '--sort',
-            sort,
         ]
         verifylist = [
-            ('host', host),
+            ('host', 'fake_host'),
-            ('detailed', str(detailed)),
+            ('detailed', 'True'),
-            ('marker', marker),
+            ('marker', None),
-            ('limit', limit),
+            ('limit', None),
-            ('offset', offset),
+            ('offset', None),
-            ('sort', sort),
+            ('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,
+            host='fake_host',
-            detailed=detailed,
+            detailed=True,
-            marker=marker,
+            marker=None,
-            limit=limit,
+            limit=None,
-            offset=offset,
+            offset=None,
-            sort=sort,
+            sort=None,
-            cluster=parsed_args.cluster,
+            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,
+            host='fake_host',
-            detailed=parsed_args.detailed,
+            detailed=False,
-            marker=parsed_args.marker,
+            marker=None,
-            limit=parsed_args.limit,
+            limit=None,
-            offset=parsed_args.offset,
+            offset=None,
-            sort=parsed_args.sort,
+            sort=None,
-            cluster=parsed_args.cluster,
+            cluster=None,
         )
 
-    def test_block_storage_volume_manage_pre_38(self):
+    def test_block_storage_snapshot_manage_list__pre_v38(self):
-        host = 'fake_host'
         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),
+            ('host', 'fake_host'),
-            ('cluster', cluster),
+            ('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),
+            'True',
-            '--marker',
+            'fake_host',
-            marker,
-            '--limit',
-            limit,
-            '--offset',
-            offset,
-            '--sort',
-            sort,
         ]
         verifylist = [
-            ('host', host),
+            ('host', 'fake_host'),
-            ('detailed', str(detailed)),
+            ('detailed', 'True'),
-            ('marker', marker),
+            ('marker', None),
-            ('limit', limit),
+            ('limit', None),
-            ('offset', offset),
+            ('offset', None),
-            ('sort', sort),
+            ('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,
+            host='fake_host',
-            detailed=detailed,
+            detailed=True,
-            marker=marker,
+            marker=None,
-            limit=limit,
+            limit=None,
-            offset=offset,
+            offset=None,
-            sort=sort,
+            sort=None,
-            cluster=parsed_args.cluster,
+            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,
         )

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='<detailed>',
-            default=True,
+            default=None,
-            help=_('Returns detailed information (Default=True).'),
+            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)
+        detailed = parsed_args.long
-        cluster = getattr(parsed_args, 'cluster', None)
+        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,
+            default=None,
-            help=_('Returns detailed information (Default=True).'),
+            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)
+        detailed = parsed_args.long
-        cluster = getattr(parsed_args, 'cluster', None)
+        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 (

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.