From bae89b30144fdf40d0fea31e1a595b507e32c4f3 Mon Sep 17 00:00:00 2001
From: jay <jayadityagupta11@gmail.com>
Date: Thu, 4 Jun 2020 14:09:03 +0200
Subject: [PATCH] Output correct json for security groups in 'openstack server
 show'

Fixes incorrect json output for 'openstack server show -f json'.
The security group json output groups all the json as one
for e.g. "security_groups": "name='group1'\nname='group2'"
The correct output should be
"security_groups" : [{"name" : "group1"}, {"name" : "group2"}]
properties and volumes_attached fields also has similar issue.

Story: 2007755

Change-Id: I1b1cac716329e0530400aff782c08000b21d8e1d
---
 openstackclient/compute/v2/server.py          | 17 +++++---
 .../functional/compute/v2/test_server.py      | 39 ++++++++++++-------
 .../tests/unit/compute/v2/test_server.py      |  3 ++
 ...ty-grp-json-fix.yaml-2af1f48a48034d64.yaml |  4 ++
 4 files changed, 42 insertions(+), 21 deletions(-)
 create mode 100644 releasenotes/notes/security-grp-json-fix.yaml-2af1f48a48034d64.yaml

diff --git a/openstackclient/compute/v2/server.py b/openstackclient/compute/v2/server.py
index 93e9f966ae..b3b0b7be15 100644
--- a/openstackclient/compute/v2/server.py
+++ b/openstackclient/compute/v2/server.py
@@ -24,6 +24,7 @@ import os
 from novaclient import api_versions
 from novaclient.v2 import servers
 from openstack import exceptions as sdk_exceptions
+from osc_lib.cli import format_columns
 from osc_lib.cli import parseractions
 from osc_lib.command import command
 from osc_lib import exceptions
@@ -166,14 +167,14 @@ def _prep_server_detail(compute_client, image_client, server, refresh=True):
     if 'os-extended-volumes:volumes_attached' in info:
         info.update(
             {
-                'volumes_attached': utils.format_list_of_dicts(
+                'volumes_attached': format_columns.ListDictColumn(
                     info.pop('os-extended-volumes:volumes_attached'))
             }
         )
     if 'security_groups' in info:
         info.update(
             {
-                'security_groups': utils.format_list_of_dicts(
+                'security_groups': format_columns.ListDictColumn(
                     info.pop('security_groups'))
             }
         )
@@ -182,9 +183,14 @@ def _prep_server_detail(compute_client, image_client, server, refresh=True):
     info['addresses'] = _format_servers_list_networks(server.networks)
 
     # Map 'metadata' field to 'properties'
-    info.update(
-        {'properties': utils.format_dict(info.pop('metadata'))}
-    )
+    if not info['metadata']:
+        info.update(
+            {'properties': utils.format_dict(info.pop('metadata'))}
+        )
+    else:
+        info.update(
+            {'properties': format_columns.DictColumn(info.pop('metadata'))}
+        )
 
     # Migrate tenant_id to project_id naming
     if 'tenant_id' in info:
@@ -2530,7 +2536,6 @@ class ShowServer(command.ShowOne):
             data = _prep_server_detail(compute_client,
                                        self.app.client_manager.image, server,
                                        refresh=False)
-
         return zip(*sorted(data.items()))
 
 
diff --git a/openstackclient/tests/functional/compute/v2/test_server.py b/openstackclient/tests/functional/compute/v2/test_server.py
index 6e080e9ba2..50c59c17a3 100644
--- a/openstackclient/tests/functional/compute/v2/test_server.py
+++ b/openstackclient/tests/functional/compute/v2/test_server.py
@@ -230,7 +230,7 @@ class ServerTests(common.ComputeTestCase):
         ))
         # Really, shouldn't this be a list?
         self.assertEqual(
-            "a='b', c='d'",
+            {'a': 'b', 'c': 'd'},
             cmd_output['properties'],
         )
 
@@ -244,7 +244,7 @@ class ServerTests(common.ComputeTestCase):
             name
         ))
         self.assertEqual(
-            "c='d'",
+            {'c': 'd'},
             cmd_output['properties'],
         )
 
@@ -619,8 +619,8 @@ class ServerTests(common.ComputeTestCase):
             server_name
         ))
         volumes_attached = cmd_output['volumes_attached']
-        self.assertTrue(volumes_attached.startswith('id='))
-        attached_volume_id = volumes_attached.replace('id=', '')
+        self.assertIsNotNone(volumes_attached)
+        attached_volume_id = volumes_attached[0]["id"]
 
         # check the volume that attached on server
         cmd_output = json.loads(self.openstack(
@@ -699,8 +699,8 @@ class ServerTests(common.ComputeTestCase):
             server_name
         ))
         volumes_attached = cmd_output['volumes_attached']
