From 09a76374c38e217f29520be5dd898993f2fba37f Mon Sep 17 00:00:00 2001 From: Cao ShuFeng Date: Fri, 22 Apr 2016 14:46:23 +0800 Subject: [PATCH] Fix useless api_version of Manager class Manager's api_version property is used by api_version.wraps. It should not be an "empty" class and the real api_version can be read from Client class. The 3.0 version's upload-to-image doesn't work as excepted because of this useless api_version of VolumeManager class. This patch also fix it and update some unit tests for it, because the changes are co-dependent on one another. Co-Authored-By: Nate Potter Change-Id: I398cbd02b61f30918a427291d1d3ae00435e0f4c Closes-Bug: #1573414 Closes-Bug: #1589040 --- cinderclient/base.py | 4 +- cinderclient/tests/unit/test_api_versions.py | 33 +++++++++++++++ cinderclient/tests/unit/test_utils.py | 12 ++++++ cinderclient/tests/unit/v3/test_shell.py | 16 +++++++- cinderclient/tests/unit/v3/test_volumes.py | 42 ++++++++++++++++++++ cinderclient/v3/shell.py | 27 ++++++++----- cinderclient/v3/volumes.py | 27 ++++++++++--- 7 files changed, 142 insertions(+), 19 deletions(-) create mode 100644 cinderclient/tests/unit/v3/test_volumes.py diff --git a/cinderclient/base.py b/cinderclient/base.py index 9cf33008d..4e92c534e 100644 --- a/cinderclient/base.py +++ b/cinderclient/base.py @@ -26,7 +26,6 @@ import os import six from six.moves.urllib import parse -from cinderclient import api_versions from cinderclient import exceptions from cinderclient.openstack.common.apiclient import base as common_base from cinderclient import utils @@ -63,12 +62,11 @@ class Manager(common_base.HookableMixin): resource_class = None def __init__(self, api): - self._api_version = api_versions.APIVersion() self.api = api @property def api_version(self): - return self._api_version + return self.api.api_version def _list(self, url, response_key, obj_class=None, body=None, limit=None, items=None): diff --git a/cinderclient/tests/unit/test_api_versions.py b/cinderclient/tests/unit/test_api_versions.py index f13e671fc..b28b2fd8b 100644 --- a/cinderclient/tests/unit/test_api_versions.py +++ b/cinderclient/tests/unit/test_api_versions.py @@ -18,7 +18,9 @@ import mock from cinderclient import api_versions from cinderclient import exceptions +from cinderclient.v3 import client from cinderclient.tests.unit import utils +from cinderclient.tests.unit import test_utils @ddt.ddt @@ -93,6 +95,37 @@ class APIVersionTestCase(utils.TestCase): api_versions.APIVersion().get_string) +class ManagerTest(utils.TestCase): + def test_api_version(self): + # The function manager.return_api_version has two versions, + # when called with api version 3.1 it should return the + # string '3.1' and when called with api version 3.2 or higher + # it should return the string '3.2'. + version = api_versions.APIVersion('3.1') + api = client.Client(api_version=version) + manager = test_utils.FakeManagerWithApi(api) + self.assertEqual('3.1', manager.return_api_version()) + + version = api_versions.APIVersion('3.2') + api = client.Client(api_version=version) + manager = test_utils.FakeManagerWithApi(api) + self.assertEqual('3.2', manager.return_api_version()) + + # pick up the highest version + version = api_versions.APIVersion('3.3') + api = client.Client(api_version=version) + manager = test_utils.FakeManagerWithApi(api) + self.assertEqual('3.2', manager.return_api_version()) + + version = api_versions.APIVersion('3.0') + api = client.Client(api_version=version) + manager = test_utils.FakeManagerWithApi(api) + # An exception will be returned here because the function + # return_api_version doesn't support version 3.0 + self.assertRaises(exceptions.VersionNotFoundForAPIMethod, + manager.return_api_version) + + class UpdateHeadersTestCase(utils.TestCase): def test_api_version_is_null(self): headers = {} diff --git a/cinderclient/tests/unit/test_utils.py b/cinderclient/tests/unit/test_utils.py index b44888c59..c7f9f0643 100644 --- a/cinderclient/tests/unit/test_utils.py +++ b/cinderclient/tests/unit/test_utils.py @@ -17,6 +17,7 @@ import sys import mock from six import moves +from cinderclient import api_versions from cinderclient import exceptions from cinderclient import utils from cinderclient import base @@ -61,6 +62,17 @@ class FakeManager(base.ManagerWithFind): return common_base.ListWithMeta(self.resources, fakes.REQUEST_ID) +class FakeManagerWithApi(base.Manager): + + @api_versions.wraps('3.1') + def return_api_version(self): + return '3.1' + + @api_versions.wraps('3.2') + def return_api_version(self): + return '3.2' + + class FakeDisplayResource(object): NAME_ATTR = 'display_name' diff --git a/cinderclient/tests/unit/v3/test_shell.py b/cinderclient/tests/unit/v3/test_shell.py index c56911dbf..0ee1e7cb8 100644 --- a/cinderclient/tests/unit/v3/test_shell.py +++ b/cinderclient/tests/unit/v3/test_shell.py @@ -71,13 +71,24 @@ class ShellTest(utils.TestCase): self.assert_called('GET', '/os-availability-zone') def test_upload_to_image(self): + expected = {'os-volume_upload_image': {'force': False, + 'container_format': 'bare', + 'disk_format': 'raw', + 'image_name': 'test-image'}} + self.run_command('upload-to-image 1234 test-image') + self.assert_called_anytime('GET', '/volumes/1234') + self.assert_called_anytime('POST', '/volumes/1234/action', + body=expected) + + def test_upload_to_image_private_not_protected(self): expected = {'os-volume_upload_image': {'force': False, 'container_format': 'bare', 'disk_format': 'raw', 'image_name': 'test-image', 'protected': False, 'visibility': 'private'}} - self.run_command('upload-to-image 1234 test-image') + self.run_command('--os-volume-api-version 3.1 ' + 'upload-to-image 1234 test-image') self.assert_called_anytime('GET', '/volumes/1234') self.assert_called_anytime('POST', '/volumes/1234/action', body=expected) @@ -89,7 +100,8 @@ class ShellTest(utils.TestCase): 'image_name': 'test-image', 'protected': 'True', 'visibility': 'public'}} - self.run_command('upload-to-image --visibility=public ' + self.run_command('--os-volume-api-version 3.1 ' + 'upload-to-image --visibility=public ' '--protected=True 1234 test-image') self.assert_called_anytime('GET', '/volumes/1234') self.assert_called_anytime('POST', '/volumes/1234/action', diff --git a/cinderclient/tests/unit/v3/test_volumes.py b/cinderclient/tests/unit/v3/test_volumes.py new file mode 100644 index 000000000..8f356992b --- /dev/null +++ b/cinderclient/tests/unit/v3/test_volumes.py @@ -0,0 +1,42 @@ +# Copyright 2016 FUJITSU LIMITED +# +# All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +from cinderclient import api_versions +from cinderclient.tests.unit import utils +from cinderclient.tests.unit.v3 import fakes +from cinderclient.v3.volumes import Volume +from cinderclient.v3.volumes import VolumeManager + + +class VolumesTest(utils.TestCase): + + def test_volume_manager_upload_to_image(self): + expected = {'os-volume_upload_image': + {'force': False, + 'container_format': 'bare', + 'disk_format': 'raw', + 'image_name': 'name', + 'visibility': 'public', + 'protected': True}} + api_version = api_versions.APIVersion('3.1') + cs = fakes.FakeClient(api_version) + manager = VolumeManager(cs) + fake_volume = Volume(manager, {'id': 1234, + 'name': 'sample-volume'}, + loaded=True) + fake_volume.upload_to_image(False, 'name', 'bare', 'raw', + visibility='public', protected=True) + cs.assert_called_anytime('POST', '/volumes/1234/action', body=expected) diff --git a/cinderclient/v3/shell.py b/cinderclient/v3/shell.py index e5a5ea148..05d8d1155 100644 --- a/cinderclient/v3/shell.py +++ b/cinderclient/v3/shell.py @@ -1282,22 +1282,31 @@ def _find_volume_type(cs, vtype): help=argparse.SUPPRESS) @utils.arg('--visibility', metavar='', - help='Makes image publicly accessible. Default=private.', - default='private') + help='Set image visibility to either public or private. ' + 'Default=private.', + default='private', + start_version='3.1') @utils.arg('--protected', metavar='', help='Prevents image from being deleted. Default=False.', - default=False) + default=False, + start_version='3.1') @utils.service_type('volumev3') def do_upload_to_image(cs, args): """Uploads volume to Image Service as an image.""" volume = utils.find_volume(cs, args.volume) - _print_volume_image(volume.upload_to_image(args.force, - args.image_name, - args.container_format, - args.disk_format, - args.visibility, - args.protected)) + if cs.api_version >= api_versions.APIVersion("3.1"): + _print_volume_image(volume.upload_to_image(args.force, + args.image_name, + args.container_format, + args.disk_format, + args.visibility, + args.protected)) + else: + _print_volume_image(volume.upload_to_image(args.force, + args.image_name, + args.container_format, + args.disk_format)) @utils.arg('volume', metavar='', help='ID of volume to migrate.') diff --git a/cinderclient/v3/volumes.py b/cinderclient/v3/volumes.py index c7654d635..9414c009c 100644 --- a/cinderclient/v3/volumes.py +++ b/cinderclient/v3/volumes.py @@ -111,11 +111,28 @@ class Volume(base.Resource): return self.manager.show_image_metadata(self) def upload_to_image(self, force, image_name, container_format, - disk_format, visibility, protected): - """Upload a volume to image service as an image.""" - return self.manager.upload_to_image(self, force, image_name, - container_format, disk_format, - visibility, protected) + disk_format, visibility=None, + protected=None): + """Upload a volume to image service as an image. + :param force: Boolean to enables or disables upload of a volume that + is attached to an instance. + :param image_name: The new image name. + :param container_format: Container format type. + :param disk_format: Disk format type. + :param visibility: The accessibility of image (allowed for + 3.1-latest). + :param protected: Boolean to decide whether prevents image from being + deleted (allowed for 3.1-latest). + """ + if self.manager.api_version >= api_versions.APIVersion("3.1"): + visibility = 'private' if visibility is None else visibility + protected = False if protected is None else protected + return self.manager.upload_to_image(self, force, image_name, + container_format, disk_format, + visibility, protected) + else: + return self.manager.upload_to_image(self, force, image_name, + container_format, disk_format) def force_delete(self): """Delete the specified volume ignoring its current state.