[Freeipa-devel] [PATCH 0086] disable ipa-replica prepare in non-zero domain levels

[Tomas Babej]

On 10/22/2015 02:15 PM, Petr Spacek wrote:
> On 20.10.2015 17:47, Martin Babinsky wrote:
>> +    def check_domainlevel(self, api):
>> +        domain_level = dsinstance.get_domain_level(api)
>> +        if domain_level > MIN_DOMAIN_LEVEL:
>> +            raise RuntimeError(
>> +                UNSUPPORTED_DOMAIN_LEVEL_TEMPLATE.format(
>> +                    command_name=self.command_name,
>> +                    min_domain_level=MIN_DOMAIN_LEVEL,
>> +                    curr_domain_level=domain_level)
>> +            )
> 
> NACK.
> 
> This is very very weird function because it compares two values which are not
> passed as parameters, and also the parameter "api" seems to be unused.
> 
> At very least a explanatory doc string is needed, but a new name might be even
> better.
> 
> Check domain level of what against what? It would be great if function name
> could answer this question.
> 

Also note we have a dedicated exception InvalidDomainLevelError which
should be used in such situations.

Additionally, I'm not sure if putting this huge blob of text (with
instructions) into the exception is the best way forward, imho we can
either document it somewhere else ('ipa help something?' wiki?) and
reference it here.

Alternatively, we can just use a logger to log these instructions
instead of passing them in the exception itself.

HTH,

Tomas