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 ?

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.

thanks
thierry


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

Reply via email to