On 01/15/2014 03:43 PM, Alexander Bokovoy wrote: > On Wed, 15 Jan 2014, Alexander Bokovoy wrote: >>>>>>> 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. >> Well, I tried to explain it :) >> >> You can drop that bit if you want. > Since it required removal of the whole try block, I've made new version. >
I already did it on my own few second ago :) So updated and pushed to master, ipa-3-3. Martin _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel