Merge "Add support for location parameters in v2 commands"

.mailmap

@@ -1,5 +1,4 @@
-# Format is:
+# "man git-shortlog" for reference
-# <preferred e-mail> <other e-mail 1>
-# <preferred e-mail> <other e-mail 2>
 <mr.alex.meade@gmail.com> <hatboy112@yahoo.com>
 <mr.alex.meade@gmail.com> <alex.meade@rackspace.com>
+David Koo <david.koo@huawei.com> <kpublicmail@gmail.com>

glanceclient/common/utils.py

@@ -325,3 +325,13 @@ def strip_version(endpoint):
     if re.match('v\d+\.?\d*', url_bits[-1]):
         endpoint = '/'.join(url_bits[:-1])
     return endpoint
+
+
+def print_image(image_obj, max_col_width=None):
+    ignore = ['self', 'access', 'file', 'schema']
+    image = dict([item for item in six.iteritems(image_obj)
+                  if item[0] not in ignore])
+    if str(max_col_width).isdigit():
+        print_dict(image, max_column_width=max_col_width)
+    else:
+        print_dict(image)

glanceclient/v2/images.py

@@ -13,12 +13,14 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import json
 import six
 from six.moves.urllib import parse
 
 import warlock
 
 from glanceclient.common import utils
+from glanceclient import exc
 from glanceclient.openstack.common import strutils
 
 DEFAULT_PAGE_SIZE = 20
@@ -170,3 +172,94 @@ class Controller(object):
         # we need to fetch the image again to get a clean history. This is
         # an obvious optimization for warlock
         return self.get(image_id)
+
+    def _get_image_with_locations_or_fail(self, image_id):
+        image = self.get(image_id)
+        if getattr(image, 'locations', None) is None:
+            raise exc.HTTPBadRequest('The administrator has disabled '
+                                     'API access to image locations')
+        return image
+
+    def _send_image_update_request(self, image_id, patch_body):
+        url = '/v2/images/%s' % image_id
+        hdrs = {'Content-Type': 'application/openstack-images-v2.1-json-patch'}
+        self.http_client.raw_request('PATCH', url,
+                                     headers=hdrs,
+                                     body=json.dumps(patch_body))
+
+    def add_location(self, image_id, url, metadata):
+        """Add a new location entry to an image's list of locations.
+
+        It is an error to add a URL that is already present in the list of
+        locations.
+
+        :param image_id: ID of image to which the location is to be added.
+        :param url: URL of the location to add.
+        :param metadata: Metadata associated with the location.
+        :returns: The updated image
+        """
+        image = self._get_image_with_locations_or_fail(image_id)
+        url_list = [l['url'] for l in image.locations]
+        if url in url_list:
+            err_str = 'A location entry at %s already exists' % url
+            raise exc.HTTPConflict(err_str)
+
+        add_patch = [{'op': 'add', 'path': '/locations/-',
+                      'value': {'url': url, 'metadata': metadata}}]
+        self._send_image_update_request(image_id, add_patch)
+        return self.get(image_id)
+
+    def delete_locations(self, image_id, url_set):
+        """Remove one or more location entries of an image.
+
+        :param image_id: ID of image from which locations are to be removed.
+        :param url_set: set of URLs of location entries to remove.
+        :returns: None
+        """
+        image = self._get_image_with_locations_or_fail(image_id)
+        current_urls = [l['url'] for l in image.locations]
+
+        missing_locs = url_set.difference(set(current_urls))
+        if missing_locs:
+            raise exc.HTTPNotFound('Unknown URL(s): %s' % list(missing_locs))
+
+        # NOTE: warlock doesn't generate the most efficient patch for remove
+        # operations (it shifts everything up and deletes the tail elements) so
+        # we do it ourselves.
+        url_indices = [current_urls.index(url) for url in url_set]
+        url_indices.sort(reverse=True)
+        patches = [{'op': 'remove', 'path': '/locations/%s' % url_idx}
+                   for url_idx in url_indices]
+        self._send_image_update_request(image_id, patches)
+
+    def update_location(self, image_id, url, metadata):
+        """Update an existing location entry in an image's list of locations.
+
+        The URL specified must be already present in the image's list of
+        locations.
+
+        :param image_id: ID of image whose location is to be updated.
+        :param url: URL of the location to update.
+        :param metadata: Metadata associated with the location.
+        :returns: The updated image
+        """
+        image = self._get_image_with_locations_or_fail(image_id)
+        url_map = dict([(l['url'], l) for l in image.locations])
+        if url not in url_map:
+            raise exc.HTTPNotFound('Unknown URL: %s' % url)
+
+        if url_map[url]['metadata'] == metadata:
+            return image
+
+        # NOTE: The server (as of now) doesn't support modifying individual
+        # location entries. So we must:
+        #   1. Empty existing list of locations.
+        #   2. Send another request to set 'locations' to the new list
+        #      of locations.
+        url_map[url]['metadata'] = metadata
+        patches = [{'op': 'replace',
+                    'path': '/locations',
+                    'value': p} for p in ([], list(url_map.values()))]
+        self._send_image_update_request(image_id, patches)
+
+        return self.get(image_id)

