compute: Fix 'server * -f yaml' output

Make use of 'FormattableColumn'-derived formatters, which provide better
output than what we were using before, particularly for the YAML output
format. For example, compare before for the 'server show' command:

  $ openstack --os-compute-api-version 2.79 server show test-server -f yaml
  ...
  addresses: private=fdff:77e3:9bb4:0:f816:3eff:fe6d:a944, 10.0.0.44
  flavor: disk='1', ephemeral='0', extra_specs.hw_rng:allowed='True', original_name='m1.tiny',
    ram='512', swap='0', vcpus='1'
  ...

To after:

  $ openstack --os-compute-api-version 2.79 server show test-server -f yaml
  ...
  addresses:
    private:
    - fdff:77e3:9bb4:0:f816:3eff:fe6d:a944
    - 10.0.0.44
  flavor:
    disk: 1
    ephemeral: 0
    extra_specs:
      hw_rng:allowed: 'True'
    original_name: m1.tiny
    ram: 512
    swap: 0
    vcpus: 1
  ...

Similarly, compare before for 'server list':

  $ openstack --os-compute-api-version 2.79 server list -f yaml
  - ...
    Networks: private=fdff:77e3:9bb4:0:f816:3eff:fe6d:a944, 10.0.0.44
    Power State: Running
    Properties: ''
    ...

To after:

  $ openstack --os-compute-api-version 2.79 server list -f yaml
  - ...
    Networks:
      private:
      - fdff:77e3:9bb4:0:f816:3eff:fe6d:a944
      - 10.0.0.44
    Power State: 1
    Properties: {}
    ...

We also fix the human-readable output for the 'tags' field.

Before:

  $ openstack --os-compute-api-version 2.79 server list
  ...
  | tags   | ['bar', 'foo']  |

After:

  $ openstack --os-compute-api-version 2.79 server list
  ...
  | tags   | bar, foo  |

Change-Id: I7a8349106e211c57c4577b75326b39b88bd9ac1e
Signed-off-by: Stephen Finucane <sfinucan@redhat.com>

3 files changed, 89 insertions, 115 deletions

diff --git a/openstackclient/compute/v2/server.py b/openstackclient/compute/v2/server.py
index 2512a877..ba0243ef 100644
--- a/openstackclient/compute/v2/server.py
+++ b/openstackclient/compute/v2/server.py
@@ -21,6 +21,7 @@ import io
 import logging
 import os
 
+from cliff import columns as cliff_columns
 import iso8601
 from novaclient import api_versions
 from novaclient.v2 import servers
@@ -41,28 +42,9 @@ LOG = logging.getLogger(__name__)
 IMAGE_STRING_FOR_BFV = 'N/A (booted from volume)'
 
 
-def _format_servers_list_networks(networks):
-    """Return a formatted string of a server's networks
+class PowerStateColumn(cliff_columns.FormattableColumn):
+    """Generate a formatted string of a server's power state."""
 
