[Freeipa-devel] [PATCH] 55 Parse comma-separated lists of values in all parameter types
Jan Cholasta
jcholast at redhat.com
Wed Nov 23 10:14:01 UTC 2011
Dne 23.11.2011 09:52, Martin Kosek napsal(a):
> On Mon, 2011-11-21 at 17:10 +0100, Jan Cholasta wrote:
>> 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
>>
>
> Your patch crashes the ./make-lint script:
>
> # git apply freeipa-jcholast-55.2-comma-separated-list.patch
> # ./make-lint
> Traceback (most recent call last):
> File "./make-lint", line 239, in<module>
> sys.exit(main())
> File "./make-lint", line 210, in main
> linter.check(files)
> File "/usr/lib/python2.7/site-packages/pylint/lint.py", line 493, in check
> self.check_astng_module(astng, walker, rawcheckers)
> File "/usr/lib/python2.7/site-packages/pylint/lint.py", line 565, in check_astng_module
> walker.walk(astng)
> File "/usr/lib/python2.7/site-packages/pylint/utils.py", line 524, in walk
> self.walk(child)
> File "/usr/lib/python2.7/site-packages/pylint/utils.py", line 524, in walk
> self.walk(child)
> File "/usr/lib/python2.7/site-packages/pylint/utils.py", line 524, in walk
> self.walk(child)
> File "/usr/lib/python2.7/site-packages/pylint/utils.py", line 524, in walk
> self.walk(child)
> File "/usr/lib/python2.7/site-packages/pylint/utils.py", line 524, in walk
> self.walk(child)
> File "/usr/lib/python2.7/site-packages/pylint/utils.py", line 524, in walk
> self.walk(child)
> File "/usr/lib/python2.7/site-packages/pylint/utils.py", line 524, in walk
> self.walk(child)
> File "/usr/lib/python2.7/site-packages/pylint/utils.py", line 524, in walk
> self.walk(child)
> File "/usr/lib/python2.7/site-packages/pylint/utils.py", line 521, in walk
> cb(astng)
> File "./make-lint", line 98, in visit_getattr
> ignored = self._find_ignored_attrs(owner)
> File "./make-lint", line 82, in _find_ignored_attrs
> for klass in self._related_classes(owner):
> File "./make-lint", line 74, in _related_classes
> for base in klass.ancestors():
> File "/usr/lib/python2.7/site-packages/logilab/astng/bases.py", line 48, in __getattr__
> return getattr(self._proxied, name)
> AttributeError: 'Function' object has no attribute 'ancestors'
>
Well, the crash is obviously not related to the patch. Fedora 16 uses a
newer version of pylint (0.24), which causes the crash (it works fine on
Fedora 15 with pylint 0.22).
I have opened a ticket to resolve that:
https://fedorahosted.org/freeipa/ticket/2136
>
>
> Besides that, I consider this patch very useful. It is also a
> prerequisite for my new DNS API patch as I use custom non-List based
> parameter types and I need CSV functionality for them.
>
> Martin
>
Let me add that we agreed with Martin that parsing of CSV values should
probably be done on client-side, because it's merely a convenience for
CLI users, not something the protocol depends (or should depend) on. The
downside is that a change like this breaks the API, so we can't do it
right now, but it is one of the things that we should do once we decided
to bump the major version of the API.
Honza
--
Jan Cholasta
More information about the Freeipa-devel
mailing list