glanceclient/v2/shell.py

@@ -19,7 +19,6 @@ from glanceclient import exc
 import json
 import os
 from os.path import expanduser
-import six
 
 IMAGE_SCHEMA = None
 
@@ -54,14 +53,11 @@ def do_image_create(gc, args):
         fields[key] = value
 
     image = gc.images.create(**fields)
-    ignore = ['self', 'access', 'file', 'schema']
+    utils.print_image(image)
-    image = dict([item for item in six.iteritems(image)
-                  if item[0] not in ignore])
-    utils.print_dict(image)
 
 
 @utils.arg('id', metavar='<IMAGE_ID>', help='ID of image to update.')
-@utils.schema_args(get_image_schema, omit=['id'])
+@utils.schema_args(get_image_schema, omit=['id', 'locations'])
 @utils.arg('--property', metavar="<key=value>", action='append',
            default=[], help=('Arbitrary property to associate with image.'
                              ' May be used multiple times.'))
@@ -85,10 +81,7 @@ def do_image_update(gc, args):
 
     image_id = fields.pop('id')
     image = gc.images.update(image_id, remove_properties, **fields)
-    ignore = ['self', 'access', 'file', 'schema']
+    utils.print_image(image)
-    image = dict([item for item in six.iteritems(image)
-                  if item[0] not in ignore])
-    utils.print_dict(image)
 
 
 @utils.arg('--page-size', metavar='<SIZE>', default=None, type=int,
@@ -125,9 +118,7 @@ def do_image_show(gc, args):
     """Describe a specific image."""
     image = gc.images.get(args.id)
     ignore = ['self', 'access', 'file', 'schema']
-    image = dict([item for item in six.iteritems(image) if item[0] not in
+    utils.print_image(image, int(args.max_column_width))
-                  ignore])
-    utils.print_dict(image, max_column_width=int(args.max_column_width))
 
 
 @utils.arg('--image-id', metavar='<IMAGE_ID>', required=True,
@@ -268,3 +259,48 @@ def do_image_tag_delete(gc, args):
         utils.exit('Unable to delete tag. Specify image_id and tag_value')
     else:
         gc.image_tags.delete(args.image_id, args.tag_value)
+
+
+@utils.arg('--url', metavar='<URL>', required=True,
+           help='URL of location to add.')
+@utils.arg('--metadata', metavar='<STRING>', default='{}',
+           help=('Metadata associated with the location. '
+                 'Must be a valid JSON object (default: %(default)s)'))
+@utils.arg('id', metavar='<ID>',
+           help='ID of image to which the location is to be added.')
+def do_location_add(gc, args):
+    """Add a location (and related metadata) to an image."""
+    try:
+        metadata = json.loads(args.metadata)
+    except ValueError:
+        utils.exit('Metadata is not a valid JSON object.')
+    else:
+        image = gc.images.add_location(args.id, args.url, metadata)
+        utils.print_dict(image)
+
+
+@utils.arg('--url', metavar='<URL>', action='append', required=True,
+           help='URL of location to remove. May be used multiple times.')
+@utils.arg('id', metavar='<ID>',
+           help='ID of image whose locations are to be removed.')
+def do_location_delete(gc, args):
+    """Remove locations (and related metadata) from an image."""
+    image = gc.images.delete_locations(args.id, set(args.url))
+
+
+@utils.arg('--url', metavar='<URL>', required=True,
+           help='URL of location to update.')
+@utils.arg('--metadata', metavar='<STRING>', default='{}',
+           help=('Metadata associated with the location. '
+                 'Must be a valid JSON object (default: %(default)s)'))
+@utils.arg('id', metavar='<ID>',
+           help='ID of image whose location is to be updated.')
+def do_location_update(gc, args):
+    """Update metadata of an image's location."""
+    try:
+        metadata = json.loads(args.metadata)
+    except ValueError:
+        utils.exit('Metadata is not a valid JSON object.')
+    else:
+        image = gc.images.update_location(args.id, args.url, metadata)
+        utils.print_dict(image)

tests/v2/test_images.py

@@ -14,11 +14,13 @@
 #    under the License.
 
 import errno
+import json
 import testtools
 
 import six
 import warlock
 
+from glanceclient import exc
 from glanceclient.v2 import images
 from tests import utils
 
@@ -303,12 +305,43 @@ fixtures = {
             {'images': []},
         ),
     },
+    '/v2/images/a2b83adc-888e-11e3-8872-78acc0b951d8': {
+        'GET': (
+            {},
+            {
+                'id': 'a2b83adc-888e-11e3-8872-78acc0b951d8',
+                'name': 'image-location-tests',
+                'locations': [{u'url': u'http://foo.com/',
+                               u'metadata': {u'foo': u'foometa'}},
+                              {u'url': u'http://bar.com/',
+                               u'metadata': {u'bar': u'barmeta'}}],
+            },
+        ),
+        'PATCH': (
+            {},
+            '',
+        )
+    },
 }
 
 
 fake_schema = {
     'name': 'image',
-    'properties': {'id': {}, 'name': {}},
+    'properties': {
+        'id': {},
+        'name': {},
+        'locations': {
+            'type': 'array',
+            'items': {
+                'type': 'object',
+                'properties': {
+                    'metadata': {'type': 'object'},
+                    'url': {'type': 'string'},
+                },
+                'required': ['url', 'metadata'],
+            },
+        },
+    },
     'additionalProperties': {'type': 'string'}
 }
 FakeModel = warlock.model_factory(fake_schema)
