Switch list server volume to sdk

Switch the server volume list command from novaclient to SDK.
Modified functional test for server add/remove volume.

Change-Id: I5b4ab7d0275aec2e02451c5371319ac350af6a5f

openstackclient/compute/v2/server_volume.py

@@ -15,6 +15,7 @@
 """Compute v2 Server action implementations"""
 
 from novaclient import api_versions
+from openstack import utils as sdk_utils
 from osc_lib.command import command
 from osc_lib import exceptions
 from osc_lib import utils
@@ -34,27 +35,25 @@ class ListServerVolume(command.Lister):
         return parser
 
     def take_action(self, parsed_args):
+        compute_client = self.app.client_manager.sdk_connection.compute
 
-        compute_client = self.app.client_manager.compute
+        server = compute_client.find_server(
-
-        server = utils.find_resource(
-            compute_client.servers,
             parsed_args.server,
+            ignore_missing=False,
         )
-
+        volumes = compute_client.volume_attachments(server)
-        volumes = compute_client.volumes.get_server_volumes(server.id)
 
         columns = ()
         column_headers = ()
 
-        if compute_client.api_version < api_versions.APIVersion('2.89'):
+        if not sdk_utils.supports_microversion(compute_client, '2.89'):
             columns += ('id',)
             column_headers += ('ID',)
 
         columns += (
             'device',
-            'serverId',
+            'server_id',
-            'volumeId',
+            'volume_id',
         )
         column_headers += (
             'Device',
@@ -62,25 +61,21 @@ class ListServerVolume(command.Lister):
             'Volume ID',
         )
 
-        if compute_client.api_version >= api_versions.APIVersion('2.70'):
+        if sdk_utils.supports_microversion(compute_client, '2.70'):
             columns += ('tag',)
             column_headers += ('Tag',)
 
-        if compute_client.api_version >= api_versions.APIVersion('2.79'):
+        if sdk_utils.supports_microversion(compute_client, '2.79'):
             columns += ('delete_on_termination',)
             column_headers += ('Delete On Termination?',)
 
-        if compute_client.api_version >= api_versions.APIVersion('2.89'):
+        if sdk_utils.supports_microversion(compute_client, '2.89'):
-            columns += ('attachment_id', 'bdm_uuid')
+            columns += ('attachment_id', 'bdm_id')
             column_headers += ('Attachment ID', 'BlockDeviceMapping UUID')
 
         return (
             column_headers,
-            (
+            (utils.get_item_properties(s, columns) for s in volumes),
-                utils.get_item_properties(
-                    s, columns, mixed_case_fields=('serverId', 'volumeId')
-                ) for s in volumes
-            ),
         )
 
 

openstackclient/tests/functional/compute/v2/test_server.py

@@ -1291,10 +1291,8 @@ class ServerTests(common.ComputeTestCase):
             parse_output=True,
         )
 
-        self.assertIsNotNone(cmd_output['ID'])
         self.assertEqual(server_id, cmd_output['Server ID'])
         self.assertEqual(volume_id, cmd_output['Volume ID'])
-        volume_attachment_id = cmd_output['ID']
 
         cmd_output = self.openstack(
             'server volume list ' +
@@ -1302,7 +1300,6 @@ class ServerTests(common.ComputeTestCase):
             parse_output=True,
         )
 
-        self.assertEqual(volume_attachment_id, cmd_output[0]['ID'])
         self.assertEqual(server_id, cmd_output[0]['Server ID'])
         self.assertEqual(volume_id, cmd_output[0]['Volume ID'])
 

openstackclient/tests/unit/compute/v2/fakes.py

@@ -1609,122 +1609,49 @@ class FakeServerMigration(object):
         return migration
 
 
