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

[Rob Crittenden]

Alexander Bokovoy wrote:
> On Fri, 12 Oct 2012, Petr Viktorin wrote:
>>>>>> 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.
>>>
>>>
>>
>> This works, however the test regexp doesn't do what it claims (doesn't
>> check if each string is on a separate line).
>> Checking the entire message would make the test more straightforward.
>> Squash in the attached patch if you agree.
> I purposedly went regexp way because of _("Additional instructions"). I
> know that our testsuite is not passing when running it localized so it
> may be a moot point but avoiding dependency on a localized content in
> the test was one specific reason for the approach.
>

I pushed these both to master and ipa-3-0 since the only controversy is 
in the way we are testing this. I wanted to get this fix in upstream.

rob