On 01/15/2014 03:10 PM, Alexander Bokovoy wrote: > On Wed, 15 Jan 2014, Martin Kosek wrote: >> 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. > The full try block looks like this: > > try: > <retrieve from LDAP> > except errors.NotFound, e: > raise e > else: > <act on the data from LDAP> > > The key here is that it is get_dn(). errors.NotFound here has special > meaning and is intercepted by the framework, never checked by the LDAP* > operations (LDAPRetrieve, LDAPSearch, ...). Other exceptions may get > produced and they'll get shown in the logs but errors.NotFound from > get_dn() will never appear in the stacktrace. > > The change from 'return None' to re-raising exception is intentional. I > could have dropped the whole 'except ...:' stanza too but this is more of > being explicit in intent here to make clear we want to drop out > exceptionally from get_dn() when entry was not found.
Thanks for explanation, but it still does not make sense to me and is IMO a wrong and confusing statement. We also want to drop out in errors.DatabaseError, errors.DatabaseTimeOut, errors.LimitsExceeded and we do not add a special except statement for it. If you really want to add a note about the NotFound case, I would suggest using rather comment which would be less confusing and more informative. Martin _______________________________________________ Freeipa-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/freeipa-devel