@@ -626,3 +659,105 @@ class TestController(testtools.TestCase):
         params = {'name': 'pong', 'bad_prop': False}
         with testtools.ExpectedException(TypeError):
             self.controller.update(image_id, **params)
+
+    def test_location_ops_when_server_disabled_location_ops(self):
+        # Location operations should not be allowed if server has not
+        # enabled location related operations
+        image_id = '3a4560a1-e585-443e-9b39-553b46ec92d1'
+        estr = 'The administrator has disabled API access to image locations'
+        url = 'http://bar.com/'
+        meta = {'bar': 'barmeta'}
+
+        e = self.assertRaises(exc.HTTPBadRequest,
+                              self.controller.add_location,
+                              image_id, url, meta)
+        self.assertTrue(estr in str(e))
+
+        e = self.assertRaises(exc.HTTPBadRequest,
+                              self.controller.delete_locations,
+                              image_id, set([url]))
+        self.assertTrue(estr in str(e))
+
+        e = self.assertRaises(exc.HTTPBadRequest,
+                              self.controller.update_location,
+                              image_id, url, meta)
+        self.assertTrue(estr in str(e))
+
+    def _empty_get(self, image_id):
+        return ('GET', '/v2/images/%s' % image_id, {}, None)
+
+    def _patch_req(self, image_id, patch_body):
+        c_type = 'application/openstack-images-v2.1-json-patch'
+        return ('PATCH',
+                '/v2/images/%s' % image_id,
+                {'Content-Type': c_type},
+                json.dumps(patch_body))
+
+    def test_add_location(self):
+        image_id = 'a2b83adc-888e-11e3-8872-78acc0b951d8'
+        new_loc = {'url': 'http://spam.com/', 'metadata': {'spam': 'ham'}}
+        add_patch = {'path': '/locations/-', 'value': new_loc, 'op': 'add'}
+        image = self.controller.add_location(image_id, **new_loc)
+        self.assertEqual(self.api.calls, [
+            self._empty_get(image_id),
+            self._patch_req(image_id, [add_patch]),
+            self._empty_get(image_id)
+        ])
+
+    def test_add_duplicate_location(self):
+        image_id = 'a2b83adc-888e-11e3-8872-78acc0b951d8'
+        new_loc = {'url': 'http://foo.com/', 'metadata': {'foo': 'newfoo'}}
+        err_str = 'A location entry at %s already exists' % new_loc['url']
+
+        err = self.assertRaises(exc.HTTPConflict,
+                                self.controller.add_location,
+                                image_id, **new_loc)
+        self.assertIn(err_str, str(err))
+
+    def test_remove_location(self):
+        image_id = 'a2b83adc-888e-11e3-8872-78acc0b951d8'
+        url_set = set(['http://foo.com/', 'http://bar.com/'])
+        del_patches = [{'path': '/locations/1', 'op': 'remove'},
+                       {'path': '/locations/0', 'op': 'remove'}]
+        image = self.controller.delete_locations(image_id, url_set)
+        self.assertEqual(self.api.calls, [
+            self._empty_get(image_id),
+            self._patch_req(image_id, del_patches)
+        ])
+
+    def test_remove_missing_location(self):
+        image_id = 'a2b83adc-888e-11e3-8872-78acc0b951d8'
+        url_set = set(['http://spam.ham/'])
+        err_str = 'Unknown URL(s): %s' % list(url_set)
+
+        err = self.assertRaises(exc.HTTPNotFound,
+                                self.controller.delete_locations,
+                                image_id, url_set)
+        self.assertTrue(err_str in str(err))
+
+    def test_update_location(self):
+        image_id = 'a2b83adc-888e-11e3-8872-78acc0b951d8'
+        new_loc = {'url': 'http://foo.com/', 'metadata': {'spam': 'ham'}}
+        fixture_idx = '/v2/images/%s' % (image_id)
+        orig_locations = fixtures[fixture_idx]['GET'][1]['locations']
+        loc_map = dict([(l['url'], l) for l in orig_locations])
+        loc_map[new_loc['url']] = new_loc
+        mod_patch = [{'path': '/locations', 'op': 'replace',
+                      'value': []},
+                     {'path': '/locations', 'op': 'replace',
+                      'value': list(loc_map.values())}]
+        image = self.controller.update_location(image_id, **new_loc)
+        self.assertEqual(self.api.calls, [
+            self._empty_get(image_id),
+            self._patch_req(image_id, mod_patch),
+            self._empty_get(image_id)
+        ])
+
+    def test_update_missing_location(self):
+        image_id = 'a2b83adc-888e-11e3-8872-78acc0b951d8'
+        new_loc = {'url': 'http://spam.com/', 'metadata': {'spam': 'ham'}}
+        err_str = 'Unknown URL: %s' % new_loc['url']
+        err = self.assertRaises(exc.HTTPNotFound,
+                                self.controller.update_location,
+                                image_id, **new_loc)
+        self.assertTrue(err_str in str(err))

