On Wed, 2012-03-28 at 10:39 +0200, Jan Cholasta wrote: > On 27.3.2012 17:41, Martin Kosek wrote: > > On Tue, 2012-03-27 at 16:42 +0200, Martin Kosek wrote: > >> On Tue, 2012-03-27 at 16:30 +0200, Jan Cholasta wrote: > >>> On 27.3.2012 16:00, Martin Kosek wrote: > >>>> On Thu, 2012-03-15 at 14:57 +0100, Jan Cholasta wrote: > >>>>> On 15.3.2012 14:20, Petr Viktorin wrote: > >>>>>> On 03/15/2012 12:05 PM, Jan Cholasta wrote: > >>>>>>> On 15.3.2012 11:36, Jan Cholasta wrote: > >>>>>>>> (this is a continuation of > >>>>>>>> <http://www.redhat.com/archives/freeipa-devel/2011-September/msg00327.html>) > >>>>>>>> > >>>>>>>> > >>>>>>>> > >>>>>>>> > >>>>>>>> Hi, > >>>>>>>> > >>>>>>>> the attached patches > >>>>>>>> fix<https://fedorahosted.org/freeipa/ticket/1847> > >>>>>>>> and<https://fedorahosted.org/freeipa/ticket/2245>: > >>>>>>>> > >>>>>>>> [PATCH] Fix the procedure for getting default values of command > >>>>>>>> parameters. > >>>>>>>> > >>>>>>>> The parameters used in default_from of other parameters are now > >>>>>>>> properly > >>>>>>>> validated before the default_from is called. > >>>>>>>> > >>>>>>>> [PATCH] Change parameters to use only default_from for dynamic > >>>>>>>> default > >>>>>>>> values. > >>>>>>>> > >>>>>>>> Replace all occurences of create_default with equivalent default_from > >>>>>>>> and remove create_default from the framework. This is needed for > >>>>>>>> proper > >>>>>>>> parameter validation, as there is no way to tell which parameters to > >>>>>>>> validate prior to calling create_default, because create_default does > >>>>>>>> not provide information about which parameters are used for > >>>>>>>> generating > >>>>>>>> the default value. > >>>>>>>> > >>>>>>>> Honza > >>>>>>>> > >>>>>>> > >>>>>>> Forgot to remove one FIXME bit in dns.py. Update patch attached. > >>>>>>> > >>>>>>> Honza > >>>>>> > >>>>>> > >>>>>> > diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py > >>>>>> > index a10960a..61c645d 100644 > >>>>>> > --- a/ipalib/plugins/dns.py > >>>>>> > +++ b/ipalib/plugins/dns.py > >>>>>> > @@ -1528,7 +1528,7 @@ class dnszone(LDAPObject): > >>>>>> > label=_('SOA serial'), > >>>>>> > doc=_('SOA record serial number'), > >>>>>> > minvalue=1, > >>>>>> > - create_default=_create_zone_serial, > >>>>>> > + default_from=_create_zone_serial, > >>>>>> > autofill=True, > >>>>>> > ), > >>>>>> > Int('idnssoarefresh', > >>>>>> > diff --git a/ipalib/plugins/passwd.py b/ipalib/plugins/passwd.py > >>>>>> > index b26f7e9..9bee314 100644 > >>>>>> > --- a/ipalib/plugins/passwd.py > >>>>>> > +++ b/ipalib/plugins/passwd.py > >>>>>> > @@ -69,7 +69,7 @@ class passwd(Command): > >>>>>> > label=_('User name'), > >>>>>> > primary_key=True, > >>>>>> > autofill=True, > >>>>>> > - create_default=lambda **kw: util.get_current_principal(), > >>>>>> > + default_from=lambda: util.get_current_principal(), > >>>>>> > normalizer=lambda value: normalize_principal(value), > >>>>>> > ), > >>>>>> > Password('password', > >>>>>> > >>>>>> > >>>>>> This is just a minor nitpick, but I'd like to know if there's a reason > >>>>>> behind it: why are you sometimes using lambda and sometimes not? > >>>>> > >>>>> I use lambda as a protective measure against accidents caused by adding > >>>>> optional arguments to the functions used. _create_zone_serial is an > >>>>> exception to that rule, because it is private to the dns plugin. > >>>>> > >>>>>> > >>>>>> The patch works well here, but I think I'm not the one to ack it. > >>>>>> > >>>>> > >>>>> Honza > >>>>> > >>>> > >>>> The patch looks OK, I found just minor issues. > >>>> > >>>> 1) We may want to add some check for wildcards (**kw) in default_from, I > >>>> guess it would mess with your dependency solver. Some nice error would > >>>> warn developers that they are doing something bad. > >>> > >>> Added the check. > >>> > >>>> > >>>> 2) Patch 47.4 needs minor rebasing > >>> > >>> Done. > >>> > >>>> > >>>> Martin > >>>> > >>> > >>> Updated patches attached. > >>> > >>> Honza > >>> > >> > >> I think you squashed the change with an incorrect commit. The check > >> should be rather included in patch 44. > > I beg to differ, patch 44 changes the way parameter defaults are > generated, which is not affected by this change at all. The check is > there just to warn developers early if they use default_from in a wrong > way (it wouldn't have worked even without the check). This could have > happened in patch 47, if I had done a bad job replacing create_default > with default_from, so the check belongs there (or into a separate patch).
Ok, it makes a sense as well. > > >> > >> Martin > > > > I just noticed it breaks one unit test: > > > > ====================================================================== > > ERROR: Test the `ipalib.parameters.DefaultFrom.__init__` method. > > ---------------------------------------------------------------------- > > Traceback (most recent call last): > > File "/usr/lib/python2.7/site-packages/nose/case.py", line 197, in > > runTest > > self.test(*self.arg) > > File "/home/mkosek/freeipa/tests/test_ipalib/test_parameters.py", line > > 52, in test_init > > o = self.cls(callback, *keys) > > File "/home/mkosek/freeipa/ipalib/parameters.py", line 201, in __init__ > > raise ValueError("callback: variable-length argument list not allowed") > > ValueError: callback: variable-length argument list not allowed > > > > ---------------------------------------------------------------------- > > Fixed. > > > > > I also think it would be useful to have one test specifically for this > > new check. > > Added. > > > > > Martin > > > > Updated patches attached. > > Honza > ACK. Pushed to master, ipa-2-2. Martin _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel