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

[Martin Babinsky]

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.

-- 
Martin^3 Babinsky
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbabinsk-0092.1-ipa-replica-prepare-domain-level-check-improvements.patch
Type: text/x-patch
Size: 2466 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151029/9efb75ac/attachment.bin>