On Thu, 11 Oct 2012, Petr Viktorin wrote:
On 10/11/2012 02:44 PM, Alexander Bokovoy wrote:
On Thu, 11 Oct 2012, Petr Viktorin wrote:
On 10/11/2012 12:27 PM, Alexander Bokovoy wrote:
On Thu, 11 Oct 2012, Petr Viktorin wrote:
On 10/08/2012 02:22 PM, Alexander Bokovoy wrote:
On Mon, 08 Oct 2012, Petr Vobornik wrote:
On 10/05/2012 08:14 PM, Alexander Bokovoy wrote:
On Fri, 05 Oct 2012, Petr Vobornik wrote:
On 10/05/2012 03:24 PM, Alexander Bokovoy wrote:
On Fri, 05 Oct 2012, Petr Vobornik wrote:
On 10/04/2012 05:06 PM, Alexander Bokovoy wrote:
Hi,
two attached patches attempt to solve
https://fedorahosted.org/freeipa/ticket/3103
We cannot make educated guess where trusted domain's DNS
server is
located as we ended up with NotFound exception precisely
because we
were
unable to discover trusted domain's domain controller location.
Thus, all we can do is to suggest ways to fix the issue. Since
suggestion becomes relatively long (multi-line, at least), it
creates
few issues for current exception error message handling:
- root_logger does not use textui to format output
- textui is only printing to standard output
- multi-line error messages thus become non-wrapped
- localizing such messages would become a harder context-wise.
Web UI is showing error message as a single paragraph (<p/>),
all
multi-line markup would be lost.
In the end, I decided to support list-based multi-line error
messages in
PublicError class. Its constructor will join all list-based
arguments
into single string per argument (first patch).
In Web UI I've added special text processing of error messages
that
splits message into multiple lines and wraps those which start
with a
TAB symbol into 'error-message-hinted' span to visually
separate it
from
the rest of error message. Trust code uses this to display
suggested CLI
command to set up DNS forwarder (second patch).
In the end, CLI shows such error message fine (note tabulated
CLI
command):
-----------------------------------------------------------------------
# ipa trust-add --type=ad --admin Administrator@ad.local1
--password
ad.local1
Active directory domain administrator's password: ipa: ERROR:
Unable to
resolve domain controller for 'ad.local1' domain. IPA manages
DNS,
please configure forwarder to 'ad.local1' domain by using
following
CLI
command. Please replace DNS_SERVER and IP_ADDRESS by name and
address of
the domain's name server:
ipa dnszone-add ad.local1 --name-server=DNS_SERVER
--admin-email='hostmaster@ad.local1' --force
--forwarder=IP_ADDRESS
--forward-policy=only
When using Web UI, please create DNS zone for domain 'ad.local1'
first
and then set forwarder and forward policy
-----------------------------------------------------------------------
Web UI looks like this:
http://abbra.fedorapeople.org/.paste/ui.png
You have undeclared identifier: lines.
I recommend to run `jsl -conf jsl.conf` command in install/ui
folder
to prevent these issues.
Fixed.
I'm not convinced that "beautify" call should be in command
object. I
would rather see it in error_dialog.
I moved everything to error_dialog and used $() selectors
instead of
directly playing with text.
1)
+ var error_message = $('<span />', {});
I would rather see a <div /> as a container. Span is and should
contain only inline elements.
Fixed.
2)
var line_span = $('<span />', {
class: 'error-message-hinted',
text: lines[i].substr(1)
}).appendTo(container);
Why do you use <span> for hinted message and <p> for other lines?
Shouldn't we use <p> for both?
Fixed to use <p> everywhere.
3) var line_span is declared twice. JS use function scope, not
block
scope.
Fixed.
Thanks for the review. New patch 0082 attached.
ACK on the UI part with a little change (updated patch attached):
class: 'error-message-hinted',
needs to be changed to
'class': 'error-message-hinted',
because class is a keyword and should not be used this way. Sorry
that
I missed it before.
Thanks!
The patch works as advertised. I would give ACK to the python part of
82 and patch 83 as well but probably someone more skilled in python
and ipa internals should do it.
Why do you have to concatenate the value in PublicErrors' __init__
method? Shouldn't it be a responsibility of error source (in this
case
'execute_ad' method)? - just a question, not an issue
This is a good question and gives me some space to explain why certain
decisions are made.
The whole idea of the patch is to introduce a way to provide
multi-line
error messages as a feature of error handling in the FreeIPA
framework.
Suppose we had to join multiple lines always before creating an error
object. This means we would have hundreds of '\n'.join() calls across
the code. Besides maintenance burden, it would also make computational
burden later if we would add proper content formating (which we
don't do
now for errors) -- since we would need to split strings later to
reformat them.
Python is flexible enough to discover parameter types which
allows to design APIs that easer to use. "Easier to use" means least
surprise for the caller rather than callee. So, if you pass multiple
lines (list) of error message, each array element gets processed
separately before displaying.
So, in the end there is only one place where this processing happens,
and it encapsulates all the knowledge required to accept multi-lines
messages, regardless how they come, since it is applied uniformly to
every
argument of a constructor of a PublicError-derived class.
Some of our errors already accept lists. From our doctest suite:
File "/home/pviktori/freeipa/ipalib/errors.py", line 731, in
ipalib.errors.OverlapError
Failed example:
raise OverlapError(names=['givenname', 'login'])
Expected:
Traceback (most recent call last):
...
OverlapError: overlapping arguments and options: ['givenname',
'login']
Got:
Traceback (most recent call last):
File "/usr/lib64/python2.7/doctest.py", line 1289, in __run
compileflags, 1) in test.globs
File "<doctest ipalib.errors.OverlapError[0]>", line 1, in <module>
raise OverlapError(names=['givenname', 'login'])
OverlapError: overlapping arguments and options: u'givenname\nlogin'
In this case, ['givenname', 'login'] is much better than
u'givenname\nlogin' (or givenname<newline>login, if we switched the
message from %r to %s).
I don't think what's best for trust-add's NotFound is the best thing
to do everywhere.
Sure. Could you make a ticket so that I can investigate it more and find
appropriate way to handle this? Perhaps an error class could define how
it wants to treat its arguments?
Sure. Please share findings of your investigation, then.
https://fedorahosted.org/freeipa/ticket/3167
Basically, I think you generalized too much. The result is much more
complex (=less understandable, maintainable, changeable) than a
'\n'.join() when creating the error.
With the following patch I have this behavior:
from ipalib import errors
raise errors.PublicError(lines=['foo','bar'], format="You've got a
message: %(lines)s")
Traceback (most recent call last):
File "<console>", line 1, in <module>
PublicError: You've got a message: foo
bar
raise errors.OverlapError(names=['foo','bar'])
Traceback (most recent call last):
File "<console>", line 1, in <module>
OverlapError: overlapping arguments and options: ['foo', 'bar']
The new patch adds even more complexity. Adding the conversions only
to error classes that need them (if any), as opposed to PublicError
with an option to turn them off, would be much more straightforward.
After discussing more with Petr on irc, I came up with the following
patch: allow `instructions' additional parameter to PublicError() to
add instructions to an error message.
Trust's error message is converted to show its use.
The rationale:
The need for multi-line error messages was prompted by requests that
IPA should either fix/prevent errors itself, or print out more
detailed instructions for the user.
Automatic joining of lists is problematic, because it repurposes a
standard datatype for the needs of a fairly specific use case.
Converting individual arguments that need it is better than a blanket
approach.
An extra argument for instructions allows us to separate the error
message and instructions in the future, if we need to e.g. add more
complex formatting than the \t.
Patch comments:
This needs some tests to verify it's right and prevent regressions.
Please rename the convert_keyword function to e.g. format_instructions.
It's not necessary to add the instructions to self.msg, that one is
not user-friendly.
Please wrap the long line in errors.py.
Updated the patch with new test case and fixes. Also split out trust.py
changes to a separate patch.
Both attached.
--
/ Alexander Bokovoy
>From 317f59aab403f17ef280832ba8202d63250f5c18 Mon Sep 17 00:00:00 2001
From: Alexander Bokovoy <aboko...@redhat.com>
Date: Fri, 12 Oct 2012 12:13:59 +0300
Subject: [PATCH 7/8] Add instructions support to PublicError
When long additional text should follow the error message, one can
supply instructions parameter to a class derived from PublicError.
This will cause following text added to the error message:
Additional instructions:
<additional text>
`instructions' optional parameter could be a list or anything that coerces
into unicode(). List entries will be joined with '\n'.
https://fedorahosted.org/freeipa/ticket/3167
---
ipalib/errors.py | 21 ++++++++++++---------
tests/test_ipalib/test_errors.py | 17 +++++++++++++++++
2 files changed, 29 insertions(+), 9 deletions(-)
diff --git a/ipalib/errors.py b/ipalib/errors.py
index
7f1113200b6b2cd80ea9156d347211e4d978b9be..a6c8e76836c0b4c9809f891a423abaf51d057108
100644
--- a/ipalib/errors.py
+++ b/ipalib/errors.py
@@ -265,17 +265,20 @@ class PublicError(StandardError):
)
self.format = format
self.forwarded = False
- def convert_keyword(value):
- if isinstance(value, list):
- result=u'\n'.join(map(lambda line: unicode(line), value))
- return result
- return value
- kwargs = dict(map(lambda (k,v): (k, convert_keyword(v)),
kw.items()))
- self.msg = self.format % kwargs
+ self.msg = self.format % kw
if isinstance(self.format, basestring):
- self.strerror = ugettext(self.format) % kwargs
+ self.strerror = ugettext(self.format) % kw
else:
- self.strerror = self.format % kwargs
+ self.strerror = self.format % kw
+ if 'instructions' in kw:
+ def convert_instructions(value):
+ if isinstance(value, list):
+ result=u'\n'.join(map(lambda line: unicode(line),
value))
+ return result
+ return value
+ instructions = u'\n'.join((unicode(_('Additional
instructions:')),
+
convert_instructions(kw['instructions'])))
+ self.strerror = u'\n'.join((self.strerror, instructions))
else:
if isinstance(message, (Gettext, NGettext)):
message = unicode(message)
diff --git a/tests/test_ipalib/test_errors.py b/tests/test_ipalib/test_errors.py
index
2fcdc56c804891b89870adc8fb7eeaed9a686803..1421e7844a7068605568bccf78715669dfe0d8ec
100644
--- a/tests/test_ipalib/test_errors.py
+++ b/tests/test_ipalib/test_errors.py
@@ -318,6 +318,23 @@ class test_PublicError(PublicExceptionTester):
assert inst.text is kw['text']
assert inst.number is kw['number']
+ # Test with instructions:
+ # first build up "instructions", then get error and search for
+ # lines of instructions appended to the end of the strerror
+ # despite the parameter 'instructions' not existing in the format
+ instructions = u"The quick brown fox jumps over the lazy dog".split()
+ # this expression checks if each word of instructions
+ # exists in a string as a separate line, with right order
+ regexp = re.compile('(?ims).*' +
+ ''.join(map(lambda x: '(%s).*' % (x),
+ instructions)) +
+ '$')
+ inst = subclass(instructions=instructions, **kw)
+ assert inst.format is subclass.format
+ assert_equal(inst.instructions, instructions)
+ inst_match = regexp.match(inst.strerror).groups()
+ assert_equal(list(inst_match),list(instructions))
+
def test_public_errors():
"""
--
1.7.12
>From d386180e7d60cef609e5c9d85b769de6425a4614 Mon Sep 17 00:00:00 2001
From: Alexander Bokovoy <aboko...@redhat.com>
Date: Fri, 12 Oct 2012 12:14:24 +0300
Subject: [PATCH 8/8] Use PublicError instructions support for trust-add case
when domain is not found
https://fedorahosted.org/freeipa/ticket/3167
---
ipalib/plugins/trust.py | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/ipalib/plugins/trust.py b/ipalib/plugins/trust.py
index
8632d42df578d81b6fdbcd9e5be8979994699206..9fc6c5ad4638fa237632876207cc85c64ade6503
100644
--- a/ipalib/plugins/trust.py
+++ b/ipalib/plugins/trust.py
@@ -346,32 +346,33 @@ class trust_add(LDAPCreate):
try:
result = trustinstance.join_ad_full_credentials(keys[-1],
realm_server, realm_admin, realm_passwd)
except errors.NotFound, e:
- error_message=[_("Unable to resolve domain controller for '%s'
domain. ") % (keys[-1])]
+ error_message=_("Unable to resolve domain controller for '%s'
domain. ") % (keys[-1])
+ instructions=[]
if dns_container_exists(self.obj.backend):
try:
dns_zone = api.Command.dnszone_show(keys[-1])['result']
if ('idnsforwardpolicy' in dns_zone) and
dns_zone['idnsforwardpolicy'][0] == u'only':
- error_message.append(_("Forward policy is defined
for it in IPA DNS, "
+ instructions.append(_("Forward policy is defined
for it in IPA DNS, "
"perhaps forwarder points
to incorrect host?"))
except (errors.NotFound, KeyError) as e:
- error_message.append(_("IPA manages DNS, please
configure forwarder to "
+ instructions.append(_("IPA manages DNS, please
configure forwarder to "
"'%(domain)s' domain using
following CLI command. "
"Make sure to replace
DNS_SERVER and IP_ADDRESS by "
"actual values corresponding to
the trusted domain's "
"DNS server:") %
dict(domain=keys[-1]))
# tab character at the beginning of a multiline error
message will be replaced
# in the web UI by a colorful hint. Does not affect
CLI.
- error_message.append(_("\tipa dnszone-add %(domain)s
--name-server=[DNS_SERVER] "
+ instructions.append(_("\tipa dnszone-add %(domain)s
--name-server=[DNS_SERVER] "
"--admin-email='hostmaster@%(domain)s' "
"--force
--forwarder=[IP_ADDRESS] "
"--forward-policy=only") %
dict(domain=keys[-1]))
- error_message.append(_("When using Web UI, please
create DNS zone for domain '%(domain)s' "
+ instructions.append(_("When using Web UI, please
create DNS zone for domain '%(domain)s' "
"first and then set forwarder
and forward policy.") % dict(domain=keys[-1]))
else:
- error_message.append(_("Since IPA does not manage DNS
records, ensure DNS "
+ instructions.append(_("Since IPA does not manage DNS
records, ensure DNS "
"is configured to resolve
'%(domain)s' domain from "
"IPA hosts and back.") %
dict(domain=keys[-1]))
- raise errors.NotFound(reason=error_message)
+ raise errors.NotFound(reason=error_message,
instructions=instructions)
if result is None:
raise errors.ValidationError(name=_('AD Trust setup'),
--
1.7.12
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
