-- Reviewed-by: Paulo Vital <pvital@linux.vnet.ibm.com> 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@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", "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