[Freeipa-devel] [PATCH] 0130 -- create missing idranges in trust-fetch-domains

Martin Kosek mkosek at redhat.com
Wed Jan 15 14:46:27 UTC 2014


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




More information about the Freeipa-devel mailing list