[Freeipa-devel] [PATCH] 0001 Raise ValidationError for incorrect subtree option
Using incorrect input for --subtree option of ipa permission-add command
now raises a ValidationError.
Previously, a ValueError was raised, which resulted in a user unfriendly
error message:
ipa: ERROR: an internal error has occurred
I have added a try-except block to catch the ValueError and raise an
appropriate ValidationError.
https://fedorahosted.org/freeipa/ticket/3233
--
Regards,
Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.
From e6dc0fa67f9b8d6c2e4450af8cded51b0e6afe75 Mon Sep 17 00:00:00 2001
From: Ana Krivokapic akriv...@redhat.com
Date: Wed, 2 Jan 2013 16:12:46 -0500
Subject: [PATCH] Raise ValidationError for incorrect subtree option.
Ticket: https://fedorahosted.org/freeipa/ticket/3233
---
ipalib/plugins/aci.py | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/ipalib/plugins/aci.py b/ipalib/plugins/aci.py
index 702ae010160984636603c75872ddb79bd52c..fa64c8a7a7a3ec9cb04da3729399edb4d501160e 100644
--- a/ipalib/plugins/aci.py
+++ b/ipalib/plugins/aci.py
@@ -341,7 +341,10 @@ def _aci_to_kw(ldap, a, test=False, pkey_only=False):
else:
# See if the target is a group. If so we set the
# targetgroup attr, otherwise we consider it a subtree
-targetdn = DN(target.replace('ldap:///',''))
+try:
+targetdn = DN(target.replace('ldap:///',''))
+except ValueError as e:
+raise errors.ValidationError(name='subtree', error=_(e.message))
if targetdn.endswith(DN(api.env.container_group, api.env.basedn)):
kw['targetgroup'] = targetdn[0]['cn']
else:
--
1.8.0.1
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0001 Raise ValidationError for incorrect subtree option
On 01/03/2013 12:56 PM, Ana Krivokapic wrote:
Using incorrect input for --subtree option of ipa permission-add command
now raises a ValidationError.
Previously, a ValueError was raised, which resulted in a user unfriendly
error message:
ipa: ERROR: an internal error has occurred
I have added a try-except block to catch the ValueError and raise an
appropriate ValidationError.
https://fedorahosted.org/freeipa/ticket/3233
...
--- a/ipalib/plugins/aci.py
+++ b/ipalib/plugins/aci.py
@@ -341,7 +341,10 @@ def _aci_to_kw(ldap, a, test=False, pkey_only=False):
else:
# See if the target is a group. If so we set the
# targetgroup attr, otherwise we consider it a subtree
-targetdn = DN(target.replace('ldap:///',''))
+try:
+targetdn = DN(target.replace('ldap:///',''))
+except ValueError as e:
+raise errors.ValidationError(name='subtree',
error=_(e.message))
`_(e.message)` is useless. The message can only be translated if the
string is grabbed by gettext, which uses static analysis. In other
words, the argument to _() must be a literal string.
You can do either `_(invalid DN)`, or if the error message is
important, include it like this (e.message still won't be translated,
but at least the users will get something in their language):
_(invalid DN (%s)) % e.message
--
Petr³
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0001 Raise ValidationError for incorrect subtree option
On 01/03/2013 01:42 PM, Petr Viktorin wrote:
On 01/03/2013 12:56 PM, Ana Krivokapic wrote:
Using incorrect input for --subtree option of ipa permission-add command
now raises a ValidationError.
Previously, a ValueError was raised, which resulted in a user unfriendly
error message:
ipa: ERROR: an internal error has occurred
I have added a try-except block to catch the ValueError and raise an
appropriate ValidationError.
https://fedorahosted.org/freeipa/ticket/3233
...
--- a/ipalib/plugins/aci.py
+++ b/ipalib/plugins/aci.py
@@ -341,7 +341,10 @@ def _aci_to_kw(ldap, a, test=False,
pkey_only=False):
else:
# See if the target is a group. If so we set the
# targetgroup attr, otherwise we consider it a subtree
-targetdn = DN(target.replace('ldap:///',''))
+try:
+targetdn = DN(target.replace('ldap:///',''))
+except ValueError as e:
+raise errors.ValidationError(name='subtree',
error=_(e.message))
`_(e.message)` is useless. The message can only be translated if the
string is grabbed by gettext, which uses static analysis. In other
words, the argument to _() must be a literal string.
You can do either `_(invalid DN)`, or if the error message is
important, include it like this (e.message still won't be translated,
but at least the users will get something in their language):
_(invalid DN (%s)) % e.message
Fixed.
Thanks, Petr.
--
Regards,
Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.
From 877601dfdd8cde10365cce2cceb701645d2a995c Mon Sep 17 00:00:00 2001
From: Ana Krivokapic akriv...@redhat.com
Date: Thu, 3 Jan 2013 08:40:40 -0500
Subject: [PATCH] Raise ValidationError for incorrect subtree option.
Ticket: https://fedorahosted.org/freeipa/ticket/3233
---
ipalib/plugins/aci.py | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/ipalib/plugins/aci.py b/ipalib/plugins/aci.py
index 702ae010160984636603c75872ddb79bd52c..11a483812e45541ba83cbf800351d0d4ca2ed9de 100644
--- a/ipalib/plugins/aci.py
+++ b/ipalib/plugins/aci.py
@@ -341,7 +341,10 @@ def _aci_to_kw(ldap, a, test=False, pkey_only=False):
else:
# See if the target is a group. If so we set the
# targetgroup attr, otherwise we consider it a subtree
-targetdn = DN(target.replace('ldap:///',''))
+try:
+targetdn = DN(target.replace('ldap:///',''))
+except ValueError as e:
+raise errors.ValidationError(name='subtree', error=_(invalid DN (%s)) % e.message)
if targetdn.endswith(DN(api.env.container_group, api.env.basedn)):
kw['targetgroup'] = targetdn[0]['cn']
else:
--
1.8.0.1
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 0117-0118 Port ipa-replica-prepare to the admintool framework
On 01/03/2013 08:00 AM, Petr Viktorin wrote: Hello, The first patch implements logging-related changes to the admintool framework and ipa-ldap-updater (the only thing ported to it so far). The design document is at http://freeipa.org/page/V3/Logging_and_output John, I decided to go ahead and put an explicit logger attribute on the tool class rather than adding debug, info, warn. etc methods dynamically using log_mgr.get_logger. I believe it's the cleanest solution. We had a discussion about this in this thread: https://www.redhat.com/archives/freeipa-devel/2012-July/msg00223.html; I didn't get a reaction to my conclusion so I'm letting you know in case you have more to say. I'm fine with not directly adding the debug, info, warn etc. methods, that practice was historical dating back to the days of Jason. However I do think it's useful to use a named logger and not the global root_logger. I'd prefer we got away from using the root_logger, it's continued existence is historical as well and the idea was over time we would slowly eliminate it's usage. FWIW the log_mgr.get_logger() is still useful for what you want to do. def get_logger(self, who, bind_logger_names=False) If you don't set bind_logger_names to True (and pass the class instance as who) you won't get the offensive debug, info, etc. methods added to the class instance. But it still does all the other bookeeping. The 'who' in this instance could be either the name of the admin tool or the class instance. Also I'd prefer using the attribute 'log' rather than 'logger'. That would make it consistent with code which does already use get_logger() passing a class instance because it's adds a 'log' attribute which is the logger. Also 'log' is twice as succinct than 'logger' (shorter line lengths). Thus if you do: log_mgr.get_logger(self) I think you'll get exactly what you want. A logger named for the class and being able to say self.log.debug() self.log.error() inside the class. In summary, just drop the True from the get_logger() call. The second patch ports ipa-replica-prepare to the framework. (I chose ipa-replica-prepare because there was a bug filed against its error handling, something the framework should take care of.) As far as Git can tell, it's a complete rewrite, so it might be hard to do a review. I have several smaller patches that make it easier to see what gets moved where. Please say I'm wrong, but as I understand, broken commits aren't allowed in the FreeIPA repo so I can only present the squashed patch for review. To get the smaller commits, do `git fetch git://github.com/encukou/freeipa.git replica-prepare:pviktori-replica-prepare`. Part of: https://fedorahosted.org/freeipa/ticket/2652 Fixes: https://fedorahosted.org/freeipa/ticket/3285 -- John Dennis jden...@redhat.com Looking to carve out IT costs? www.redhat.com/carveoutcosts/ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
