[Kimchi-devel] vmtemplate logic while validating disk parameters

[Ramon Medeiros]

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)


.....
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.
>
>