-class FakeVolumeAttachment(object):
+def create_one_volume_attachment(attrs=None):
-    """Fake one or more volume attachments (BDMs)."""
+    """Create a fake volume attachment.
 
-    @staticmethod
+    :param dict attrs: A dictionary with all attributes
-    def create_one_volume_attachment(attrs=None, methods=None):
+    :return: A fake openstack.compute.v2.volume_attachment.VolumeAttachment
-        """Create a fake volume attachment.
+        object
+    """
+    attrs = attrs or {}
 
-        :param dict attrs:
+    # Set default attributes.
-            A dictionary with all attributes
+    volume_attachment_info = {
-        :param dict methods:
+        "id": uuid.uuid4().hex,
-            A dictionary with all methods
+        "device": "/dev/sdb",
-        :return:
+        "server_id": uuid.uuid4().hex,
-            A FakeResource object, with id, device, and so on
+        "volume_id": uuid.uuid4().hex,
-        """
+        # introduced in API microversion 2.70
-        attrs = attrs or {}
+        "tag": "foo",
-        methods = methods or {}
+        # introduced in API microversion 2.79
+        "delete_on_termination": True,
+        # introduced in API microversion 2.89
+        "attachment_id": uuid.uuid4().hex,
+        "bdm_id": uuid.uuid4().hex,
+    }
 
-        # Set default attributes.
+    # Overwrite default attributes.
-        volume_attachment_info = {
+    volume_attachment_info.update(attrs)
-            "id": uuid.uuid4().hex,
-            "device": "/dev/sdb",
-            "serverId": uuid.uuid4().hex,
-            "volumeId": uuid.uuid4().hex,
-            # introduced in API microversion 2.70
-            "tag": "foo",
-            # introduced in API microversion 2.79
-            "delete_on_termination": True,
-            # introduced in API microversion 2.89
-            "attachment_id": uuid.uuid4().hex,
-            "bdm_uuid": uuid.uuid4().hex
-        }
 
-        # Overwrite default attributes.
+    return volume_attachment.VolumeAttachment(**volume_attachment_info)
-        volume_attachment_info.update(attrs)
 
-        volume_attachment = fakes.FakeResource(
-            info=copy.deepcopy(volume_attachment_info),
-            methods=methods,
-            loaded=True)
-        return volume_attachment
 
-    @staticmethod
+def create_volume_attachments(attrs=None, count=2):
-    def create_volume_attachments(attrs=None, methods=None, count=2):
+    """Create multiple fake volume attachments.
-        """Create multiple fake volume attachments (BDMs).
 
-        :param dict attrs:
+    :param dict attrs: A dictionary with all attributes
-            A dictionary with all attributes
+    :param int count: The number of volume attachments to fake
-        :param dict methods:
+    :return: A list of fake
-            A dictionary with all methods
+        openstack.compute.v2.volume_attachment.VolumeAttachment objects
-        :param int count:
+    """
-            The number of volume attachments to fake
+    volume_attachments = []
-        :return:
+    for i in range(0, count):
-            A list of FakeResource objects faking the volume attachments.
+        volume_attachments.append(create_one_volume_attachment(attrs))
-        """
-        volume_attachments = []
-        for i in range(0, count):
-            volume_attachments.append(
-                FakeVolumeAttachment.create_one_volume_attachment(
-                    attrs, methods))
 
-        return volume_attachments
+    return volume_attachments
-
-    @staticmethod
-    def create_one_sdk_volume_attachment(attrs=None, methods=None):
-        """Create a fake sdk VolumeAttachment.
-
-        :param dict attrs:
-            A dictionary with all attributes
-        :param dict methods:
-            A dictionary with all methods
-        :return:
-            A fake VolumeAttachment object, with id, device, and so on
-        """
-        attrs = attrs or {}
-        methods = methods or {}
-
-        # Set default attributes.
-        volume_attachment_info = {
-            "id": uuid.uuid4().hex,
-            "device": "/dev/sdb",
-            "server_id": uuid.uuid4().hex,
-            "volume_id": uuid.uuid4().hex,
-            # introduced in API microversion 2.70
-            "tag": "foo",
-            # introduced in API microversion 2.79
-            "delete_on_termination": True,
-            # introduced in API microversion 2.89
-            "attachment_id": uuid.uuid4().hex,
-            "bdm_uuid": uuid.uuid4().hex
-        }
-
-        # Overwrite default attributes.
-        volume_attachment_info.update(attrs)
-
-        return volume_attachment.VolumeAttachment(**volume_attachment_info)
-
-    @staticmethod
-    def create_sdk_volume_attachments(attrs=None, methods=None, count=2):
-        """Create multiple fake VolumeAttachment objects (BDMs).
-
-        :param dict attrs:
-            A dictionary with all attributes
-        :param dict methods:
-            A dictionary with all methods
-        :param int count:
-            The number of volume attachments to fake
-        :return:
-            A list of VolumeAttachment objects faking the volume attachments.
-        """
-        volume_attachments = []
-        for i in range(0, count):
-            volume_attachments.append(
-                FakeVolumeAttachment.create_one_sdk_volume_attachment(
-                    attrs, methods))
-
-        return volume_attachments
 
 
 def create_one_hypervisor(attrs=None):

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

