[Freeipa-devel] [PATCH] 0082/0083 Handle NotFound exception when establishing trust

[Petr Vobornik]

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 at 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 at 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.

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

-- 
Petr Vobornik
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-abbra-0082-support-multi-line-error-messages-in-exceptions-3.patch
Type: text/x-patch
Size: 4358 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20121008/da176561/attachment.bin>