From baa23254ec4cedf21a03c7417553edddbba5cacd Mon Sep 17 00:00:00 2001 From: zhangdaolong Date: Sun, 16 Jul 2017 02:09:00 +0800 Subject: [PATCH] Fix the inappropriate parameter name The argument to the function should be the volume ID, not the attachment ID.So the argument name of the function here is easy to misunderstand. Change-Id: Ie093b6acebe8b9e8dbdfed89856666db25bcf756 --- novaclient/tests/unit/v2/test_volumes.py | 38 +++++++++++++++++++ novaclient/v2/volumes.py | 47 ++++++++++++++++++++---- 2 files changed, 77 insertions(+), 8 deletions(-) diff --git a/novaclient/tests/unit/v2/test_volumes.py b/novaclient/tests/unit/v2/test_volumes.py index 60178cb02..932b71d79 100644 --- a/novaclient/tests/unit/v2/test_volumes.py +++ b/novaclient/tests/unit/v2/test_volumes.py @@ -13,6 +13,8 @@ # License for the specific language governing permissions and limitations # under the License. +import mock + from novaclient import api_versions from novaclient.tests.unit import utils from novaclient.tests.unit.v2 import fakes @@ -55,6 +57,24 @@ class VolumesTest(utils.TestCase): '/servers/1234/os-volume_attachments/Work') self.assertIsInstance(v, volumes.Volume) + def test_get_server_volume_with_exception(self): + self.assertRaises(TypeError, + self.cs.volumes.get_server_volume, + "1234") + + self.assertRaises(TypeError, + self.cs.volumes.get_server_volume, + "1234", + volume_id="Work", + attachment_id="123") + + @mock.patch('warnings.warn') + def test_get_server_volume_with_warn(self, mock_warn): + self.cs.volumes.get_server_volume(1234, + volume_id=None, + attachment_id="Work") + mock_warn.assert_called_once() + def test_list_server_volumes(self): vl = self.cs.volumes.get_server_volumes(1234) self.assert_request_id(vl, fakes.FAKE_REQUEST_ID_LIST) @@ -88,3 +108,21 @@ class VolumesV249Test(VolumesTest): 'device': '/dev/vdb', 'tag': 'test_tag'}}) self.assertIsInstance(v, volumes.Volume) + + def test_delete_server_volume_with_exception(self): + self.assertRaises(TypeError, + self.cs.volumes.delete_server_volume, + "1234") + + self.assertRaises(TypeError, + self.cs.volumes.delete_server_volume, + "1234", + volume_id="Work", + attachment_id="123") + + @mock.patch('warnings.warn') + def test_delete_server_volume_with_warn(self, mock_warn): + self.cs.volumes.delete_server_volume(1234, + volume_id=None, + attachment_id="Work") + mock_warn.assert_called_once() diff --git a/novaclient/v2/volumes.py b/novaclient/v2/volumes.py index ebccf8a09..d6208cbd1 100644 --- a/novaclient/v2/volumes.py +++ b/novaclient/v2/volumes.py @@ -16,6 +16,7 @@ """ Volume interface """ +import warnings from novaclient import api_versions from novaclient import base @@ -95,17 +96,32 @@ class VolumeManager(base.Manager): (server_id, src_volid,), body, "volumeAttachment") - def get_server_volume(self, server_id, attachment_id): + def get_server_volume(self, server_id, volume_id=None, attachment_id=None): """ - Get the volume identified by the attachment ID, that is attached to + Get the volume identified by the volume ID, that is attached to the given server ID :param server_id: The ID of the server - :param attachment_id: The ID of the attachment + :param volume_id: The ID of the volume to attach :rtype: :class:`Volume` """ + + if attachment_id is not None and volume_id is not None: + raise TypeError("You cannot specify both volume_id " + "and attachment_id arguments.") + + elif attachment_id is not None: + warnings.warn("attachment_id argument " + "of volumes.get_server_volume " + "method is deprecated in favor " + "of volume_id.") + volume_id = attachment_id + + if volume_id is None: + raise TypeError("volume_id is required argument.") + return self._get("/servers/%s/os-volume_attachments/%s" % (server_id, - attachment_id,), "volumeAttachment") + volume_id,), "volumeAttachment") def get_server_volumes(self, server_id): """ @@ -117,13 +133,28 @@ class VolumeManager(base.Manager): return self._list("/servers/%s/os-volume_attachments" % server_id, "volumeAttachments") - def delete_server_volume(self, server_id, attachment_id): + def delete_server_volume(self, server_id, volume_id=None, + attachment_id=None): """ - Detach a volume identified by the attachment ID from the given server + Detach a volume identified by the volume ID from the given server :param server_id: The ID of the server - :param attachment_id: The ID of the attachment + :param volume_id: The ID of the volume to attach :returns: An instance of novaclient.base.TupleWithMeta """ + if attachment_id is not None and volume_id is not None: + raise TypeError("You cannot specify both volume_id " + "and attachment_id arguments.") + + elif attachment_id is not None: + warnings.warn("attachment_id argument " + "of volumes.delete_server_volume " + "method is deprecated in favor " + "of volume_id.") + volume_id = attachment_id + + if volume_id is None: + raise TypeError("volume_id is required argument.") + return self._delete("/servers/%s/os-volume_attachments/%s" % - (server_id, attachment_id,)) + (server_id, volume_id,))