Re: [Freeipa-devel] [PATCH] 55 Parse comma-separated lists of values in all parameter types
On Wed, 2011-11-30 at 15:19 +0100, Jan Cholasta wrote: > Dne 23.11.2011 16:46, Martin Kosek napsal(a): > > On Wed, 2011-11-23 at 11:14 +0100, Jan Cholasta wrote: > >> Dne 23.11.2011 09:52, Martin Kosek napsal(a): > > ... > >> > >> 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 > > > > The crash _is_ related to the patch since the combination of pylint and > > the changed source code causes it :-) But of course, it does not mean > > that the patch is wrong. > > > > If possible, I would like to push both your acked patch 55 and a patch > > fixing 2136 at one time. I don't want to confuse F-16 users with this > > crash when they try to compile our sources. Therefore, I moved 2136 to > > November milestone. > > > > Martin > > > > Fixed the crash (patch 58, see attachment) and rebased the CSV patch to > current master. > > Honza > ACK. Both pushed to master. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 55 Parse comma-separated lists of values in all parameter types
On Wed, 2011-11-23 at 11:14 +0100, Jan Cholasta wrote: > Dne 23.11.2011 09:52, Martin Kosek napsal(a): ... > > 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 The crash _is_ related to the patch since the combination of pylint and the changed source code causes it :-) But of course, it does not mean that the patch is wrong. If possible, I would like to push both your acked patch 55 and a patch fixing 2136 at one time. I don't want to confuse F-16 users with this crash when they try to compile our sources. Therefore, I moved 2136 to November milestone. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 55 Parse comma-separated lists of values in all parameter types
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 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
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",
Re: [Freeipa-devel] [PATCH] 55 Parse comma-separated lists of values in all parameter types
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 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
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(ast
Re: [Freeipa-devel] [PATCH] 55 Parse comma-separated lists of values in all parameter types
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 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
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 55 Parse comma-separated lists of values in all parameter types
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 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
--
Jan Cholasta
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 55 Parse comma-separated lists of values in all parameter types
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 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. One downside is you can no longer tell in the help with arguments take a CSV and which don't. I think the CSV-related Parameter options should all begin with csv, separator and skipspace. The batch command may eventually be made into a command, how will that affect the Any type? It otherwise seems to work in my spot-testing. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 55 Parse comma-separated lists of values in all parameter types
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 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 -- Jan Cholasta ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 55 Parse comma-separated lists of values in all parameter types
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 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