-        self.assertTrue(volumes_attached.startswith('id='))
-        attached_volume_id = volumes_attached.replace('id=', '')
+        self.assertIsNotNone(volumes_attached)
+        attached_volume_id = volumes_attached[0]["id"]
 
         # check the volume that attached on server
         cmd_output = json.loads(self.openstack(
@@ -773,10 +773,12 @@ class ServerTests(common.ComputeTestCase):
             server_name
         ))
         volumes_attached = cmd_output['volumes_attached']
-        self.assertTrue(volumes_attached.startswith('id='))
-        attached_volume_id = volumes_attached.replace('id=', '')
-        # Don't leak the volume when the test exits.
-        self.addCleanup(self.openstack, 'volume delete ' + attached_volume_id)
+        self.assertIsNotNone(volumes_attached)
+        attached_volume_id = volumes_attached[0]["id"]
+        for vol in volumes_attached:
+            self.assertIsNotNone(vol['id'])
+            # Don't leak the volume when the test exits.
+            self.addCleanup(self.openstack, 'volume delete ' + vol['id'])
 
         # Since the server is volume-backed the GET /servers/{server_id}
         # response will have image=''.
@@ -785,7 +787,7 @@ class ServerTests(common.ComputeTestCase):
         # check the volume that attached on server
         cmd_output = json.loads(self.openstack(
             'volume show -f json ' +
-            attached_volume_id
+            volumes_attached[0]["id"]
         ))
         # The volume size should be what we specified on the command line.
         self.assertEqual(1, int(cmd_output['size']))
@@ -879,14 +881,21 @@ class ServerTests(common.ComputeTestCase):
 
         self.assertIsNotNone(server['id'])
         self.assertEqual(server_name, server['name'])
-        self.assertIn(str(security_group1['id']), server['security_groups'])
-        self.assertIn(str(security_group2['id']), server['security_groups'])
+        sec_grp = ""
+        for sec in server['security_groups']:
+            sec_grp += sec['name']
+        self.assertIn(str(security_group1['id']), sec_grp)
+        self.assertIn(str(security_group2['id']), sec_grp)
         self.wait_for_status(server_name, 'ACTIVE')
         server = json.loads(self.openstack(
             'server show -f json ' + server_name
         ))
-        self.assertIn(sg_name1, server['security_groups'])
-        self.assertIn(sg_name2, server['security_groups'])
+        # check if security group exists in list
+        sec_grp = ""
+        for sec in server['security_groups']:
+            sec_grp += sec['name']
+        self.assertIn(sg_name1, sec_grp)
+        self.assertIn(sg_name2, sec_grp)
 
     def test_server_create_with_empty_network_option_latest(self):
         """Test server create with empty network option in nova 2.latest."""
diff --git a/openstackclient/tests/unit/compute/v2/test_server.py b/openstackclient/tests/unit/compute/v2/test_server.py
index 7e4c71c50c..933c4e7ddd 100644
--- a/openstackclient/tests/unit/compute/v2/test_server.py
+++ b/openstackclient/tests/unit/compute/v2/test_server.py
@@ -5166,6 +5166,8 @@ class TestServerGeneral(TestServer):
             'tenant_id': u'tenant-id-xxx',
             'networks': {u'public': [u'10.20.30.40', u'2001:db8::f']},
             'links': u'http://xxx.yyy.com',
+            'properties': '',
+            'volumes_attached': [{"id": "6344fe9d-ef20-45b2-91a6"}],
         }
         _server = compute_fakes.FakeServer.create_one_server(attrs=server_info)
         find_resource.side_effect = [_server, _flavor]
@@ -5182,6 +5184,7 @@ class TestServerGeneral(TestServer):
             'properties': '',
             'OS-EXT-STS:power_state': server._format_servers_list_power_state(
                 getattr(_server, 'OS-EXT-STS:power_state')),
+            'volumes_attached': [{"id": "6344fe9d-ef20-45b2-91a6"}],
         }
 
         # Call _prep_server_detail().
diff --git a/releasenotes/notes/security-grp-json-fix.yaml-2af1f48a48034d64.yaml b/releasenotes/notes/security-grp-json-fix.yaml-2af1f48a48034d64.yaml
new file mode 100644
index 0000000000..3a0155a1da
--- /dev/null
+++ b/releasenotes/notes/security-grp-json-fix.yaml-2af1f48a48034d64.yaml
@@ -0,0 +1,4 @@
+---
+fixes:
+  - The ``openstack server show -f json`` command was not outputting
+    json for security groups, volumes and properties  properly.