tests/v2/test_shell_v2.py

@@ -14,6 +14,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import json
 import mock
 import six
 import testtools
@@ -53,7 +54,7 @@ class ShellV2Test(testtools.TestCase):
         utils.print_dict = mock.Mock()
         utils.save_image = mock.Mock()
 
-    def _test_with_few_arguments(self, func, func_args, err_msg):
+    def assert_exits_with_msg(self, func, func_args, err_msg):
         with mock.patch.object(utils, 'exit') as mocked_utils_exit:
             mocked_utils_exit.return_value = '%s' % err_msg
 
@@ -207,6 +208,59 @@ class ShellV2Test(testtools.TestCase):
             utils.print_dict.assert_called_once_with({
                 'id': 'pass', 'name': 'IMG-01', 'disk_format': 'vhd'})
 
+    def test_do_location_add_update_with_invalid_json_metadata(self):
+        args = self._make_args({'id': 'pass',
+                                'url': 'http://foo/bar',
+                                'metadata': '{1, 2, 3}'})
+        self.assert_exits_with_msg(test_shell.do_location_add,
+                                   args,
+                                   'Metadata is not a valid JSON object.')
+        self.assert_exits_with_msg(test_shell.do_location_update,
+                                   args,
+                                   'Metadata is not a valid JSON object.')
+
+    def test_do_location_add(self):
+        gc = self.gc
+        loc = {'url': 'http://foo.com/', 'metadata': {'foo': 'bar'}}
+        args = self._make_args({'id': 'pass',
+                                'url': loc['url'],
+                                'metadata': json.dumps(loc['metadata'])})
+        with mock.patch.object(gc.images, 'add_location') as mocked_addloc:
+            expect_image = {'id': 'pass', 'locations': [loc]}
+            mocked_addloc.return_value = expect_image
+
+            test_shell.do_location_add(self.gc, args)
+            mocked_addloc.assert_called_once_with('pass',
+                                                  loc['url'],
+                                                  loc['metadata'])
+            utils.print_dict.assert_called_once_with(expect_image)
+
+    def test_do_location_delete(self):
+        gc = self.gc
+        loc_set = set(['http://foo/bar', 'http://spam/ham'])
+        args = self._make_args({'id': 'pass', 'url': loc_set})
+
+        with mock.patch.object(gc.images, 'delete_locations') as mocked_rmloc:
+            expect_image = {'id': 'pass', 'locations': []}
+            test_shell.do_location_delete(self.gc, args)
+            mocked_rmloc.assert_called_once_with('pass', loc_set)
+
+    def test_do_location_update(self):
+        gc = self.gc
+        loc = {'url': 'http://foo.com/', 'metadata': {'foo': 'bar'}}
+        args = self._make_args({'id': 'pass',
+                                'url': loc['url'],
+                                'metadata': json.dumps(loc['metadata'])})
+        with mock.patch.object(gc.images, 'update_location') as mocked_modloc:
+            expect_image = {'id': 'pass', 'locations': [loc]}
+            mocked_modloc.return_value = expect_image
+
+            test_shell.do_location_update(self.gc, args)
+            mocked_modloc.assert_called_once_with('pass',
+                                                  loc['url'],
+                                                  loc['metadata'])
+            utils.print_dict.assert_called_once_with(expect_image)
+
     def test_do_explain(self):
         input = {
             'page_size': 18,
@@ -305,9 +359,9 @@ class ShellV2Test(testtools.TestCase):
         args = self._make_args({'image_id': None, 'member_id': 'MEM-01'})
         msg = 'Unable to create member. Specify image_id and member_id'
 
-        self._test_with_few_arguments(func=test_shell.do_member_create,
+        self.assert_exits_with_msg(func=test_shell.do_member_create,
-                                      func_args=args,
+                                   func_args=args,
-                                      err_msg=msg)
+                                   err_msg=msg)
 
     def test_do_member_update(self):
         input = {
@@ -335,9 +389,9 @@ class ShellV2Test(testtools.TestCase):
         msg = 'Unable to update member. Specify image_id, member_id' \
               ' and member_status'
 
-        self._test_with_few_arguments(func=test_shell.do_member_update,
+        self.assert_exits_with_msg(func=test_shell.do_member_update,
-                                      func_args=args,
+                                   func_args=args,
-                                      err_msg=msg)
+                                   err_msg=msg)
 
     def test_do_member_delete(self):
         args = self._make_args({'image_id': 'IMG-01', 'member_id': 'MEM-01'})
@@ -350,9 +404,9 @@ class ShellV2Test(testtools.TestCase):
         args = self._make_args({'image_id': None, 'member_id': 'MEM-01'})
         msg = 'Unable to delete member. Specify image_id and member_id'
 
-        self._test_with_few_arguments(func=test_shell.do_member_delete,
+        self.assert_exits_with_msg(func=test_shell.do_member_delete,
-                                      func_args=args,
+                                   func_args=args,
-                                      err_msg=msg)
+                                   err_msg=msg)
 
     def test_image_tag_update(self):
         args = self._make_args({'image_id': 'IMG-01', 'tag_value': 'tag01'})
@@ -368,9 +422,9 @@ class ShellV2Test(testtools.TestCase):
         args = self._make_args({'image_id': None, 'tag_value': 'tag01'})
         msg = 'Unable to update tag. Specify image_id and tag_value'
 
-        self._test_with_few_arguments(func=test_shell.do_image_tag_update,
+        self.assert_exits_with_msg(func=test_shell.do_image_tag_update,
-                                      func_args=args,
+                                   func_args=args,
-                                      err_msg=msg)
+                                   err_msg=msg)
 
     def test_image_tag_delete(self):
         args = self._make_args({'image_id': 'IMG-01', 'tag_value': 'tag01'})
@@ -385,6 +439,6 @@ class ShellV2Test(testtools.TestCase):
         args = self._make_args({'image_id': 'IMG-01', 'tag_value': None})
         msg = 'Unable to delete tag. Specify image_id and tag_value'
 
-        self._test_with_few_arguments(func=test_shell.do_image_tag_delete,
+        self.assert_exits_with_msg(func=test_shell.do_image_tag_delete,
-                                      func_args=args,
+                                   func_args=args,
-                                      err_msg=msg)
+                                   err_msg=msg)