Merge "compute: Show flavor in 'server list' with API >= 2.47" into stable/ussuri

3 files changed, 231 insertions, 135 deletions

diff --git a/openstackclient/compute/v2/server.py b/openstackclient/compute/v2/server.py
index 224b8d5d..194ccc8c 100644
--- a/openstackclient/compute/v2/server.py
+++ b/openstackclient/compute/v2/server.py
@@ -1422,21 +1422,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 += (
@@ -1539,18 +1546,13 @@ class ListServer(command.Lister):
                 s.image_name = ''
                 s.image_id = ''
 
-            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 8ec8217d..c715e814 100644
--- a/openstackclient/tests/unit/compute/v2/test_server.py
+++ b/openstackclient/tests/unit/compute/v2/test_server.py
@@ -2498,7 +2498,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 = (
@@ -2526,7 +2526,7 @@ class TestServerList(TestServer):
     )
 
     def setUp(self):
-        super(TestServerList, self).setUp()
+        super(_TestServerList, self).setUp()
 
         self.search_opts = {
             'reservation_id': None,
@@ -2584,10 +2584,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 = [
@@ -2602,8 +2603,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,
@@ -2611,34 +2612,8 @@ class TestServerList(TestServer):
                 # Image will be an empty string if boot-from-volume
                 self.image.name if s.image else s.image,
                 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 s.image,
-                s.image['id'] if s.image else s.image,
-                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 s.image,
-                s.flavor['id']
-            ))
+            ) for s in self.servers
+        )
 
     def test_server_list_no_option(self):
         arglist = []
@@ -2659,7 +2634,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 = []
@@ -2678,9 +2653,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 s.image,
+                s.image['id'] if s.image else s.image,
+                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',
         ]
@@ -2691,12 +2685,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 s.image,
+                s.flavor['id']
+            ) for s in self.servers
+        )
+
         arglist = [
             '--no-name-lookup',
         ]
@@ -2710,9 +2715,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 s.image,
+                s.flavor['id']
+            ) for s in self.servers
+        )
+
         arglist = [
             '-n',
         ]
@@ -2726,7 +2743,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 = [
@@ -2748,7 +2765,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):
 
@@ -2769,145 +2786,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 s.image,
+                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 = [
@@ -2924,13 +3009,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'))
diff --git a/releasenotes/notes/fix-flavor-in-server-list-microversion-2.47-af200e9bb4747e2d.yaml b/releasenotes/notes/fix-flavor-in-server-list-microversion-2.47-af200e9bb4747e2d.yaml
new file mode 100644
index 00000000..fdb37bbb
--- /dev/null
+++ b/releasenotes/notes/fix-flavor-in-server-list-microversion-2.47-af200e9bb4747e2d.yaml
@@ -0,0 +1,8 @@
+---
+features:
+  - |
+    Add support for compute API microversion 2.47, which changes how flavor
+    details are included in server detail responses. In 2.46 and below,
+    only the flavor ID was shown in the server detail response. Starting in
+    2.47, flavor information is embedded in the server response. The newer
+    behavior is now supported.