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

[Jan Cholasta]

Dne 11.11.2011 20:07, Rob Crittenden napsal(a):
> 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.

Just tested a few commands that use CSV (selfservice-find, dnszone-add, 
dnszone-del) from an unpatched client and everything seems to be working 
fine.

>
> thanks
>
> rob

I have updated the patch with your suggestions and rebased it on top of 
current master. See attachment.

Honza

-- 
Jan Cholasta
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-55.2-comma-separated-list.patch
Type: text/x-patch
Size: 131954 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20111121/c1059dba/attachment.bin>