[Freeipa-devel] [PATCH] 0004 permission-add gives confusing error when adding ACI to generated tree
thierry bordaz
tbordaz at redhat.com
Thu Oct 16 10:08:21 UTC 2014
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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141016/2a55573b/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbordaz-0004-2-permission-add-gives-confusing-error-when-adding-ACI.patch
Type: text/x-patch
Size: 1473 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141016/2a55573b/attachment.bin>
More information about the Freeipa-devel
mailing list