[Freeipa-devel] [PATCH] 23 Optimize and dynamically verify group membership

[Rob Crittenden]

JR Aquino wrote:
> On Apr 20, 2011, at 10:32 AM, Rob Crittenden wrote:
> ...
>
>>>> Seems to work as advertised, I just have a couple of requests:
>>>>
>>>> - Some of the comments are really long, can you limit to ~75 chars per line?
>>>> - In this code block:
>>>>
>>>>         for r in results:
>>>>             direct.append(r[0])
>>>>             try:
>>>>                 indirect.remove(r[0].lower())
>>>>             except ValueError:
>>>>                 pass
>>>>
>>>> We should log if a ValueError is thrown, this would mean something is really wrong. Can you import logging in the file and replace pass with something like:
>>>>
>>>> logging.info('Failed to remove indirect entry %s from %s' % r[0], entry_dn)
>>>>
>>>> I wonder if we should raise the ValueError too. This means that something went very wrong.
>>>
>>> Yes I agree that we should raise the error.
>>>
>>> Here is the patch with the requested changes:
>>>
>>> * Fixed width to be PEP8 compliant
>>> * import logging
>>> * Added logging line in exception
>>> * Raise ValueError if we blow up during indirect removal.
>>>
>>
>> Fixes 1-3 look good. I think for the exception you want:
>>
>> except ValueError, e:
>>    logging ....
>>    raise e
>>
>> A ValueError won't get returned over XML-RPC but the full backtrace will be logged on the server side. The end-user will get a 903 (Internal error raised).
>>
>> rob
>
> Ok adjusted patch attached to correct for the exception raised.
>

ack, pushed to master and ipa-2-0.

Before pushing I removed some trailing whitespace and amended the commit 
message to add more information.

rob