compute: Show flavor in 'server list' with API >= 2.47

Fix the issue where the flavor name was empty in server list output.
This requires somewhat invasive unit test changes to reflect the changed
API response from the server, but this has the upside of meaning we
don't need new tests since what we have validates things.
Also drop the flavor ID column as it is removed from the compute API.

Conflicts:
    openstackclient/tests/unit/compute/v2/test_server.py

NOTE(melwitt): The conflicts and differences from the cherry picked
change are because the following changes are not in Victoria:

  * I4811f8f66dcb14ed99cc1cfb80b00e2d77afe45f
    (compute: Add 'server * --all-projects' option)
  * I7a8349106e211c57c4577b75326b39b88bd9ac1e
    (compute: Fix 'server * -f yaml' output)
  * I84848c0bf8ab3c36dd821141191e2725e4e3b58b (Remove usage of six)
  * Ieeb1f22df7092e66a411b6a36eafb3e16efc2fc2
    (compute: Add missing options for 'server list')
  * If065602792958ff0145ae9f2e05f5b7a3177905c
    (Compute: Add tags support for server)
  * I25a4da697e27c0fba4d28b504377667eb18f15fe
    (compute: Add '--force' option to 'server delete')
  * I463993170c03a1d98c47ab6a3c19131b7fca1099 (Remove oslo.utils)
  * I18991adf899c7b72c98bb89871bf0715d35943f0
    (Add a few selectable fields to the "openstack server list" output)
  * I62b2ed8488ee4ac9c42051311bcfb455506ddd90
    (Switch compute flavors from novaclient/direct to SDK)

Change-Id: Ica3320242a38901c1180b2b29109c9474366fde0
Signed-off-by: Khomesh Thakre <khomeshthakre24@gmail.com>
Story: 2008257
Task: 41113
(cherry picked from commit 8e362402dee07744668bcf7f6774af4fbe9a07e3)
(cherry picked from commit 0873e7580eceab07c3be0824d2ea4163491f8d6e)
(cherry picked from commit 4b7e777c0ce19aa67a9a33cbeb3b4ee2b052383f)

2 files changed, 223 insertions, 135 deletions

diff --git a/openstackclient/compute/v2/server.py b/openstackclient/compute/v2/server.py
index 85381fbe..f4d49a74 100644
--- a/openstackclient/compute/v2/server.py
+++ b/openstackclient/compute/v2/server.py
@@ -1444,21 +1444,28 @@ class ListServer(command.Lister):
                 columns += ('image_name',)
             column_headers += ('Image',)
 
-        if parsed_args.long:
-            columns += (
-                'flavor_name',
-                'flavor_id',
-            )
-            column_headers += (
-                'Flavor Name',
-                'Flavor ID',
-            )
+        # microversion 2.47 puts the embedded flavor into the server response
+        # body but omits the id, so if not present we just expose the original
+        # flavor name in the output
+        if compute_client.api_version >= api_versions.APIVersion('2.47'):
+            columns += ('flavor_name',)
+            column_headers += ('Flavor',)
         else:
-            if parsed_args.no_name_lookup:
-                columns += ('flavor_id',)
+            if parsed_args.long:
+                columns += (
+                    'flavor_name',
+                    'flavor_id',
+                )
+                column_headers += (
+                    'Flavor Name',
+                    'Flavor ID',
+                )
             else:
-                columns += ('flavor_name',)
-            column_headers += ('Flavor',)
+                if parsed_args.no_name_lookup:
+                    columns += ('flavor_id',)
+                else:
+                    columns += ('flavor_name',)
+                column_headers += ('Flavor',)
 
         if parsed_args.long:
             columns += (
@@ -1565,18 +1572,13 @@ class ListServer(command.Lister):
                 s.image_name = IMAGE_STRING_FOR_BFV
                 s.image_id = IMAGE_STRING_FOR_BFV
 
-            if 'id' in s.flavor:
+            if compute_client.api_version < api_versions.APIVersion('2.47'):
                 flavor = flavors.get(s.flavor['id'])
                 if flavor:
                     s.flavor_name = flavor.name
                 s.flavor_id = s.flavor['id']
             else:
-                # TODO(mriedem): Fix this for microversion >= 2.47 where the
-                # flavor is embedded in the server response without the id.
-                # We likely need to drop the Flavor ID column in that case if
-                # --long is specified.
-                s.flavor_name = ''
-                s.flavor_id = ''
+                s.flavor_name = s.flavor['original_name']
 
         table = (column_headers,
                  (utils.get_item_properties(
diff --git a/openstackclient/tests/unit/compute/v2/test_server.py b/openstackclient/tests/unit/compute/v2/test_server.py
index 02bb406c..af589228 100644
--- a/openstackclient/tests/unit/compute/v2/test_server.py
+++ b/openstackclient/tests/unit/compute/v2/test_server.py
@@ -2557,7 +2557,7 @@ class TestServerDumpCreate(TestServer):
         self.run_method_with_servers('trigger_crash_dump', 3)
 
 
-class TestServerList(TestServer):
+class _TestServerList(TestServer):
 
     # Columns to be listed up.
     columns = (
@@ -2585,7 +2585,7 @@ class TestServerList(TestServer):
     )
 
     def setUp(self):
-        super(TestServerList, self).setUp()
+        super(_TestServerList, self).setUp()
 
         self.search_opts = {
             'reservation_id': None,
@@ -2643,10 +2643,11 @@ class TestServerList(TestServer):
         # Get the command object to test
         self.cmd = server.ListServer(self.app, None)
 
-        # Prepare data returned by fake Nova API.
-        self.data = []
-        self.data_long = []
-        self.data_no_name_lookup = []
+
+class TestServerList(_TestServerList):
+
+    def setUp(self):
+        super(TestServerList, self).setUp()
 
         Image = collections.namedtuple('Image', 'id name')
         self.images_mock.return_value = [
@@ -2661,8 +2662,8 @@ class TestServerList(TestServer):
             for s in self.servers
         ]
 
-        for s in self.servers:
-            self.data.append((
+        self.data = tuple(
+            (
                 s.id,
                 s.name,
                 s.status,
@@ -2670,34 +2671,8 @@ class TestServerList(TestServer):
                 # Image will be an empty string if boot-from-volume
                 self.image.name if s.image else server.IMAGE_STRING_FOR_BFV,
                 self.flavor.name,
-            ))
-            self.data_long.append((
-                s.id,
-                s.name,
-                s.status,
-                getattr(s, 'OS-EXT-STS:task_state'),
-                server._format_servers_list_power_state(
-                    getattr(s, 'OS-EXT-STS:power_state')
-                ),
-                server._format_servers_list_networks(s.networks),
-                # Image will be an empty string if boot-from-volume
-                self.image.name if s.image else server.IMAGE_STRING_FOR_BFV,
-                s.image['id'] if s.image else server.IMAGE_STRING_FOR_BFV,
-                self.flavor.name,
-                s.flavor['id'],
-                getattr(s, 'OS-EXT-AZ:availability_zone'),
-                getattr(s, 'OS-EXT-SRV-ATTR:host'),
-                s.Metadata,
-            ))
-            self.data_no_name_lookup.append((
-                s.id,
-                s.name,
-                s.status,
-                server._format_servers_list_networks(s.networks),
-                # Image will be an empty string if boot-from-volume
-                s.image['id'] if s.image else server.IMAGE_STRING_FOR_BFV,
-                s.flavor['id']
-            ))
+            ) for s in self.servers
+        )
 
     def test_server_list_no_option(self):
         arglist = []
@@ -2718,7 +2693,7 @@ class TestServerList(TestServer):
         self.assertFalse(self.flavors_mock.get.call_count)
         self.assertFalse(self.get_image_mock.call_count)
         self.assertEqual(self.columns, columns)
-        self.assertEqual(tuple(self.data), tuple(data))
+        self.assertEqual(self.data, tuple(data))
 
     def test_server_list_no_servers(self):
         arglist = []
@@ -2737,9 +2712,28 @@ class TestServerList(TestServer):
         self.assertEqual(0, self.images_mock.list.call_count)
         self.assertEqual(0, self.flavors_mock.list.call_count)
         self.assertEqual(self.columns, columns)
-        self.assertEqual(tuple(self.data), tuple(data))
+        self.assertEqual(self.data, tuple(data))
 
     def test_server_list_long_option(self):
+        self.data = tuple(
+            (
+                s.id,
+                s.name,
+                s.status,
+                getattr(s, 'OS-EXT-STS:task_state'),
+                server._format_servers_list_power_state(
+                    getattr(s, 'OS-EXT-STS:power_state')
+                ),
+                server._format_servers_list_networks(s.networks),
+                # Image will be an empty string if boot-from-volume
+                self.image.name if s.image else server.IMAGE_STRING_FOR_BFV,
+                s.image['id'] if s.image else server.IMAGE_STRING_FOR_BFV,
+                self.flavor.name,
+                s.flavor['id'],
+                getattr(s, 'OS-EXT-AZ:availability_zone'),
+                getattr(s, 'OS-EXT-SRV-ATTR:host'),
+                s.Metadata,
+            ) for s in self.servers)
         arglist = [
             '--long',
         ]
@@ -2750,12 +2744,23 @@ class TestServerList(TestServer):
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
 
         columns, data = self.cmd.take_action(parsed_args)
-
         self.servers_mock.list.assert_called_with(**self.kwargs)
         self.assertEqual(self.columns_long, columns)
-        self.assertEqual(tuple(self.data_long), tuple(data))
+        self.assertEqual(self.data, tuple(data))
 
     def test_server_list_no_name_lookup_option(self):
+        self.data = tuple(
+            (
+                s.id,
+                s.name,
+                s.status,
+                server._format_servers_list_networks(s.networks),
+                # Image will be an empty string if boot-from-volume
+                s.image['id'] if s.image else server.IMAGE_STRING_FOR_BFV,
+                s.flavor['id']
+            ) for s in self.servers
+        )
+
         arglist = [
             '--no-name-lookup',
         ]
@@ -2769,9 +2774,21 @@ class TestServerList(TestServer):
 
         self.servers_mock.list.assert_called_with(**self.kwargs)
         self.assertEqual(self.columns, columns)
-        self.assertEqual(tuple(self.data_no_name_lookup), tuple(data))
+        self.assertEqual(self.data, tuple(data))
 
     def test_server_list_n_option(self):
+        self.data = tuple(
+            (
+                s.id,
+                s.name,
+                s.status,
+                server._format_servers_list_networks(s.networks),
+                # Image will be an empty string if boot-from-volume
+                s.image['id'] if s.image else server.IMAGE_STRING_FOR_BFV,
+                s.flavor['id']
+            ) for s in self.servers
+        )
+
         arglist = [
             '-n',
         ]
@@ -2785,7 +2802,7 @@ class TestServerList(TestServer):
 
         self.servers_mock.list.assert_called_with(**self.kwargs)
         self.assertEqual(self.columns, columns)
-        self.assertEqual(tuple(self.data_no_name_lookup), tuple(data))
+        self.assertEqual(self.data, tuple(data))
 
     def test_server_list_name_lookup_one_by_one(self):
         arglist = [
@@ -2807,7 +2824,7 @@ class TestServerList(TestServer):
         self.flavors_mock.get.assert_called()
 
         self.assertEqual(self.columns, columns)
-        self.assertEqual(tuple(self.data), tuple(data))
+        self.assertEqual(self.data, tuple(data))
 
     def test_server_list_with_image(self):
 
@@ -2828,145 +2845,213 @@ class TestServerList(TestServer):
         self.servers_mock.list.assert_called_with(**self.kwargs)
 
         self.assertEqual(self.columns, columns)
-        self.assertEqual(tuple(self.data), tuple(data))
+        self.assertEqual(self.data, tuple(data))
 
-    def test_server_list_with_locked_pre_v273(self):
+    def test_server_list_with_flavor(self):
 
         arglist = [
-            '--locked'
+            '--flavor', self.flavor.id
         ]
         verifylist = [
-            ('locked', True)
+            ('flavor', self.flavor.id)
         ]
 
         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.73 or greater is required', str(ex))
+        columns, data = self.cmd.take_action(parsed_args)
 
-    def test_server_list_with_locked_v273(self):
+        self.flavors_mock.get.has_calls(self.flavor.id)
+
+        self.search_opts['flavor'] = self.flavor.id
+        self.servers_mock.list.assert_called_with(**self.kwargs)
+
+        self.assertEqual(self.columns, columns)
+        self.assertEqual(self.data, tuple(data))
+
+    def test_server_list_with_changes_since(self):
 
-        self.app.client_manager.compute.api_version = \
-            api_versions.APIVersion('2.73')
         arglist = [
-            '--locked'
+            '--changes-since', '2016-03-04T06:27:59Z',
+            '--deleted'
         ]
         verifylist = [
-            ('locked', True)
+            ('changes_since', '2016-03-04T06:27:59Z'),
+            ('deleted', True),
         ]
 
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
         columns, data = self.cmd.take_action(parsed_args)
 
-        self.search_opts['locked'] = True
+        self.search_opts['changes-since'] = '2016-03-04T06:27:59Z'
+        self.search_opts['deleted'] = True
         self.servers_mock.list.assert_called_with(**self.kwargs)
 
         self.assertEqual(self.columns, columns)
-        self.assertEqual(tuple(self.data), tuple(data))
+        self.assertEqual(self.data, tuple(data))
 
-    def test_server_list_with_unlocked_v273(self):
+    @mock.patch.object(timeutils, 'parse_isotime', side_effect=ValueError)
+    def test_server_list_with_invalid_changes_since(self, mock_parse_isotime):
 
-        self.app.client_manager.compute.api_version = \
-            api_versions.APIVersion('2.73')
         arglist = [
-            '--unlocked'
+            '--changes-since', 'Invalid time value',
         ]
         verifylist = [
-            ('unlocked', True)
+            ('changes_since', 'Invalid time value'),
         ]
 
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
-        columns, data = self.cmd.take_action(parsed_args)
+        try:
+            self.cmd.take_action(parsed_args)
+            self.fail('CommandError should be raised.')
+        except exceptions.CommandError as e:
+            self.assertEqual('Invalid changes-since value: Invalid time '
+                             'value', str(e))
+        mock_parse_isotime.assert_called_once_with(
+            'Invalid time value'
+        )
 
-        self.search_opts['locked'] = False
-        self.servers_mock.list.assert_called_with(**self.kwargs)
 
-        self.assertEqual(self.columns, columns)
-        self.assertEqual(tuple(self.data), tuple(data))
+class TestServerListV273(_TestServerList):
+
+    # Columns to be listed up.
+    columns = (
+        'ID',
+        'Name',
+        'Status',
+        'Networks',
+        'Image',
+        'Flavor',
+    )
+    columns_long = (
+        'ID',
+        'Name',
+        'Status',
+        'Task State',
+        'Power State',
+        'Networks',
+        'Image Name',
+        'Image ID',
+        'Flavor',
+        'Availability Zone',
+        'Host',
+        'Properties',
+    )
 
-    def test_server_list_with_locked_and_unlocked_v273(self):
+    def setUp(self):
+        super(TestServerListV273, self).setUp()
+
+        # The fake servers' attributes. Use the original attributes names in
+        # nova, not the ones printed by "server list" command.
+        self.attrs['flavor'] = {
+            'vcpus': self.flavor.vcpus,
+            'ram': self.flavor.ram,
+            'disk': self.flavor.disk,
+            'ephemeral': self.flavor.ephemeral,
+            'swap': self.flavor.swap,
+            'original_name': self.flavor.name,
+            'extra_specs': self.flavor.properties,
+        }
+
+        # The servers to be listed.
+        self.servers = self.setup_servers_mock(3)
+        self.servers_mock.list.return_value = self.servers
+
+        Image = collections.namedtuple('Image', 'id name')
+        self.images_mock.return_value = [
+            Image(id=s.image['id'], name=self.image.name)
+            # Image will be an empty string if boot-from-volume
+            for s in self.servers if s.image
+        ]
+
+        # The flavor information is embedded, so now reason for this to be
+        # called
+        self.flavors_mock.list = mock.NonCallableMock()
+
+        self.data = tuple(
+            (
+                s.id,
+                s.name,
+                s.status,
+                server._format_servers_list_networks(s.networks),
+                # Image will be an empty string if boot-from-volume
+                self.image.name if s.image else server.IMAGE_STRING_FOR_BFV,
+                self.flavor.name,
+            ) for s in self.servers)
+
+    def test_server_list_with_locked_pre_v273(self):
 
-        self.app.client_manager.compute.api_version = \
-            api_versions.APIVersion('2.73')
         arglist = [
-            '--locked',
-            '--unlocked'
+            '--locked'
         ]
         verifylist = [
-            ('locked', True),
-            ('unlocked', True)
+            ('locked', True)
         ]
 
-        ex = self.assertRaises(
-            utils.ParserException,
-            self.check_parser, self.cmd, arglist, verifylist)
-        self.assertIn('Argument parse failed', str(ex))
+        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.73 or greater is required', str(ex))
 
-    def test_server_list_with_flavor(self):
+    def test_server_list_with_locked(self):
 
+        self.app.client_manager.compute.api_version = \
+            api_versions.APIVersion('2.73')
         arglist = [
-            '--flavor', self.flavor.id
+            '--locked'
         ]
         verifylist = [
-            ('flavor', self.flavor.id)
+            ('locked', True)
         ]
 
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
         columns, data = self.cmd.take_action(parsed_args)
 
-        self.flavors_mock.get.has_calls(self.flavor.id)
-
-        self.search_opts['flavor'] = self.flavor.id
+        self.search_opts['locked'] = True
         self.servers_mock.list.assert_called_with(**self.kwargs)
 
-        self.assertEqual(self.columns, columns)
-        self.assertEqual(tuple(self.data), tuple(data))
+        self.assertItemsEqual(self.columns, columns)
+        self.assertItemsEqual(self.data, tuple(data))
 
-    def test_server_list_with_changes_since(self):
+    def test_server_list_with_unlocked_v273(self):
 
+        self.app.client_manager.compute.api_version = \
+            api_versions.APIVersion('2.73')
         arglist = [
-            '--changes-since', '2016-03-04T06:27:59Z',
-            '--deleted'
+            '--unlocked'
         ]
         verifylist = [
-            ('changes_since', '2016-03-04T06:27:59Z'),
-            ('deleted', True),
+            ('unlocked', True)
         ]
 
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
         columns, data = self.cmd.take_action(parsed_args)
 
-        self.search_opts['changes-since'] = '2016-03-04T06:27:59Z'
-        self.search_opts['deleted'] = True
+        self.search_opts['locked'] = False
         self.servers_mock.list.assert_called_with(**self.kwargs)
 
-        self.assertEqual(self.columns, columns)
-        self.assertEqual(tuple(self.data), tuple(data))
+        self.assertItemsEqual(self.columns, columns)
+        self.assertItemsEqual(self.data, tuple(data))
 
-    @mock.patch.object(timeutils, 'parse_isotime', side_effect=ValueError)
-    def test_server_list_with_invalid_changes_since(self, mock_parse_isotime):
+    def test_server_list_with_locked_and_unlocked(self):
 
+        self.app.client_manager.compute.api_version = \
+            api_versions.APIVersion('2.73')
         arglist = [
-            '--changes-since', 'Invalid time value',
+            '--locked',
+            '--unlocked'
         ]
         verifylist = [
-            ('changes_since', 'Invalid time value'),
+            ('locked', True),
+            ('unlocked', True)
         ]
 
-        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
-        try:
-            self.cmd.take_action(parsed_args)
-            self.fail('CommandError should be raised.')
-        except exceptions.CommandError as e:
-            self.assertEqual('Invalid changes-since value: Invalid time '
-                             'value', str(e))
-        mock_parse_isotime.assert_called_once_with(
-            'Invalid time value'
-        )
+        ex = self.assertRaises(
+            utils.ParserException,
+            self.check_parser, self.cmd, arglist, verifylist)
+        self.assertIn('Argument parse failed', str(ex))
 
-    def test_server_list_v266_with_changes_before(self):
+    def test_server_list_with_changes_before(self):
         self.app.client_manager.compute.api_version = (
             api_versions.APIVersion('2.66'))
         arglist = [
@@ -2983,13 +3068,14 @@ class TestServerList(TestServer):
 
         self.search_opts['changes-before'] = '2016-03-05T06:27:59Z'
         self.search_opts['deleted'] = True
+
         self.servers_mock.list.assert_called_with(**self.kwargs)
 
-        self.assertEqual(self.columns, columns)
-        self.assertEqual(tuple(self.data), tuple(data))
+        self.assertItemsEqual(self.columns, columns)
+        self.assertItemsEqual(self.data, tuple(data))
 
     @mock.patch.object(timeutils, 'parse_isotime', side_effect=ValueError)
-    def test_server_list_v266_with_invalid_changes_before(
+    def test_server_list_with_invalid_changes_before(
             self, mock_parse_isotime):
         self.app.client_manager.compute.api_version = (
             api_versions.APIVersion('2.66'))