On Wed, Jan 15, 2014 at 02:14:08PM +0200, Alexander Bokovoy wrote: > On Tue, 14 Jan 2014, Sumit Bose wrote: > >On Tue, Jan 14, 2014 at 04:03:06PM +0200, Alexander Bokovoy wrote: > >>On Tue, 14 Jan 2014, Martin Kosek wrote: > >>>On 01/14/2014 01:27 PM, Alexander Bokovoy wrote: > >>>>On Tue, 14 Jan 2014, Martin Kosek wrote: > >>>>>On 01/14/2014 01:02 PM, Alexander Bokovoy wrote: > >>>>>>Hi, > >>>>>> > >>>>>>attached patch implements missing idranges when new child domains > >>>>>>discovered through 'ipa trust-fetch-domains'. This functionality existed > >>>>>>in 'ipa trust-add' but was not exposed in the 'ipa trust-fetch-domains'. > >>>>>> > >>>>>>Additionally, error message is shown in case trust name is incorrect. > >>>>>> > >>>>>>https://fedorahosted.org/freeipa/ticket/4104 > >>>>>>https://fedorahosted.org/freeipa/ticket/4111 > >>>>>> > >>>>> > >>>>>I did not test the patch, just few observations from reading it: > >>>>It is generally wrong to base opinion purely on reading the code (see > >>>>below > >>>>why) :) > >>> > >>>One does not need to run the code that see the places where it may be > >>>rusty :) > >>> > >>>> > >>>>>1) Why are you moving add_range method from trust object to the module? > >>>>>IMO it > >>>>>could be left there, it belongs to the object. Plus, the patch won't be > >>>>>that > >>>>>big and easier to backport. add_range can be still referred from other > >>>>>commands > >>>>>as "self.obj.add_range", no need to move it. > >>>>It was in trust_add class, not in the object. I need it in the other > >>>>code and without explicit dependency on the object. > >>> > >>>Ok. Though I would still consider having it rather in the trust object to > >>>make > >>>it easier accessible from other modules, though our API system. > >>I deliberately don't want that. This is internal API for purposes of > >>trust code -- do you envision any situation when anyone else might want > >>to create idranges programmatically for child domains of the existing > >>trust? Note that you are required to know fairly low-level details of > >>the AD trusts to call it. > >> > >>> > >>>> > >>>>>2) This part looks *very* suspicious: > >>>>> > >>>>>- trust = self.api.Command.trust_show(keys[0], raw=True)['result'] > >>>>>+ try: > >>>>>+ trust = self.api.Command.trust_show(keys[0], > >>>>>raw=True)['result'] > >>>>>+ except AssertionError: > >>>>> > >>>>>I do not think that AssertionError should be raised and caught in normal > >>>>>operation, it should be an exceptional exception raised when the world > >>>>>falls > >>>>>apart IMO. I.e. I would rather see some PublicError or Exception based > >>>>>exception to be raised in trust_show in that case... > >>>>It *is* raised and should be caught because this particular snippet is > >>>>to catch situation when wrong trust domain name is passed. Previously > >>>>the code simply generated server-side exception which resulted in > >>>>'internal error'. > >>> > >>>Ok, understood. My point is, trust_show should not raise AssertException > >>>just > >>>because wrong trust domain is passed. There are better means to express > >>>that > >>>error - ValidationError or NotFound, based on the situation. > >>Yes, this is due to the way trust.get_dn() is built. Do not commit this > >>patch yet (though building packages for testing would be good), I'm > >>reworking it a bit to move this logic to get_dn() -- otherwise > >>LDAPRetrieve() will always issue AssertError. > > > >I guess I already hit this while testing. trust-fetch-domains does not > >work for valid forest roots anymore :-) > > > >Btw, can you add the forest root check to trustdomain-find as well? > I've fixed it by returning an exception from get_dn(), as it is expected > from other objects' get_dn(). > > new version attached.
Thanks. I can give functional ACK. All my checks are working as expected, idranges are added if needed and 'ipa: ERROR: no such entry' is displayed if the forest name is invalid. Btw I think you can add https://fedorahosted.org/freeipa/ticket/4095 to the commit message as well. The python code looks good to me as well. bye, Sumit > > -- > / Alexander Bokovoy _______________________________________________ Freeipa-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/freeipa-devel
