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
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
