Merge "Cleanup of 'server migration list' command"

2 files changed, 211 insertions, 167 deletions

diff --git a/openstackclient/compute/v2/server.py b/openstackclient/compute/v2/server.py
index 4aed8ad7..d663a6cb 100644
--- a/openstackclient/compute/v2/server.py
+++ b/openstackclient/compute/v2/server.py
@@ -1981,99 +1981,99 @@ revert to release the new server and restart the old one.""")
 
 
 class ListMigration(command.Command):
-    _description = _("""List server migrations.""")
+    _description = _("""List server migrations""")
 
     def get_parser(self, prog_name):
         parser = super(ListMigration, self).get_parser(prog_name)
         parser.add_argument(
-            "--server",
-            metavar="<server>",
-            dest='server',
-            default=None,
-            help=_('Server to show migration details (name or ID).')
+            '--server',
+            metavar='<server>',
+            help=_(
+                'Filter migrations by server (name or ID)'
+            )
         )
         parser.add_argument(
-            "--host",
-            metavar="<host>",
-            default=None,
-            help=_('Fetch migrations for the given host.')
+            '--host',
+            metavar='<host>',
+            help=_(
+                'Filter migrations by source or destination host'
+            ),
         )
         parser.add_argument(
-            "--status",
-            metavar="<status>",
-            default=None,
-            help=_('Fetch migrations for the given status.')
+            '--status',
+            metavar='<status>',
+            help=_('Filter migrations by status')
         )
         parser.add_argument(
-            "--marker",
-            metavar="<marker>",
-            dest='marker',
-            default=None,
-            help=_("The last migration of the previous page; displays list "
-                   "of migrations after 'marker'. Note that the marker is "
-                   "the migration UUID. (Supported with "
-                   "``--os-compute-api-version`` 2.59 or greater.)")
+            '--marker',
+            metavar='<marker>',
+            help=_(
+                "The last migration of the previous page; displays list "
+                "of migrations after 'marker'. Note that the marker is "
+                "the migration UUID. "
+                "(supported with --os-compute-api-version 2.59 or above)"
+            ),
         )
         parser.add_argument(
-            "--limit",
-            metavar="<limit>",
-            dest='limit',
+            '--limit',
+            metavar='<limit>',
             type=int,
-            default=None,
-            help=_("Maximum number of migrations to display. Note that there "
-                   "is a configurable max limit on the server, and the limit "
-                   "that is used will be the minimum of what is requested "
-                   "here and what is configured in the server. "
-                   "(Supported with ``--os-compute-api-version`` 2.59 "
-                   "or greater.)")
+            help=_(
+                "Maximum number of migrations to display. Note that there "
+                "is a configurable max limit on the server, and the limit "
+                "that is used will be the minimum of what is requested "
+                "here and what is configured in the server. "
+                "(supported with --os-compute-api-version 2.59 or above)"
+            ),
         )
         parser.add_argument(
             '--changes-since',
             dest='changes_since',
             metavar='<changes-since>',
-            default=None,
-            help=_("List only migrations changed later or equal to a certain "
-                   "point of time. The provided time should be an ISO 8061 "
-                   "formatted time, e.g. ``2016-03-04T06:27:59Z``. "
-                   "(Supported with ``--os-compute-api-version`` 2.59 "
-                   "or greater.)")
+            help=_(
+                "List only migrations changed later or equal to a certain "
+                "point of time. The provided time should be an ISO 8061 "
+                "formatted time, e.g. ``2016-03-04T06:27:59Z``. "
+                "(supported with --os-compute-api-version 2.59 or above)"
+            ),
         )
         parser.add_argument(
             '--changes-before',
             dest='changes_before',
             metavar='<changes-before>',
-            default=None,
-            help=_("List only migrations changed earlier or equal to a "
-                   "certain point of time. The provided time should be an ISO "
-                   "8061 formatted time, e.g. ``2016-03-04T06:27:59Z``. "
-                   "(Supported with ``--os-compute-api-version`` 2.66 or "
-                   "greater.)")
+            help=_(
+                "List only migrations changed earlier or equal to a "
+                "certain point of time. The provided time should be an ISO "
+                "8061 formatted time, e.g. ``2016-03-04T06:27:59Z``. "
+                "(supported with --os-compute-api-version 2.66 or above)"
+            ),
         )
         parser.add_argument(
             '--project',
             metavar='<project>',
             dest='project_id',
-            default=None,
-            help=_("Filter the migrations by the given project ID. "
-                   "(Supported with ``--os-compute-api-version`` 2.80 "
-                   "or greater.)"),
+            help=_(
+                "Filter migrations by project (ID) "
+                "(supported with --os-compute-api-version 2.80 or above)"
+            ),
         )
         parser.add_argument(
             '--user',
             metavar='<user>',
             dest='user_id',
-            default=None,
-            help=_("Filter the migrations by the given user ID. "
-                   "(Supported with ``--os-compute-api-version`` 2.80 "
-                   "or greater.)"),
+            help=_(
+                "Filter migrations by user (ID) "
+                "(supported with --os-compute-api-version 2.80 or above)"
+            ),
         )
         return parser
 
     def print_migrations(self, parsed_args, compute_client, migrations):
-        columns = ['Source Node', 'Dest Node', 'Source Compute',
-                   'Dest Compute', 'Dest Host', 'Status',
-                   'Server UUID', 'Old Flavor', 'New Flavor',
-                   'Created At', 'Updated At']
+        columns = [
+            'Source Node', 'Dest Node', 'Source Compute', 'Dest Compute',
+            'Dest Host', 'Status', 'Server UUID', 'Old Flavor', 'New Flavor',
+            'Created At', 'Updated At',
+        ]
 
         # Insert migrations UUID after ID
         if compute_client.api_version >= api_versions.APIVersion("2.59"):
@@ -2093,48 +2093,73 @@ class ListMigration(command.Command):
             if parsed_args.user_id:
                 columns.insert(len(columns) - 2, "User")
 
-        columns_header = columns
-        return (columns_header, (utils.get_item_properties(
-            mig, columns) for mig in migrations))
+        return (
+            columns,
+            (utils.get_item_properties(mig, columns) for mig in migrations),
+        )
 
     def take_action(self, parsed_args):
         compute_client = self.app.client_manager.compute
 
         search_opts = {
-            "host": parsed_args.host,
-            "server": parsed_args.server,
-            "status": parsed_args.status,
+            'host': parsed_args.host,
+            'server': parsed_args.server,
+            'status': parsed_args.status,
         }
 
-        if (parsed_args.marker or parsed_args.limit or
-                parsed_args.changes_since):
-            if compute_client.api_version < api_versions.APIVersion("2.59"):
-                msg = _("marker, limit and/or changes_since is not supported "
-                        "for --os-compute-api-version less than 2.59")
+        if parsed_args.marker:
+            if compute_client.api_version < api_versions.APIVersion('2.59'):
+                msg = _(
+                    '--os-compute-api-version 2.59 or greater is required to '
+                    'support the --marker option'
+                )
                 raise exceptions.CommandError(msg)
-            if parsed_args.marker:
-                search_opts['marker'] = parsed_args.marker
-            if parsed_args.limit:
-                search_opts['limit'] = parsed_args.limit
-            if parsed_args.changes_since:
-                search_opts['changes_since'] = parsed_args.changes_since
+            search_opts['marker'] = parsed_args.marker
+
+        if parsed_args.limit:
+            if compute_client.api_version < api_versions.APIVersion('2.59'):
+                msg = _(
+                    '--os-compute-api-version 2.59 or greater is required to '
+                    'support the --limit option'
+                )
+                raise exceptions.CommandError(msg)
+            search_opts['limit'] = parsed_args.limit
+
+        if parsed_args.changes_since:
+            if compute_client.api_version < api_versions.APIVersion('2.59'):
+                msg = _(
+                    '--os-compute-api-version 2.59 or greater is required to '
+                    'support the --changes-since option'
+                )
+                raise exceptions.CommandError(msg)
+            search_opts['changes_since'] = parsed_args.changes_since
 
         if parsed_args.changes_before:
-            if compute_client.api_version < api_versions.APIVersion("2.66"):
-                msg = _("changes_before is not supported for "
-                        "--os-compute-api-version less than 2.66")
+            if compute_client.api_version < api_versions.APIVersion('2.66'):
+                msg = _(
+                    '--os-compute-api-version 2.66 or greater is required to '
+                    'support the --changes-before option'
+                )
                 raise exceptions.CommandError(msg)
             search_opts['changes_before'] = parsed_args.changes_before
 
-        if parsed_args.project_id or parsed_args.user_id:
-            if compute_client.api_version < api_versions.APIVersion("2.80"):
-                msg = _("Project and/or user is not supported for "
-                        "--os-compute-api-version less than 2.80")
+        if parsed_args.project_id:
+            if compute_client.api_version < api_versions.APIVersion('2.80'):
+                msg = _(
+                    '--os-compute-api-version 2.80 or greater is required to '
+                    'support the --project option'
+                )
+                raise exceptions.CommandError(msg)
+            search_opts['project_id'] = parsed_args.project_id
+
+        if parsed_args.user_id:
+            if compute_client.api_version < api_versions.APIVersion('2.80'):
+                msg = _(
+                    '--os-compute-api-version 2.80 or greater is required to '
+                    'support the --user option'
+                )
                 raise exceptions.CommandError(msg)
-            if parsed_args.project_id:
-                search_opts['project_id'] = parsed_args.project_id
-            if parsed_args.user_id:
-                search_opts['user_id'] = parsed_args.user_id
+            search_opts['user_id'] = parsed_args.user_id
 
         migrations = compute_client.migrations.list(**search_opts)
 
diff --git a/openstackclient/tests/unit/compute/v2/test_server.py b/openstackclient/tests/unit/compute/v2/test_server.py
index 441558fd..380ef66b 100644
--- a/openstackclient/tests/unit/compute/v2/test_server.py
+++ b/openstackclient/tests/unit/compute/v2/test_server.py
@@ -47,10 +47,14 @@ class TestServer(compute_fakes.TestComputev2):
             self.app.client_manager.compute.server_migrations
         self.server_migrations_mock.reset_mock()
 
-        # Get a shortcut to the compute client volumeManager Mock
+        # Get a shortcut to the compute client VolumeManager mock
         self.servers_volumes_mock = self.app.client_manager.compute.volumes
         self.servers_volumes_mock.reset_mock()
 
+        # Get a shortcut to the compute client MigrationManager mock
+        self.migrations_mock = self.app.client_manager.compute.migrations
+        self.migrations_mock.reset_mock()
+
         # Get a shortcut to the compute client FlavorManager Mock
         self.flavors_mock = self.app.client_manager.compute.flavors
         self.flavors_mock.reset_mock()
@@ -4039,34 +4043,7 @@ class TestServerMigrate(TestServer):
         self.assertNotCalled(self.servers_mock.live_migrate)
 
 
-class TestServerMigration(TestServer):
-
-    def setUp(self):
-        super(TestServerMigration, self).setUp()
-
-        # Get a shortcut to the compute client ServerManager Mock
-        self.servers_mock = self.app.client_manager.compute.servers
-        self.servers_mock.reset_mock()
-
-        self.migrations_mock = (
-            self.app.client_manager.compute.migrations)
-        self.migrations_mock.reset_mock()
-
-        self.server = self.setup_servers_mock(1)[0]
-
-    def setup_servers_mock(self, count):
-        servers = compute_fakes.FakeServer.create_servers(count=count)
-
-        # This is the return value for utils.find_resource()
-        self.servers_mock.get = compute_fakes.FakeServer.get_servers(servers)
-        return servers
-
-    def setup_server_migrations_mock(self, count):
-        return compute_fakes.FakeServerMigration.create_server_migrations(
-            count=count)
-
-
-class TestListMigration(TestServerMigration):
+class TestListMigration(TestServer):
     """Test fetch all migrations."""
 
     MIGRATION_COLUMNS = [
@@ -4078,24 +4055,48 @@ class TestListMigration(TestServerMigration):
     def setUp(self):
         super(TestListMigration, self).setUp()
 
-        self.cmd = server.ListMigration(self.app, None)
-        self.migrations = self.setup_server_migrations_mock(3)
-        self.migrations_mock.list.return_value = self.migrations
-        self.setup_server_migrations_data(self.migrations)
+        self.server = compute_fakes.FakeServer.create_one_server()
+        self.servers_mock.get.return_value = self.server
 
-        self.app.client_manager.compute.api_version = api_versions.APIVersion(
-            '2.20')
+        self.migrations = compute_fakes.FakeServerMigration\
+            .create_server_migrations(count=3)
+        self.migrations_mock.list.return_value = self.migrations
 
-    def setup_server_migrations_data(self, migrations):
         self.data = (common_utils.get_item_properties(
-            s, self.MIGRATION_COLUMNS) for s in migrations)
+            s, self.MIGRATION_COLUMNS) for s in self.migrations)
+
+        # Get the command object to test
+        self.cmd = server.ListMigration(self.app, None)
+
+    def test_server_migration_list_no_options(self):
+        arglist = []
+        verifylist = []
+
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+        columns, data = self.cmd.take_action(parsed_args)
+
+        # Set expected values
+        kwargs = {
+            'status': None,
+            'host': None,
+            'server': None,
+        }
+
+        self.migrations_mock.list.assert_called_with(**kwargs)
+
+        self.assertEqual(self.MIGRATION_COLUMNS, columns)
+        self.assertEqual(tuple(self.data), tuple(data))
 
-    def test_server_migraton_list(self):
+    def test_server_migration_list(self):
         arglist = [
-            '--status', 'migrating'
+            '--server', 'server1',
+            '--host', 'host1',
+            '--status', 'migrating',
         ]
         verifylist = [
-            ('status', 'migrating')
+            ('server', 'server1'),
+            ('host', 'host1'),
+            ('status', 'migrating'),
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
         columns, data = self.cmd.take_action(parsed_args)
@@ -4103,8 +4104,8 @@ class TestListMigration(TestServerMigration):
         # Set expected values
         kwargs = {
             'status': 'migrating',
-            'host': None,
-            'server': None,
+            'host': 'host1',
+            'server': 'server1',
         }
 
         self.migrations_mock.list.assert_called_with(**kwargs)
@@ -4124,15 +4125,11 @@ class TestListMigrationV223(TestListMigration):
 
     def setUp(self):
         super(TestListMigrationV223, self).setUp()
-        self.cmd = server.ListMigration(self.app, None)
-        self.migrations = self.setup_server_migrations_mock(3)
-        self.migrations_mock.list.return_value = self.migrations
-        self.setup_server_migrations_data(self.migrations)
 
         self.app.client_manager.compute.api_version = api_versions.APIVersion(
             '2.23')
 
-    def test_server_migraton_list(self):
+    def test_server_migration_list(self):
         arglist = [
             '--status', 'migrating'
         ]
@@ -4169,15 +4166,11 @@ class TestListMigrationV259(TestListMigration):
 
     def setUp(self):
         super(TestListMigrationV259, self).setUp()
-        self.cmd = server.ListMigration(self.app, None)
-        self.migrations = self.setup_server_migrations_mock(3)
-        self.migrations_mock.list.return_value = self.migrations
-        self.setup_server_migrations_data(self.migrations)
 
         self.app.client_manager.compute.api_version = api_versions.APIVersion(
             '2.59')
 
-    def test_server_migraton_list(self):
+    def test_server_migration_list(self):
         arglist = [
             '--status', 'migrating',
             '--limit', '1',
@@ -4208,7 +4201,7 @@ class TestListMigrationV259(TestListMigration):
         self.assertEqual(self.MIGRATION_COLUMNS, columns)
         self.assertEqual(tuple(self.data), tuple(data))
 
-    def test_server_migraton_list_with_limit_pre_v259(self):
+    def test_server_migration_list_with_limit_pre_v259(self):
         self.app.client_manager.compute.api_version = api_versions.APIVersion(
             '2.58')
         arglist = [
@@ -4220,10 +4213,15 @@ class TestListMigrationV259(TestListMigration):
             ('limit', 1)
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
-        self.assertRaises(exceptions.CommandError, self.cmd.take_action,
-                          parsed_args)
+        ex = self.assertRaises(
+            exceptions.CommandError,
+            self.cmd.take_action,
+            parsed_args)
+        self.assertIn(
+            '--os-compute-api-version 2.59 or greater is required',
+            str(ex))
 
-    def test_server_migraton_list_with_marker_pre_v259(self):
+    def test_server_migration_list_with_marker_pre_v259(self):
         self.app.client_manager.compute.api_version = api_versions.APIVersion(
             '2.58')
         arglist = [
@@ -4235,10 +4233,15 @@ class TestListMigrationV259(TestListMigration):
             ('marker', 'test_kp')
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
-        self.assertRaises(exceptions.CommandError, self.cmd.take_action,
-                          parsed_args)
+        ex = self.assertRaises(
+            exceptions.CommandError,
+            self.cmd.take_action,
+            parsed_args)
+        self.assertIn(
+            '--os-compute-api-version 2.59 or greater is required',
+            str(ex))
 
-    def test_server_migraton_list_with_changes_since_pre_v259(self):
+    def test_server_migration_list_with_changes_since_pre_v259(self):
         self.app.client_manager.compute.api_version = api_versions.APIVersion(
             '2.58')
         arglist = [
@@ -4250,8 +4253,13 @@ class TestListMigrationV259(TestListMigration):
             ('changes_since', '2019-08-09T08:03:25Z')
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
-        self.assertRaises(exceptions.CommandError, self.cmd.take_action,
-                          parsed_args)
+        ex = self.assertRaises(
+            exceptions.CommandError,
+            self.cmd.take_action,
+            parsed_args)
+        self.assertIn(
+            '--os-compute-api-version 2.59 or greater is required',
+            str(ex))
 
 
 class TestListMigrationV266(TestListMigration):
@@ -4265,15 +4273,11 @@ class TestListMigrationV266(TestListMigration):
 
     def setUp(self):
         super(TestListMigrationV266, self).setUp()
-        self.cmd = server.ListMigration(self.app, None)
-        self.migrations = self.setup_server_migrations_mock(3)
-        self.migrations_mock.list.return_value = self.migrations
-        self.setup_server_migrations_data(self.migrations)
 
         self.app.client_manager.compute.api_version = api_versions.APIVersion(
             '2.66')
 
-    def test_server_migraton_list_with_changes_before(self):
+    def test_server_migration_list_with_changes_before(self):
         arglist = [
             '--status', 'migrating',
             '--limit', '1',
@@ -4307,7 +4311,7 @@ class TestListMigrationV266(TestListMigration):
         self.assertEqual(self.MIGRATION_COLUMNS, columns)
         self.assertEqual(tuple(self.data), tuple(data))
 
-    def test_server_migraton_list_with_changes_before_pre_v266(self):
+    def test_server_migration_list_with_changes_before_pre_v266(self):
         self.app.client_manager.compute.api_version = api_versions.APIVersion(
             '2.65')
         arglist = [
@@ -4319,8 +4323,13 @@ class TestListMigrationV266(TestListMigration):
             ('changes_before', '2019-08-09T08:03:25Z')
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
-        self.assertRaises(exceptions.CommandError, self.cmd.take_action,
-                          parsed_args)
+        ex = self.assertRaises(
+            exceptions.CommandError,
+            self.cmd.take_action,
+            parsed_args)
+        self.assertIn(
+            '--os-compute-api-version 2.66 or greater is required',
+            str(ex))
 
 
 class TestListMigrationV280(TestListMigration):
@@ -4334,15 +4343,11 @@ class TestListMigrationV280(TestListMigration):
 
     def setUp(self):
         super(TestListMigrationV280, self).setUp()
-        self.cmd = server.ListMigration(self.app, None)
-        self.migrations = self.setup_server_migrations_mock(3)
-        self.migrations_mock.list.return_value = self.migrations
-        self.setup_server_migrations_data(self.migrations)
 
         self.app.client_manager.compute.api_version = api_versions.APIVersion(
             '2.80')
 
-    def test_server_migraton_list_with_project(self):
+    def test_server_migration_list_with_project(self):
         arglist = [
             '--status', 'migrating',
             '--limit', '1',
@@ -4397,10 +4402,15 @@ class TestListMigrationV280(TestListMigration):
             ('project_id', '0c2accde-644a-45fa-8c10-e76debc7fbc3')
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
-        self.assertRaises(exceptions.CommandError, self.cmd.take_action,
-                          parsed_args)
+        ex = self.assertRaises(
+            exceptions.CommandError,
+            self.cmd.take_action,
+            parsed_args)
+        self.assertIn(
+            '--os-compute-api-version 2.80 or greater is required',
+            str(ex))
 
-    def test_server_migraton_list_with_user(self):
+    def test_server_migration_list_with_user(self):
         arglist = [
             '--status', 'migrating',
             '--limit', '1',
@@ -4455,11 +4465,15 @@ class TestListMigrationV280(TestListMigration):
             ('user_id', 'dd214878-ca12-40fb-b035-fa7d2c1e86d6')
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+        ex = self.assertRaises(
+            exceptions.CommandError,
+            self.cmd.take_action,
+            parsed_args)
+        self.assertIn(
+            '--os-compute-api-version 2.80 or greater is required',
+            str(ex))
 
-        self.assertRaises(exceptions.CommandError, self.cmd.take_action,
-                          parsed_args)
-
-    def test_server_migraton_list_with_project_and_user(self):
+    def test_server_migration_list_with_project_and_user(self):
         arglist = [
             '--status', 'migrating',
             '--limit', '1',
@@ -4519,8 +4533,13 @@ class TestListMigrationV280(TestListMigration):
             ('user_id', 'dd214878-ca12-40fb-b035-fa7d2c1e86d6')
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
-        self.assertRaises(exceptions.CommandError, self.cmd.take_action,
-                          parsed_args)
+        ex = self.assertRaises(
+            exceptions.CommandError,
+            self.cmd.take_action,
+            parsed_args)
+        self.assertIn(
+            '--os-compute-api-version 2.80 or greater is required',
+            str(ex))
 
 
 class TestServerMigrationAbort(TestServer):