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

[JR Aquino]

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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jraquino-0023-Optimize-and-dynamically-verify-group-membership.patch
Type: application/octet-stream
Size: 6399 bytes
Desc: freeipa-jraquino-0023-Optimize-and-dynamically-verify-group-membership.patch
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110420/bd31b43c/attachment.obj>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: ATT00001.txt
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110420/bd31b43c/attachment.txt>