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.

Is it good just add it or it's worth to rethink this logic?

What about removing this, since some validations already take place at API.json?

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