[Kimchi-devel] [PATCH] storagevolume: Use default value for param 'name' when appropriate

Paulo Ricardo Paz Vital pvital at linux.vnet.ibm.com
Tue Sep 9 17:31:27 UTC 2014


On Mon, 2014-09-08 at 16:22 -0300, Crístian Viana wrote:
> The parameter 'name' is only required when creating storage volumes with
> the parameter 'capacity' (i.e. an empty volume). When creating volumes
> with 'url' or 'file', the volume name will have a default value if 'name'
> is not specified.
> 
> Use a default name when creating storage volumes from a file or from a URL.
> The default name will be the file's or the URL's basename.
> 
> Signed-off-by: Crístian Viana <vianac at linux.vnet.ibm.com>
> ---
>  src/kimchi/API.json                |  2 --
>  src/kimchi/mockmodel.py            | 19 ++++++++++++++-----
>  src/kimchi/model/storagevolumes.py | 26 +++++++++++++++++++++-----
>  tests/test_model.py                | 17 +++++++++++------
>  tests/test_rest.py                 | 16 ++++++++++++----
>  5 files changed, 58 insertions(+), 22 deletions(-)
> 
> diff --git a/src/kimchi/API.json b/src/kimchi/API.json
> index 8a95804..1319531 100644
> --- a/src/kimchi/API.json
> +++ b/src/kimchi/API.json
> @@ -156,13 +156,11 @@
>          },
>          "storagevolumes_create": {
>              "type": "object",
> -            "error": "KCHVOL0016E",

Why removing the object error?

>              "properties": {
>                  "name": {
>                      "description": "The name of the Storage Volume",
>                      "type": "string",
>                      "minLength": 1,
> -                    "required": true,
>                      "error": "KCHVOL0013E"
>                  },
>                  "capacity": {
> diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py
> index 15a75af..ef09a5b 100644
> --- a/src/kimchi/mockmodel.py
> +++ b/src/kimchi/mockmodel.py
> @@ -482,8 +482,9 @@ class MockModel(object):
>  
>      def storagevolumes_create(self, pool_name, params):
>          vol_source = ['file', 'url', 'capacity']
> +        require_name_params = ['capacity']
>  
> -        name = params['name']
> +        name = params.get('name')
>  
>          index_list = list(i for i in range(len(vol_source))
>                            if vol_source[i] in params)
> @@ -491,12 +492,20 @@ class MockModel(object):
>              raise InvalidParameter("KCHVOL0018E",
>                                     {'param': ",".join(vol_source)})
>  
> +        create_param = vol_source[index_list[0]]
> +
> +        if name is None:
> +            if create_param in require_name_params:
> +                raise InvalidParameter('KCHVOL0016E')
> +
> +            name = os.path.basename(create_param)
> +            params['name'] = name
> +
>          try:
> -            create_func = getattr(self, "_create_volume_with_" +
> -                                        vol_source[index_list[0]])
> +            create_func = getattr(self, '_create_volume_with_%s' %
> +                                        create_param)
>          except AttributeError:
> -            raise InvalidParameter("KCHVOL0019E",
> -                                   {'param': vol_source[index_list[0]]})
> +            raise InvalidParameter("KCHVOL0019E", {'param': create_param})
>  
>          pool = self._get_storagepool(pool_name)
>          if pool.info['type'] in READONLY_POOL_TYPE:
> diff --git a/src/kimchi/model/storagevolumes.py b/src/kimchi/model/storagevolumes.py
> index ef8e684..ec610ac 100644
> --- a/src/kimchi/model/storagevolumes.py
> +++ b/src/kimchi/model/storagevolumes.py
> @@ -44,6 +44,9 @@ VOLUME_TYPE_MAP = {0: 'file',
>  DOWNLOAD_CHUNK_SIZE = 1048576  # 1 MiB
>  
> 
> +REQUIRE_NAME_PARAMS = ['capacity']
> +
> +
>  class StorageVolumesModel(object):
>      def __init__(self, **kargs):
>          self.conn = kargs['conn']
> @@ -53,7 +56,7 @@ class StorageVolumesModel(object):
>      def create(self, pool_name, params):
>          vol_source = ['file', 'url', 'capacity']
>  
> -        name = params['name']
> +        name = params.get('name')
>  
>          index_list = list(i for i in range(len(vol_source))
>                            if vol_source[i] in params)
> @@ -61,12 +64,25 @@ class StorageVolumesModel(object):
>              raise InvalidParameter("KCHVOL0018E",
>                                     {'param': ",".join(vol_source)})
>  
> +        create_param = vol_source[index_list[0]]
> +
> +        if name is None:
> +            # the methods listed in 'REQUIRE_NAME_PARAMS' cannot have
> +            # 'name' == None
> +            if create_param in REQUIRE_NAME_PARAMS:
> +                raise InvalidParameter('KCHVOL0016E')
> +
> +            # if 'name' is omitted - except for the methods listed in
> +            # 'REQUIRE_NAME_PARAMS' - the default volume name will be the
> +            # file/URL basename.
> +            name = os.path.basename(create_param)
> +            params['name'] = name
> +
>          try:
> -            create_func = getattr(self, "_create_volume_with_" +
> -                                        vol_source[index_list[0]])
> +            create_func = getattr(self, '_create_volume_with_%s' %
> +                                        create_param)
>          except AttributeError:
> -            raise InvalidParameter("KCHVOL0019E",
> -                                   {'param': vol_source[index_list[0]]})
> +            raise InvalidParameter("KCHVOL0019E", {'param': create_param})
>  
>          pool_info = StoragePoolModel(conn=self.conn,
>                                       objstore=self.objstore).lookup(pool_name)
> diff --git a/tests/test_model.py b/tests/test_model.py
> index 4e9ba97..ecb0a8a 100644
> --- a/tests/test_model.py
> +++ b/tests/test_model.py
> @@ -526,10 +526,13 @@ class ModelTests(unittest.TestCase):
>  
>              vols = inst.storagevolumes_get_list(pool)
>              num = len(vols) + 2
> -            params = {'name': vol,
> -                      'capacity': 1024,
> +            params = {'capacity': 1024,
>                        'allocation': 512,
>                        'format': 'raw'}
> +            # 'name' is required for this type of volume
> +            self.assertRaises(InvalidParameter, inst.storagevolumes_create,
> +                              pool, params)
> +            params['name'] = vol
>              task_id = inst.storagevolumes_create(pool, params)['id']
>              self._wait_task(inst, task_id)
>              self.assertEquals('finished', inst.task_lookup(task_id)['status'])
> @@ -560,20 +563,22 @@ class ModelTests(unittest.TestCase):
>  
>              # download remote volume
>              # 1) try an invalid URL
> -            params = {'name': 'foo', 'url': 'http://www.invalid.url'}
> +            params = {'url': 'http://www.invalid.url'}
>              taskid = inst.storagevolumes_create(pool, params)['id']
>              self._wait_task(inst, taskid)
>              self.assertEquals('failed', inst.task_lookup(taskid)['status'])
>              # 2) download Kimchi's "COPYING" from Github and compare its
>              #    content to the corresponding local file's
>              url = 'https://github.com/kimchi-project/kimchi/raw/master/COPYING'
> -            params = {'name': 'copying', 'url': url}
> -            taskid = inst.storagevolumes_create(pool, params)['id']
> +            params = {'url': url}
> +            task_response = inst.storagevolumes_create(pool, params)
> +            taskid = task_response['id']
> +            vol_name = task_response['target_uri'].split('/')[-1]
>              self._wait_task(inst, taskid)
>              self.assertEquals('finished', inst.task_lookup(taskid)['status'])
>              rollback.prependDefer(inst.storagevolume_delete, pool,
>                                    params['name'])
> -            vol_path = os.path.join(args['path'], params['name'])
> +            vol_path = os.path.join(args['path'], vol_name)
>              self.assertTrue(os.path.isfile(vol_path))
>              with open(vol_path) as vol_file:
>                  vol_content = vol_file.read()
> diff --git a/tests/test_rest.py b/tests/test_rest.py
> index ae1c971..a3dcf87 100644
> --- a/tests/test_rest.py
> +++ b/tests/test_rest.py
> @@ -489,6 +489,14 @@ class RestTests(unittest.TestCase):
>              resp = self.request('/storagepools/tmp/activate', req, 'POST')
>              self.assertEquals(200, resp.status)
>  
> +            # 'name' is required for this type of volume
> +            req = json.dumps({'capacity': 1024,
> +                              'allocation': 512,
> +                              'type': 'disk',
> +                              'format': 'raw'})
> +            resp = self.request('/storagepools/tmp/storagevolumes',
> +                                req, 'POST')
> +            self.assertEquals(400, resp.status)
>              req = json.dumps({'name': "attach-volume",
>                                'capacity': 1024,
>                                'allocation': 512,
> @@ -1027,16 +1035,16 @@ class RestTests(unittest.TestCase):
>          self.assertEquals('/var/lib/libvirt/images/volume-1',
>                            storagevolume['path'])
>  
> -        req = json.dumps({'name': 'downloaded',
> -                          'url': 'https://anyurl.wor.kz'})
> +        req = json.dumps({'url': 'https://anyurl.wor.kz'})
>          resp = self.request('/storagepools/pool-1/storagevolumes', req, 'POST')
>          self.assertEquals(202, resp.status)
>          task = json.loads(resp.read())
>          self._wait_task(task['id'])
>          task = json.loads(self.request('/tasks/%s' % task['id']).read())
>          self.assertEquals('finished', task['status'])
> -        resp = self.request('/storagepools/pool-1/storagevolumes/downloaded',
> -                            '{}', 'GET')
> +        vol_name = task['target_uri'].split('/')[-1]
> +        resp = self.request('/storagepools/pool-1/storagevolumes/%s' %
> +                            vol_name, '{}', 'GET')
>          self.assertEquals(200, resp.status)
>  
>          # Now remove the StoragePool from mock model




More information about the Kimchi-devel mailing list