@@ -933,8 +933,7 @@ class TestServerVolume(TestServer):
             'volume_id': self.volumes[0].id,
         }
         self.volume_attachment = \
-            compute_fakes.FakeVolumeAttachment.\
+            compute_fakes.create_one_volume_attachment(attrs=attrs)
-            create_one_sdk_volume_attachment(attrs=attrs)
 
         self.sdk_client.create_volume_attachment.return_value = \
             self.volume_attachment

openstackclient/tests/unit/compute/v2/test_server_volume.py

@@ -11,7 +11,10 @@
 #   under the License.
 #
 
+from unittest import mock
+
 from novaclient import api_versions
+from openstack import utils as sdk_utils
 from osc_lib import exceptions
 
 from openstackclient.compute.v2 import server_volume
@@ -31,26 +34,31 @@ class TestServerVolume(compute_fakes.TestComputev2):
         self.servers_volumes_mock = self.app.client_manager.compute.volumes
         self.servers_volumes_mock.reset_mock()
 
+        self.app.client_manager.sdk_connection = mock.Mock()
+        self.app.client_manager.sdk_connection.compute = mock.Mock()
+        self.sdk_client = self.app.client_manager.sdk_connection.compute
+
 
 class TestServerVolumeList(TestServerVolume):
 
     def setUp(self):
         super().setUp()
 
-        self.server = compute_fakes.FakeServer.create_one_server()
+        self.server = compute_fakes.FakeServer.create_one_sdk_server()
-        self.volume_attachments = (
+        self.volume_attachments = compute_fakes.create_volume_attachments()
-            compute_fakes.FakeVolumeAttachment.create_volume_attachments())
 
-        self.servers_mock.get.return_value = self.server
+        self.sdk_client.find_server.return_value = self.server
-        self.servers_volumes_mock.get_server_volumes.return_value = (
+        self.sdk_client.volume_attachments.return_value = (
             self.volume_attachments)
 
         # Get the command object to test
         self.cmd = server_volume.ListServerVolume(self.app, None)
 
-    def test_server_volume_list(self):
+    @mock.patch.object(sdk_utils, 'supports_microversion')
+    def test_server_volume_list(self, sm_mock):
         self.app.client_manager.compute.api_version = \
             api_versions.APIVersion('2.1')
+        sm_mock.side_effect = [False, False, False, False]
 
         arglist = [
             self.server.id,
@@ -68,24 +76,23 @@ class TestServerVolumeList(TestServerVolume):
                 (
                     self.volume_attachments[0].id,
                     self.volume_attachments[0].device,
-                    self.volume_attachments[0].serverId,
+                    self.volume_attachments[0].server_id,
-                    self.volume_attachments[0].volumeId,
+                    self.volume_attachments[0].volume_id,
                 ),
                 (
                     self.volume_attachments[1].id,
                     self.volume_attachments[1].device,
-                    self.volume_attachments[1].serverId,
+                    self.volume_attachments[1].server_id,
-                    self.volume_attachments[1].volumeId,
+                    self.volume_attachments[1].volume_id,
                 ),
             ),
             tuple(data),
         )
-        self.servers_volumes_mock.get_server_volumes.assert_called_once_with(
+        self.sdk_client.volume_attachments.assert_called_once_with(self.server)
-            self.server.id)
 
-    def test_server_volume_list_with_tags(self):
+    @mock.patch.object(sdk_utils, 'supports_microversion')
-        self.app.client_manager.compute.api_version = \
+    def test_server_volume_list_with_tags(self, sm_mock):
-            api_versions.APIVersion('2.70')
+        sm_mock.side_effect = [False, True, False, False]
 
         arglist = [
             self.server.id,
@@ -105,27 +112,25 @@ class TestServerVolumeList(TestServerVolume):
                 (
                     self.volume_attachments[0].id,
                     self.volume_attachments[0].device,
-                    self.volume_attachments[0].serverId,
+                    self.volume_attachments[0].server_id,
-                    self.volume_attachments[0].volumeId,
+                    self.volume_attachments[0].volume_id,
                     self.volume_attachments[0].tag,
                 ),
                 (
                     self.volume_attachments[1].id,
                     self.volume_attachments[1].device,
-                    self.volume_attachments[1].serverId,
+                    self.volume_attachments[1].server_id,
-                    self.volume_attachments[1].volumeId,
+                    self.volume_attachments[1].volume_id,
                     self.volume_attachments[1].tag,
                 ),
             ),
             tuple(data),
         )
