On 10/16/2014 03:32 PM, thierry bordaz wrote: > On 10/16/2014 12:45 PM, Martin Kosek wrote: >> 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 > Thanks Martin for the patch and the explanations. I missed that the > LDAPCreate > triggered the exception when calling permission_add.post_callback. Now I > understand what you meant by catching the exception as close as possible. > > Thierry >
I understand that as an ACK from you then :-) Given the effort I had to develop the patch, I will push that original version and add you as a reviewer. Pushed to: master: 061f7ff331531fa01801fb597feed924de6a2fd7 ipa-4-1: 0a54b1c948e5c25d971f5b0ef4bc079cbd605069 Thanks! Martin _______________________________________________ Freeipa-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/freeipa-devel
