[Freeipa-devel] [PATCH] 23 Optimize and dynamically verify group membership
JR Aquino
JR.Aquino at citrix.com
Wed Apr 20 18:19:29 UTC 2011
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>
More information about the Freeipa-devel
mailing list