-    :param networks: a Server.networks field
-    :rtype: a string of formatted network addresses
-    """
-    output = []
-    for (network, addresses) in networks.items():
-        if not addresses:
-            continue
-        addresses_csv = ', '.join(addresses)
-        group = "%s=%s" % (network, addresses_csv)
-        output.append(group)
-    return '; '.join(output)
-
-
-def _format_servers_list_power_state(state):
-    """Return a formatted string of a server's power state
-
-    :param state: the power state number of a server
-    :rtype: a string mapped to the power state number
-    """
     power_states = [
         'NOSTATE',      # 0x00
         'Running',      # 0x01
@@ -74,10 +56,11 @@ def _format_servers_list_power_state(state):
         'Suspended'     # 0x07
     ]
 
-    try:
-        return power_states[state]
-    except Exception:
-        return 'N/A'
+    def human_readable(self):
+        try:
+            return self.power_states[self._value]
+        except Exception:
+            return 'N/A'
 
 
 def _get_ip_address(addresses, address_type, ip_address_family):
@@ -169,7 +152,7 @@ def _prep_server_detail(compute_client, image_client, server, refresh=True):
         except Exception:
             info['flavor'] = flavor_id
     else:
-        info['flavor'] = utils.format_dict(flavor_info)
+        info['flavor'] = format_columns.DictColumn(flavor_info)
 
     if 'os-extended-volumes:volumes_attached' in info:
         info.update(
@@ -185,19 +168,15 @@ def _prep_server_detail(compute_client, image_client, server, refresh=True):
                     info.pop('security_groups'))
             }
         )
+    if 'tags' in info:
+        info.update({'tags': format_columns.ListColumn(info.pop('tags'))})
+
     # NOTE(dtroyer): novaclient splits these into separate entries...
     # Format addresses in a useful way
-    info['addresses'] = _format_servers_list_networks(server.networks)
+    info['addresses'] = format_columns.DictListColumn(server.networks)
 
     # Map 'metadata' field to 'properties'
-    if not info['metadata']:
-        info.update(
-            {'properties': utils.format_dict(info.pop('metadata'))}
-        )
-    else:
-        info.update(
-            {'properties': format_columns.DictColumn(info.pop('metadata'))}
-        )
+    info['properties'] = format_columns.DictColumn(info.pop('metadata'))
 
     # Migrate tenant_id to project_id naming
     if 'tenant_id' in info:
@@ -205,7 +184,7 @@ def _prep_server_detail(compute_client, image_client, server, refresh=True):
 
     # Map power state num to meaningful string
     if 'OS-EXT-STS:power_state' in info:
-        info['OS-EXT-STS:power_state'] = _format_servers_list_power_state(
+        info['OS-EXT-STS:power_state'] = PowerStateColumn(
             info['OS-EXT-STS:power_state'])
 
     # Remove values that are long and not too useful
@@ -1873,10 +1852,9 @@ class ListServer(command.Lister):
                     s, columns,
                     mixed_case_fields=mixed_case_fields,
                     formatters={
-                        'OS-EXT-STS:power_state':
-                            _format_servers_list_power_state,
-                        'Networks': _format_servers_list_networks,
-                        'Metadata': utils.format_dict,
+                        'OS-EXT-STS:power_state': PowerStateColumn,
+                        'Networks': format_columns.DictListColumn,
+                        'Metadata': format_columns.DictColumn,
                     },
                 ) for s in data
             ),
diff --git a/openstackclient/tests/functional/compute/v2/test_server.py b/openstackclient/tests/functional/compute/v2/test_server.py
index 44d9c61f..bad3f93d 100644
--- a/openstackclient/tests/functional/compute/v2/test_server.py
+++ b/openstackclient/tests/functional/compute/v2/test_server.py
@@ -10,6 +10,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import itertools
 import json
 import time
 import uuid
@@ -346,6 +347,14 @@ class ServerTests(common.ComputeTestCase):
             #                DevStack without cells.
             self.skipTest("No Network service present")
 
+        def _chain_addresses(addresses):
+            # Flatten a dict of lists mapping network names to IP addresses,
+            # returning only the IP addresses
+            #
+            # >>> _chain_addresses({'private': ['10.1.0.32', '172.24.5.41']})
+            # ['10.1.0.32', '172.24.5.41']
+            return itertools.chain(*[*addresses.values()])
+
         cmd_output = self.server_create()
         name = cmd_output['name']
         self.wait_for_status(name, "ACTIVE")
@@ -387,7 +396,7 @@ class ServerTests(common.ComputeTestCase):
                 'server show -f json ' +
                 name
             ))
-            if floating_ip not in cmd_output['addresses']:
+            if floating_ip not in _chain_addresses(cmd_output['addresses']):
                 # Hang out for a bit and try again
                 print('retrying floating IP check')
                 wait_time += 10
@@ -397,7 +406,7 @@ class ServerTests(common.ComputeTestCase):
 
         self.assertIn(
             floating_ip,
-            cmd_output['addresses'],
+            _chain_addresses(cmd_output['addresses']),
         )
 
         # detach ip
@@ -417,7 +426,7 @@ class ServerTests(common.ComputeTestCase):
                 'server show -f json ' +
                 name
             ))
-            if floating_ip in cmd_output['addresses']:
+            if floating_ip in _chain_addresses(cmd_output['addresses']):
                 # Hang out for a bit and try again
                 print('retrying floating IP check')
                 wait_time += 10
@@ -431,7 +440,7 @@ class ServerTests(common.ComputeTestCase):
         ))
         self.assertNotIn(
             floating_ip,
-            cmd_output['addresses'],
+            _chain_addresses(cmd_output['addresses']),
         )
 
     def test_server_reboot(self):
@@ -856,8 +865,7 @@ class ServerTests(common.ComputeTestCase):
         server = json.loads(self.openstack(
             'server show -f json ' + server_name
         ))
-        self.assertIsNotNone(server['addresses'])
-        self.assertEqual('', server['addresses'])
+        self.assertEqual({}, server['addresses'])
 
     def test_server_create_with_security_group(self):
         """Test server create with security group ID and name"""
diff --git a/openstackclient/tests/unit/compute/v2/test_server.py b/openstackclient/tests/unit/compute/v2/test_server.py
index 1c854193..efc105e5 100644
--- a/openstackclient/tests/unit/compute/v2/test_server.py
+++ b/openstackclient/tests/unit/compute/v2/test_server.py
@@ -22,6 +22,7 @@ from unittest.mock import call
 import iso8601
 from novaclient import api_versions
 from openstack import exceptions as sdk_exceptions
+from osc_lib.cli import format_columns
 from osc_lib import exceptions
 from osc_lib import utils as common_utils
 
@@ -33,6 +34,29 @@ from openstackclient.tests.unit import utils
 from openstackclient.tests.unit.volume.v2 import fakes as volume_fakes
 
 
+class TestPowerStateColumn(utils.TestCase):
+
+    def test_human_readable(self):
+        self.assertEqual(
+            'NOSTATE', server.PowerStateColumn(0x00).human_readable())
+        self.assertEqual(
+            'Running', server.PowerStateColumn(0x01).human_readable())
+        self.assertEqual(
+            '', server.PowerStateColumn(0x02).human_readable())
+        self.assertEqual(
+            'Paused', server.PowerStateColumn(0x03).human_readable())
+        self.assertEqual(
+            'Shutdown', server.PowerStateColumn(0x04).human_readable())
+        self.assertEqual(
+            '', server.PowerStateColumn(0x05).human_readable())
+        self.assertEqual(
+            'Crashed', server.PowerStateColumn(0x06).human_readable())
+        self.assertEqual(
+            'Suspended', server.PowerStateColumn(0x07).human_readable())
+        self.assertEqual(
+            'N/A', server.PowerStateColumn(0x08).human_readable())
+
+
 class TestServer(compute_fakes.TestComputev2):
 
     def setUp(self):
@@ -1015,15 +1039,15 @@ class TestServerCreate(TestServer):
 
     def datalist(self):
         datalist = (
-            server._format_servers_list_power_state(
+            server.PowerStateColumn(
                 getattr(self.new_server, 'OS-EXT-STS:power_state')),
-            '',
+            format_columns.DictListColumn({}),
             self.flavor.name + ' (' + self.new_server.flavor.get('id') + ')',
             self.new_server.id,
             self.image.name + ' (' + self.new_server.image.get('id') + ')',
             self.new_server.name,
             self.new_server.networks,
-            '',
+            format_columns.DictColumn(self.new_server.metadata),
         )
         return datalist
 
@@ -3041,7 +3065,7 @@ class TestServerList(TestServer):
                 s.id,
                 s.name,
                 s.status,
-                server._format_servers_list_networks(s.networks),
+                format_columns.DictListColumn(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,
@@ -3051,10 +3075,10 @@ class TestServerList(TestServer):
                 s.name,
                 s.status,
                 getattr(s, 'OS-EXT-STS:task_state'),
-                server._format_servers_list_power_state(
+                server.PowerStateColumn(
                     getattr(s, 'OS-EXT-STS:power_state')
                 ),
-                server._format_servers_list_networks(s.networks),
+                format_columns.DictListColumn(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,
@@ -3062,13 +3086,13 @@ class TestServerList(TestServer):
                 s.flavor['id'],
                 getattr(s, 'OS-EXT-AZ:availability_zone'),
                 getattr(s, 'OS-EXT-SRV-ATTR:host'),
-                s.Metadata,
+                format_columns.DictColumn({}),
             ))
             self.data_no_name_lookup.append((
                 s.id,
                 s.name,
                 s.status,
-                server._format_servers_list_networks(s.networks),
+                format_columns.DictListColumn(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']
@@ -3128,7 +3152,7 @@ class TestServerList(TestServer):
 
         self.servers_mock.list.assert_called_with(**self.kwargs)
         self.assertEqual(self.columns_long, columns)
-        self.assertEqual(tuple(self.data_long), tuple(data))
+        self.assertCountEqual(tuple(self.data_long), tuple(data))
 
     def test_server_list_column_option(self):
         arglist = [
@@ -3429,9 +3453,11 @@ class TestServerList(TestServer):
     def test_server_list_v269_with_partial_constructs(self):
         self.app.client_manager.compute.api_version = \
             api_versions.APIVersion('2.69')
+
         arglist = []
         verifylist = []
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
         # include "partial results" from non-responsive part of
         # infrastructure.
         server_dict = {
@@ -3454,10 +3480,10 @@ class TestServerList(TestServer):
             # it will fail at formatting the networks info later on.
             "networks": {}
         }
-        server = compute_fakes.fakes.FakeResource(
+        _server = compute_fakes.fakes.FakeResource(
             info=server_dict,
         )
-        self.servers.append(server)
+        self.servers.append(_server)
         columns, data = self.cmd.take_action(parsed_args)
         # get the first three servers out since our interest is in the partial
         # server.
@@ -3466,8 +3492,13 @@ class TestServerList(TestServer):
         next(data)
         partial_server = next(data)
         expected_row = (
-            'server-id-95a56bfc4xxxxxx28d7e418bfd97813a', '',
-            'UNKNOWN', '', '', '')
+            'server-id-95a56bfc4xxxxxx28d7e418bfd97813a',
+            '',
+            'UNKNOWN',
+            format_columns.DictListColumn({}),
+            '',
+            '',
+        )
         self.assertEqual(expected_row, partial_server)
 
     def test_server_list_with_tag(self):
@@ -6292,15 +6323,16 @@ class TestServerShow(TestServer):
         )
 
         self.data = (
-            'Running',
-            'public=10.20.30.40, 2001:db8::f',
+            server.PowerStateColumn(
+                getattr(self.server, 'OS-EXT-STS:power_state')),
+            format_columns.DictListColumn(self.server.networks),
             self.flavor.name + " (" + self.flavor.id + ")",
             self.server.id,
             self.image.name + " (" + self.image.id + ")",
             self.server.name,
             {'public': ['10.20.30.40', '2001:db8::f']},
             'tenant-id-xxx',
-            '',
+            format_columns.DictColumn({}),
         )
 
     def test_show_no_options(self):
@@ -6323,7 +6355,7 @@ class TestServerShow(TestServer):
         columns, data = self.cmd.take_action(parsed_args)
 
         self.assertEqual(self.columns, columns)
-        self.assertEqual(self.data, data)
+        self.assertCountEqual(self.data, data)
 
     def test_show_embedded_flavor(self):
         # Tests using --os-compute-api-version >= 2.47 where the flavor
@@ -6350,7 +6382,7 @@ class TestServerShow(TestServer):
         self.assertEqual(self.columns, columns)
         # Since the flavor details are in a dict we can't be sure of the
         # ordering so just assert that one of the keys is in the output.
-        self.assertIn('original_name', data[2])
+        self.assertIn('original_name', data[2]._value)
 
     def test_show_diagnostics(self):
         arglist = [
@@ -6719,50 +6751,6 @@ class TestServerGeneral(TestServer):
         self.assertRaises(exceptions.CommandError,
                           server._get_ip_address, self.OLD, 'private', [6])
 
-    def test_format_servers_list_power_state(self):
-        self.assertEqual("NOSTATE",
-                         server._format_servers_list_power_state(0x00))
-        self.assertEqual("Running",
-                         server._format_servers_list_power_state(0x01))
-        self.assertEqual("",
-                         server._format_servers_list_power_state(0x02))
-        self.assertEqual("Paused",
-                         server._format_servers_list_power_state(0x03))
-        self.assertEqual("Shutdown",
-                         server._format_servers_list_power_state(0x04))
-        self.assertEqual("",
-                         server._format_servers_list_power_state(0x05))
-        self.assertEqual("Crashed",
-                         server._format_servers_list_power_state(0x06))
-        self.assertEqual("Suspended",
-                         server._format_servers_list_power_state(0x07))
-        self.assertEqual("N/A",
-                         server._format_servers_list_power_state(0x08))
-
-    def test_format_servers_list_networks(self):
-        # Setup network info to test.
-        networks = {
-            u'public': [u'10.20.30.40', u'2001:db8::f'],
-            u'private': [u'2001:db8::f', u'10.20.30.40'],
-        }
-
-        # Prepare expected data.
-        # Since networks is a dict, whose items are in random order, there
-        # could be two results after formatted.
-        data_1 = (u'private=2001:db8::f, 10.20.30.40; '
-                  u'public=10.20.30.40, 2001:db8::f')
-        data_2 = (u'public=10.20.30.40, 2001:db8::f; '
-                  u'private=2001:db8::f, 10.20.30.40')
-
-        # Call _format_servers_list_networks().
-        networks_format = server._format_servers_list_networks(networks)
-
-        msg = ('Network string is not formatted correctly.\n'
-               'reference = %s or %s\n'
-               'actual    = %s\n' %
-               (data_1, data_2, networks_format))
-        self.assertIn(networks_format, (data_1, data_2), msg)
-
     @mock.patch('osc_lib.utils.find_resource')
     def test_prep_server_detail(self, find_resource):
         # Setup mock method return value. utils.find_resource() will be called
@@ -6789,14 +6777,14 @@ class TestServerGeneral(TestServer):
         info = {
             'id': _server.id,
             'name': _server.name,
-            'addresses': u'public=10.20.30.40, 2001:db8::f',
-            'flavor': u'%s (%s)' % (_flavor.name, _flavor.id),
-            'image': u'%s (%s)' % (_image.name, _image.id),
-            'project_id': u'tenant-id-xxx',
-            'properties': '',
-            'OS-EXT-STS:power_state': server._format_servers_list_power_state(
+            'image': '%s (%s)' % (_image.name, _image.id),
+            'flavor': '%s (%s)' % (_flavor.name, _flavor.id),
+            'OS-EXT-STS:power_state': server.PowerStateColumn(
                 getattr(_server, 'OS-EXT-STS:power_state')),
+            'properties': '',
             'volumes_attached': [{"id": "6344fe9d-ef20-45b2-91a6"}],
+            'addresses': format_columns.DictListColumn(_server.networks),
+            'project_id': 'tenant-id-xxx',
         }
 
         # Call _prep_server_detail().
@@ -6809,4 +6797,4 @@ class TestServerGeneral(TestServer):
         server_detail.pop('networks')
 
         # Check the results.
-        self.assertEqual(info, server_detail)
+        self.assertCountEqual(info, server_detail)