[Freeipa-devel] [PATCHES] 198-202 Refactor indirect membership processing

[Petr Viktorin]

On 11/25/2013 03:27 PM, Jan Cholasta wrote:
> On 8.11.2013 17:56, Petr Viktorin wrote:
>>> Patch 198:
>>>
>>> Also update ipaldap's find_entries docstring, it no longer uses IPA
>>> defaults.
>
> Done.
>
>>>
>>>
>>> While you're touching this part of code, I had some other improvements
>>> in mind -- you can consider them:
>>>
>>> In find_entries,
>>>      attrs_list = [a.lower() for a in attrs_list]
>>> to make sure 'memberindirect' is case insensitive
>
> Done.
>
>>
>>> In get_memberof, construct `indirect` as a set, for Ο(1) remove().
>> ^ ignore that, it's nuked in 201 \o/
>>
>>> Changing MEMBERS_ALL et.al. from numbers to descriptive strings, for
>>> easier debugging.
>> ^ these can be removed entirely in 201
>
> Removed.
>
>>
>>
>>>
>>> Patch 199: Looks great
>>>
>>>
>>> Patch 200:
>>>
>>> objtype, res_list, red_id, res_ctrls = result
>>> Minor typo ----------^
>
> Fixed.
>
>>>
>>>
>>> This construction won't work as you'd expect in Python 2:
>>>
>>> try:
>>>      (possibly raise interesting exception) (*)
>>> except:
>>>      try:
>>>          (possibly raise exception to ignore) (**)
>>>      except:
>>>          pass
>>>      raise  # (***)
>>>
>>> The problem is that the exception in (**) overwrites the "current active
>>> exception" raised in (*). In (***) the exception from the cleanup
>>> will be
>>> re-raised.
>>> The solution is to store the wanted exception info, including the
>>> traceback:
>>>      exc_type, exc_value, exc_traceback = sys.exc_info()
>>> and then re-raise explicitly:
>>>      raise exc_type, exc_value, exc_traceback
>
> Turns out LDAPError does not fly well with sys.exc_info() (all exception
> data is lost, probably a python-ldap bug), so I used "except LDAPError,
> e: ... raise e" instead.
>
>>>
>>> Also, please log the ignored exception from cancelling the paged search.
>
> Done.
>
>>>
>>>
>>
>>
>> Patch 201:
>> Great patch!
>> A nitpick, I'd rename _process_member{,of} to
>> _process_member{,of}indirect
>
> Renamed.
>
>>
>>
>> Patch 202: Looks good
>> While we're on the subject: Each Plugin has an "api" attribute. It would
>> be nice if we started preferring `self.api` instead of the global
>> singleton wherever possible, even though they're currently always the
>> same.
>
> +1, fixed.
Not fixed, but it's okay for now.
>
> Updated patches attached.
>
> Honza
>

Thanks! ACK, pushed to master: 4050e553c30d8c8d93c7045f871f8c1cef65aa71

-- 
Petr³