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

[Petr Spacek]

On 22.10.2015 14:24, Tomas Babej wrote:
> 
> 
> 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.

One more point:

+        if domain_level > MIN_DOMAIN_LEVEL:
+            raise RuntimeError(
+                UNSUPPORTED_DOMAIN_LEVEL_TEMPLATE.format(

It is kind of weird that error happens if domain level is greater than some
minimal value. Better naming is badly needed.

-- 
Petr^2 Spacek