[PATCH] issue#383: Only allow one type of parameter when adding vm storage
From: Royce Lv <lvroyce@linux.vnet.ibm.com> When adding vm storage, volume and path cannot be specified at the same time. Fix it. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/model/vmstorages.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py index 8c51716..093b9ee 100644 --- a/src/kimchi/model/vmstorages.py +++ b/src/kimchi/model/vmstorages.py @@ -154,6 +154,8 @@ class VMStoragesModel(object): # Path will never be blank due to API.json verification. # There is no need to cover this case here. params['format'] = 'raw' + if 'vol' in params and 'path' in params: + raise InvalidParameter("KCHVMSTOR0012E") if params.get('vol'): try: pool = params['pool'] -- 1.8.3.2
On 06/26/2014 03:35 PM, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
When adding vm storage, volume and path cannot be specified at the same time. Fix it.
Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/model/vmstorages.py | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py index 8c51716..093b9ee 100644 --- a/src/kimchi/model/vmstorages.py +++ b/src/kimchi/model/vmstorages.py @@ -154,6 +154,8 @@ class VMStoragesModel(object): # Path will never be blank due to API.json verification. # There is no need to cover this case here. params['format'] = 'raw' + if 'vol' in params and 'path' in params: + raise InvalidParameter("KCHVMSTOR0012E") "KCHVMSTOR0012E": _("Specify type and path or type and pool/volume to add a new virtual machine disk")
It is OK. But IMHO, a more precise error message is better for user, such as "can not specify both path and pool/volume to a new virtual machine disk"
if params.get('vol'): try: pool = params['pool']
-- Thanks and best regards! Sheldon Feng(冯少合)<shaohef@linux.vnet.ibm.com> IBM Linux Technology Center
On 2014年06月26日 21:10, Sheldon wrote:
On 06/26/2014 03:35 PM, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
When adding vm storage, volume and path cannot be specified at the same time. Fix it.
Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/model/vmstorages.py | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py index 8c51716..093b9ee 100644 --- a/src/kimchi/model/vmstorages.py +++ b/src/kimchi/model/vmstorages.py @@ -154,6 +154,8 @@ class VMStoragesModel(object): # Path will never be blank due to API.json verification. # There is no need to cover this case here. params['format'] = 'raw' + if 'vol' in params and 'path' in params: + raise InvalidParameter("KCHVMSTOR0012E") "KCHVMSTOR0012E": _("Specify type and path or type and pool/volume to add a new virtual machine disk")
It is OK.
But IMHO, a more precise error message is better for user, such as "can not specify both path and pool/volume to a new virtual machine disk" ACK.
if params.get('vol'): try: pool = params['pool']
On 06/26/2014 04:35 AM, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
When adding vm storage, volume and path cannot be specified at the same time. Fix it.
Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/model/vmstorages.py | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py index 8c51716..093b9ee 100644 --- a/src/kimchi/model/vmstorages.py +++ b/src/kimchi/model/vmstorages.py @@ -154,6 +154,8 @@ class VMStoragesModel(object): # Path will never be blank due to API.json verification. # There is no need to cover this case here. params['format'] = 'raw' + if 'vol' in params and 'path' in params: + raise InvalidParameter("KCHVMSTOR0012E") if params.get('vol'): try: pool = params['pool']
This is one part of the solution we should have for #382 if type not in ['disk', 'cdrom']: raise InvalidParameter(Disk type not support) path = params.get(path, None) if type == 'cdrom': if not path: raise MissingParameter() # validate path and continue the logic elif type == 'disk': pool = params.get(pool, None) vol = params.get(vol, None) if not pool or not vol: raise MissingParameter() # validate values and continue the logic
On 2014?06?27? 04:57, Aline Manera wrote:
On 06/26/2014 04:35 AM, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv<lvroyce@linux.vnet.ibm.com>
When adding vm storage, volume and path cannot be specified at the same time. Fix it.
Signed-off-by: Royce Lv<lvroyce@linux.vnet.ibm.com> --- src/kimchi/model/vmstorages.py | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py index 8c51716..093b9ee 100644 --- a/src/kimchi/model/vmstorages.py +++ b/src/kimchi/model/vmstorages.py @@ -154,6 +154,8 @@ class VMStoragesModel(object): # Path will never be blank due to API.json verification. # There is no need to cover this case here. params['format'] = 'raw' + if 'vol' in params and 'path' in params: + raise InvalidParameter("KCHVMSTOR0012E") if params.get('vol'): try: pool = params['pool']
This is one part of the solution we should have for #382 ACK.
if type not in ['disk', 'cdrom']: raise InvalidParameter(Disk type not support) This part is covered by jsonschema.
path = params.get(path, None) if type == 'cdrom': if not path: raise MissingParameter()
I think we need to reserve volume/pool for cdrom type, so that in the future, ui will be able to pick one iso from the iso pool. What do you think?
# validate path and continue the logic
elif type == 'disk': pool = params.get(pool, None) vol = params.get(vol, None) if not pool or not vol: raise MissingParameter()
# validate values and continue the logic
We have 3 issues here: 1. type needs to be in 'cdrom' and 'disk' -- already covered by current json schema. 2. For type 'disk', "pool" and "vol" need to be existed -- need to be covered by json schema 'dependencies' key word. 3. For type 'cdrom', one of "path" or "pool"+"vol" needs to be existed, but not at the same time -- need to be covered by json schema 'dependencies' and "oneof" keywords. When I'm considering to introduce jsonschema "oneOf" keyword, (http://json-schema.org/latest/json-schema-validation.html#anchor88) jsonschema V4 support this, and this (python-jsonschema 2.3.0) is only available on ubuntu 14.04 and above. Forturnately, 13.10 ends support in July 2014. So do you think we can add this now? REF: https://launchpad.net/ubuntu/+source/python-jsonschema https://wiki.ubuntu.com/Releases I will send patches using oneof and validate by python code. So that you can merge it before release.
On 2014?06?27? 11:35, Royce Lv wrote:
On 2014?06?27? 04:57, Aline Manera wrote:
On 06/26/2014 04:35 AM, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv<lvroyce@linux.vnet.ibm.com>
When adding vm storage, volume and path cannot be specified at the same time. Fix it.
Signed-off-by: Royce Lv<lvroyce@linux.vnet.ibm.com> --- src/kimchi/model/vmstorages.py | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py index 8c51716..093b9ee 100644 --- a/src/kimchi/model/vmstorages.py +++ b/src/kimchi/model/vmstorages.py @@ -154,6 +154,8 @@ class VMStoragesModel(object): # Path will never be blank due to API.json verification. # There is no need to cover this case here. params['format'] = 'raw' + if 'vol' in params and 'path' in params: + raise InvalidParameter("KCHVMSTOR0012E") if params.get('vol'): try: pool = params['pool']
This is one part of the solution we should have for #382 ACK.
if type not in ['disk', 'cdrom']: raise InvalidParameter(Disk type not support) This part is covered by jsonschema.
path = params.get(path, None) if type == 'cdrom': if not path: raise MissingParameter()
I think we need to reserve volume/pool for cdrom type, so that in the future, ui will be able to pick one iso from the iso pool. What do you think?
# validate path and continue the logic
elif type == 'disk': pool = params.get(pool, None) vol = params.get(vol, None) if not pool or not vol: raise MissingParameter()
# validate values and continue the logic
We have 3 issues here: 1. type needs to be in 'cdrom' and 'disk' -- already covered by current json schema. 2. For type 'disk', "pool" and "vol" need to be existed -- need to be covered by json schema 'dependencies' key word. 3. For type 'cdrom', one of "path" or "pool"+"vol" needs to be existed, but not at the same time -- need to be covered by json schema 'dependencies' and "oneof" keywords.
When I'm considering to introduce jsonschema "oneOf" keyword, (http://json-schema.org/latest/json-schema-validation.html#anchor88) jsonschema V4 support this, and this (python-jsonschema 2.3.0) is only available on ubuntu 14.04 and above. After integrated with jsonschema V4, found it broke a lot of existed schema because of bug: http://flexget.com/ticket/2268 So just work around with validate with python code. Forturnately, 13.10 ends support in July 2014. So do you think we can add this now? REF: https://launchpad.net/ubuntu/+source/python-jsonschema https://wiki.ubuntu.com/Releases
I will send patches using oneof and validate by python code. So that you can merge it before release.
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
participants (4)
-
Aline Manera -
lvroyce＠linux.vnet.ibm.com -
Royce Lv
-
Sheldon