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.

Attachment: binBzK83O11Mp.bin
Description: freeipa-jraquino-0023-Optimize-and-dynamically-verify-group-membership.patch

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to