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

[Tomas Babej]

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