[Freeipa-devel] [PATCH] 0004 permission-add gives confusing error when adding ACI to generated tree
Martin Kosek
mkosek at redhat.com
Thu Oct 16 10:45:55 UTC 2014
On 10/16/2014 12:08 PM, thierry bordaz wrote:
> On 10/15/2014 04:33 PM, Martin Kosek wrote:
>> On 10/15/2014 01:57 PM, thierry bordaz wrote:
>>> On 10/15/2014 01:26 PM, Martin Kosek wrote:
>>>> On 10/15/2014 01:08 PM, thierry bordaz wrote:
>>>>> https://fedorahosted.org/freeipa/ticket/4523
>>>> I see 2 issues with the patch:
>>>>
>>>> 1) Patch description should not contain "
>>>> Reviewed by:", this gets added later by a script (or human)
>>> ok
>>>> 2) The exception handling clause should be as focused as possible, i.e. not
>>>> including whole command, but rather just the failing call, i.e.:
>>>>
>>>> def post_callback(self, ldap, dn, entry, *keys, **options):
>>>> try:
>>>> self.obj.add_aci(entry)
>>>> except Exception:
>>>>
>>>> You can use
>>>>
>>>> try:
>>>> ...
>>>> except errors.NotFound:
>>>> self.obj.handle_not_found(*keys)
>>>>
>>>> to raise the right error.
>>>>
>>>> Martin
>>> Currently the exception is handled on the failure of
>>> baseldap.LDAPCreate.execute(). Do you recommend to make the fix inside
>>> baseldap.LDAPCreate.execute rather than at the 'permission_add.execute' level ?
>> No, not there. I thought that the exception happens in
>>
>> def post_callback(self, ldap, dn, entry, *keys, **options):
>> try:
>> self.obj.add_aci(entry)
>> except Exception:
>> ...
>>
>>> Also using handle_not_found looks good, but it reports something like:
>>>
>>> ipa permission-add user1 --right read --attrs cn --subtree
>>> 'cn=compat,dc=idm,dc=lab,dc=bos,dc=redhat,dc=com'
>>> ipa: ERROR: user1: permission not found
>>>
>>>
>>> If the entry 'user1' exists, it is not clear what was not found.
>>> Displaying the dn of the entry would help to know that we are updating an entry
>>> into the 'compat' tree.
>> Ah, sorry, I think I mislead you with this advise. You probably could use the
>> same except clause as already used:
>>
>> except errors.NotFound:
>> raise errors.ValidationError(
>> name='ipapermlocation',
>> error=_('Entry %s does not exist') % location)
>>
>> Martin
> Thanks Martin for your review. Here is a second fix.
I am sorry, this is still not what I meant. You cannot wrap whole
permission-add command, wait for very general error and then report very
specific error. Try-except clauses need to be as close to the guarded problem
as possible.
See PEP8 for example (http://legacy.python.org/dev/peps/pep-0008/):
..
Additionally, for all try/except clauses, limit the try clause to the absolute
minimum amount of code necessary. Again, this avoids masking bugs
...
To speed up the resolution of this patch, please see a proposed patch, I hope
it works for you.
Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mkosek-484-raise-better-error-message-for-permission-added-to-g.patch
Type: text/x-patch
Size: 1832 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141016/eb017c6c/attachment.bin>
More information about the Freeipa-devel
mailing list