On 01/15/2014 02:54 PM, Sumit Bose wrote: > 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
I have just few of my usual code purity rants :) What is the meaning of this? - except errors.NotFound: - return None + except errors.NotFound, e: + raise e Looks like NOOP to me, except it reraises the exception and thus hides the origin. Otherwise it looks fine. Martin _______________________________________________ Freeipa-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/freeipa-devel
