[Freeipa-devel] [PATCH 0092] ipa-replica-prepare: more robust and concise check for supported domain level

[Petr Vobornik]

On 10/29/2015 03:01 PM, Martin Babinsky wrote:
> On 10/29/2015 01:28 PM, Tomas Babej wrote:
>>
>>
>> On 10/29/2015 01:10 PM, Petr Vobornik wrote:
>>> On 10/29/2015 11:19 AM, Martin Babinsky wrote:
>>>> Petr^2 and Tomas were not happy by the way
>>>> https://fedorahosted.org/freeipa/ticket/5175 was handled initially, so
>>>> here is a patch that tries to amend some of the issues.
>>>>
>>>>
>>>>
>>>
>>> IMHO the original text was good.
>>>
>>> Tomas, why is the huge blob thing in exception bad?
>>>
>>> Issues with new code/text.
>>>
>>> 1. it is supported but not for domain level != 0:
>>> self.log.error("Using replica files to set up IPA replicas is not "
>>> +                           "supported."
>>> +            )
>>>
>>> 2. format() is not needed:
>>> +            self.log.info(
>>> +                "To create a replica, you must promote an existing "
>>> +                "IPA client.".format(domain_level=domain_level)
>>> +            )
>>>
>>> 3. I don't like that the exception text says 'requires', which might
>>> imply something to be done - lower domain level - which is not possible.
>>> "allowed only" might be better.
>>>
>>>
>>> Just changing RuntimeError to InvalidDomainLevelError would be fine with
>>> me since the MIN_DOMAIN_LEVEL was already changed to DOMAIN_LEVEL_0.
>>
>>
>> I was fine with the old text too. In my opinion exceptions should not be
>> used to handle detailed instructions to the user, they should be used to
>> briefly summarize the gist of the error that occurred.
>>
>> If not bad practice, it's at least quite unconventional. There are
>> better mechanisms to handle the detailed instructions spanning over
>> multiple lines (with newlines hardcoded) down to the user, such as using
>> the logger with sufficient level.
>>
>> So I would approach this issue by just dumping the formatted
>> UNSUPPORTED_DOMAIN_LEVEL_TEMPLATE down the logger at the error level and
>> then raising the InvalidDomainLevelError exception with the brief
>> reasoning.
>>
>> Tomas
>>
> OK i seemed to misunderstand your concerns. Attaching updated patch.
>

ACK

Pushed to master: 4d94367006287ed0a04c092a7b86096518cf5b8c
-- 
Petr Vobornik