[Freeipa-devel] [PATCH] 55 Parse comma-separated lists of values in all parameter types

[Rob Crittenden]

Jan Cholasta wrote:
> Dne 4.11.2011 22:25, Rob Crittenden napsal(a):
>> Jan Cholasta wrote:
>>> Dne 24.10.2011 17:42, Rob Crittenden napsal(a):
>>>> Jan Cholasta wrote:
>>>>> Dne 20.10.2011 13:20, Jan Cholasta napsal(a):
>>>>>> Parse comma-separated lists of values in all parameter types. This
>>>>>> can
>>>>>> enabled for a specific parameter by setting the "csvlist" option to
>>>>>> True.
>>>>>>
>>>>>> Remove List parameter type and replace all occurences with Str with
>>>>>> csvlist enabled.
>>>>>>
>>>>>> https://fedorahosted.org/freeipa/ticket/2007
>>>>>>
>>>>>> This change will be useful for
>>>>>> https://fedorahosted.org/freeipa/ticket/1487 and
>>>>>> https://fedorahosted.org/freeipa/ticket/1847
>>>>>>
>>>>>> Unit tests show no regressions.
>>>>>>
>>>>>> Honza
>>>>>>
>>>>>
>>>>> Self-NACK - I have noticed that the batch command no longer works.
>>>>>
>>>>> Updated patch attached.
>>>>>
>>>>> Honza
>>>>
>>>> What is the benefit of this over the List parameter type?
>>>>
>>>> rob
>>>
>>> Mainly because the List parameter type is just a hack. This is the right
>>> thing to do if we want to use comma-separated lists of parameters of any
>>> type, with all the validation and other parameter type-specific
>>> features.
>>>
>>> For example, I've added a new parameter type for IP addresses in my
>>> patch 46
>>> (http://www.redhat.com/archives/freeipa-devel/2011-September/msg00187.html)
>>>
>>>
>>> and use it for A and AAAA DNS records. Without this patch, we can either
>>> use List for the record parameters and lose validation in dnsrecord-find
>>> (because it is based on crud.Search, which strips all the custom
>>> validation rules - like _validate_ipaddr - from the command parameters,
>>> which is one of the causes of #1627) or use IPAddress for the record
>>> parameters and lose the ability to specify them as comma-separated list
>>> of values. With this patch, we can have both comma-separated lists and
>>> validation at the same time.
>>>
>>> Besides, the patch is not as big as it looks like, all the interesting
>>> stuff is in ipalib/parameters.py, everything else is just
>>> search-and-replace. Also I need it to fix #1487 and #1847 without doing
>>> ugly hacks.
>>>
>>> Honza
>>>
>>
>> I think this would constitute a major version change.
>
> I'm not sure about that, as the patch doesn't break API compatibility -
> a string containing a comma-separated list of values is used for list
> parameters both with and without the patch.
>
>>
>> One downside is you can no longer tell in the help with arguments take a
>> CSV and which don't.
>
> Why not? A simple look at csvlist value should provide enough information.
>
>>
>> I think the CSV-related Parameter options should all begin with csv,
>> separator and skipspace.
>
> OK.
>
>>
>> The batch command may eventually be made into a command, how will that
>> affect the Any type?
>
> Batch currently uses List for the "methods" parameter not because of its
> CSV capability, but because it doesn't do any type conversion and
> validation on the values. This allows it to use values of the form
> "{u'params': [args_list, kwargs_dict], u'method': method_name}". The Any
> parameter type exists so that this can still be done without List - it's
> basically a single-valued version of List (i.e. Any(csvlist=True) is
> equivalent to List()).
>
> IMO if batch is ever made into a command, it would have to be redesigned
> not to use List/Any, so that it can be used from CLI with validation of
> the parameter values. Any can then be easily removed.
>
>>
>> It otherwise seems to work in my spot-testing.
>>
>> rob
>
> Honza
>

Given that we want to maintain backward API compatibility can you make 
sure older clients will work with this? My gut tells me it will since 
this is really a marshaling issue but I don't want to assume.

thanks

rob