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.
From 5e0fff734c830878b4b0a3869931ee70629aff84 Mon Sep 17 00:00:00 2001
From: "Thierry bordaz (tbordaz)" <tbor...@redhat.com>
Date: Tue, 7 Oct 2014 18:41:44 +0200
Subject: [PATCH] permission-add gives confusing error when adding ACI to
generated tree
Raise that 'ipapermlocation' is invalid when it applies on a non existing entry
https://fedorahosted.org/freeipa/ticket/4523
---
ipalib/plugins/permission.py | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/ipalib/plugins/permission.py b/ipalib/plugins/permission.py
index 9028f02483bc113c19c75b94d70dd1b133272524..35ed2e483a65fb266bb2dfe78b020d0f5b549759 100644
--- a/ipalib/plugins/permission.py
+++ b/ipalib/plugins/permission.py
@@ -943,7 +943,13 @@ class permission_add(baseldap.LDAPCreate):
# the whole command, not just the callbacks
def execute(self, *keys, **options):
self.obj.preprocess_options(options, merge_targetfilter=True)
- return super(permission_add, self).execute(*keys, **options)
+ try:
+ res = super(permission_add, self).execute(*keys, **options)
+ except errors.NotFound:
+ raise errors.ValidationError(
+ name='ipapermlocation',
+ error=_('Entry %s does not exist') % self.obj.get_dn(*keys, **options))
+ return res
def get_args(self):
for arg in super(permission_add, self).get_args():
--
1.7.11.7
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