-        self.servers_volumes_mock.get_server_volumes.assert_called_once_with(
+        self.sdk_client.volume_attachments.assert_called_once_with(self.server)
-            self.server.id)
-
-    def test_server_volume_list_with_delete_on_attachment(self):
-        self.app.client_manager.compute.api_version = \
-            api_versions.APIVersion('2.79')
 
+    @mock.patch.object(sdk_utils, 'supports_microversion')
+    def test_server_volume_list_with_delete_on_attachment(self, sm_mock):
+        sm_mock.side_effect = [False, True, True, False]
         arglist = [
             self.server.id,
         ]
@@ -148,29 +153,28 @@ class TestServerVolumeList(TestServerVolume):
                 (
                     self.volume_attachments[0].id,
                     self.volume_attachments[0].device,
-                    self.volume_attachments[0].serverId,
+                    self.volume_attachments[0].server_id,
-                    self.volume_attachments[0].volumeId,
+                    self.volume_attachments[0].volume_id,
                     self.volume_attachments[0].tag,
                     self.volume_attachments[0].delete_on_termination,
                 ),
                 (
                     self.volume_attachments[1].id,
                     self.volume_attachments[1].device,
-                    self.volume_attachments[1].serverId,
+                    self.volume_attachments[1].server_id,
-                    self.volume_attachments[1].volumeId,
+                    self.volume_attachments[1].volume_id,
                     self.volume_attachments[1].tag,
                     self.volume_attachments[1].delete_on_termination,
                 ),
             ),
             tuple(data),
         )
-        self.servers_volumes_mock.get_server_volumes.assert_called_once_with(
+        self.sdk_client.volume_attachments.assert_called_once_with(self.server)
-            self.server.id)
 
-    def test_server_volume_list_with_attachment_ids(self):
+    @mock.patch.object(sdk_utils, 'supports_microversion')
-        self.app.client_manager.compute.api_version = \
+    def test_server_volume_list_with_attachment_ids(self, sm_mock):
-            api_versions.APIVersion('2.89')
 
+        sm_mock.side_effect = [True, True, True, True]
         arglist = [
             self.server.id,
         ]
@@ -193,28 +197,27 @@ class TestServerVolumeList(TestServerVolume):
             (
                 (
                     self.volume_attachments[0].device,
-                    self.volume_attachments[0].serverId,
+                    self.volume_attachments[0].server_id,
-                    self.volume_attachments[0].volumeId,
+                    self.volume_attachments[0].volume_id,
                     self.volume_attachments[0].tag,
                     self.volume_attachments[0].delete_on_termination,
                     self.volume_attachments[0].attachment_id,
-                    self.volume_attachments[0].bdm_uuid
+                    self.volume_attachments[0].bdm_id
 
                 ),
                 (
                     self.volume_attachments[1].device,
-                    self.volume_attachments[1].serverId,
+                    self.volume_attachments[1].server_id,
-                    self.volume_attachments[1].volumeId,
+                    self.volume_attachments[1].volume_id,
                     self.volume_attachments[1].tag,
                     self.volume_attachments[1].delete_on_termination,
                     self.volume_attachments[1].attachment_id,
-                    self.volume_attachments[1].bdm_uuid
+                    self.volume_attachments[1].bdm_id
                 ),
             ),
             tuple(data),
         )
-        self.servers_volumes_mock.get_server_volumes.assert_called_once_with(
+        self.sdk_client.volume_attachments.assert_called_once_with(self.server)
-            self.server.id)
 
 
 class TestServerVolumeUpdate(TestServerVolume):

releasenotes/notes/migrate-server-volume-list-update-to-sdk-95b1d3063e46f813.yaml

@@ -0,0 +1,3 @@
+features:
+  - |
+    Switch the server volume list command from novaclient to SDK.