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 <nathaniel.potter@intel.com>

Change-Id: I398cbd02b61f30918a427291d1d3ae00435e0f4c
Closes-Bug: #1573414
Closes-Bug: #1589040

7 files changed, 142 insertions, 19 deletions

diff --git a/cinderclient/base.py b/cinderclient/base.py
index 9cf3300..4e92c53 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 f13e671..b28b2fd 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 b44888c..c7f9f06 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 c56911d..0ee1e7c 100644
--- a/cinderclient/tests/unit/v3/test_shell.py
+++ b/cinderclient/tests/unit/v3/test_shell.py
@@ -74,10 +74,21 @@ class ShellTest(utils.TestCase):
         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 0000000..8f35699
--- /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 e5a5ea1..05d8d11 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='<public|private>',
-           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='<True|False>',
            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='<volume>', help='ID of volume to migrate.')
diff --git a/cinderclient/v3/volumes.py b/cinderclient/v3/volumes.py
index c7654d6..9414c00 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.