[Kimchi-devel] vmtemplate logic while validating disk parameters

[Aline Manera]

On 02/21/2017 09:32 AM, Ramon Medeiros wrote:
>
>
> On 02/20/2017 10:53 PM, Aline Manera wrote:
>> Hi,
>>
>> On 02/15/2017 05:34 PM, Ramon Medeiros wrote:
>>> Hi,
>>>
>>>
>>> just looking at kimchi/vmtemplate.py, i saw this confusing logic:
>>>
>>>
>>> 101         basic_disk = ['index', 'format', 'pool', 'size']
>>> 102         basic_path_disk = ['index', 'format', 'path', 'size']
>>> 103         ro_disk = ['index', 'format', 'pool', 'volume']
>>> 104         base_disk = ['index', 'base', 'pool', 'size', 'format']
>>> 105         base_path_disk = ['index', 'base', 'path', 'size', 
>>> 'format']
>>>
>>>
>>>
>>> 148                 if ((keys != sorted(basic_disk)) and
>>> 149                         (keys != sorted(ro_disk)) and
>>> 150                         (keys != sorted(base_disk))):
>>> 151                     # Addition check required only on s390x
>>> 152                     if not is_s390x() or (keys != 
>>> sorted(basic_path_disk)):
>>> 153                         raise MissingParameter('KCHTMPL0028E')
>>>
>>> The code is trying to validate if all fields are present, based on 
>>> combinations of parameters. I want to add more parameters, that are 
>>> non-optional. 
>>
>> What do you mean about non-optional parameter? Will that new 
>> parameter be required for any type of disk? Shouldn't Kimchi set the 
>> default value when no one is passed?
> you are right, i wrote wrong. They are optional
>>
>>> Is it good just add it or it's worth to rethink this logic?
>>>
>>
>> Why do we need to rethink this logic? What is wrong there?
> The verification is based on list with disks keys, to verify if all of 
> them where passed.
>
> When working with optional parameters, is it ok to removing the from 
> the verification, like this:
>
>
> for param in optional_param:
>     keys.remove(param)
>
>

If you are working with optional parameters, you should not use this 
same logic to do that.
That logic is validating a set of required parameters given a type of disk.

> .....
> 101         basic_disk = ['index', 'format', 'pool', 'size']
> 102         basic_path_disk = ['index', 'format', 'path', 'size']
> 103         ro_disk = ['index', 'format', 'pool', 'volume']
> 104         base_disk = ['index', 'base', 'pool', 'size', 'format']
> 105         base_path_disk = ['index', 'base', 'path', 'size', 'format']
>
>
>
> 148                 if ((keys != sorted(basic_disk)) and
> 149                         (keys != sorted(ro_disk)) and
> 150                         (keys != sorted(base_disk))):
> 151                     # Addition check required only on s390x
> 152                     if not is_s390x() or (keys != 
> sorted(basic_path_disk)):
> 153                         raise MissingParameter('KCHTMPL0028E')
>>
>>> What about removing this, since some validations already take place 
>>> at API.json?
>>
>> The disk validation can not be done on API.json only. The API.json 
>> only validates the paramete type but not a combination of the 
>> required fields given a type of disk.
>> So this logic still makes sense.
>>
>>
>