<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 10/15/2014 04:33 PM, Martin Kosek
wrote:<br>
</div>
<blockquote cite="mid:543E85B7.6090306@redhat.com" type="cite">
<pre wrap="">On 10/15/2014 01:57 PM, thierry bordaz wrote:
</pre>
<blockquote type="cite">
<pre wrap="">On 10/15/2014 01:26 PM, Martin Kosek wrote:
</pre>
<blockquote type="cite">
<pre wrap="">On 10/15/2014 01:08 PM, thierry bordaz wrote:
</pre>
<blockquote type="cite">
<pre wrap=""><a class="moz-txt-link-freetext" href="https://fedorahosted.org/freeipa/ticket/4523">https://fedorahosted.org/freeipa/ticket/4523</a>
</pre>
</blockquote>
<pre wrap="">I see 2 issues with the patch:
1) Patch description should not contain "
Reviewed by:", this gets added later by a script (or human)
</pre>
</blockquote>
<pre wrap="">ok
</pre>
<blockquote type="cite">
<pre wrap="">
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
</pre>
</blockquote>
<pre wrap="">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 ?
</pre>
</blockquote>
<pre wrap="">
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:
...
</pre>
<blockquote type="cite">
<pre wrap="">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.
</pre>
</blockquote>
<pre wrap="">
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
</pre>
</blockquote>
<font face="Times New Roman, Times, serif">Thanks Martin for your
review. Here is a second fix.<br>
<br>
</font>
</body>
</html>