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

[Martin Basti]

On 22.10.2015 14:29, Petr Spacek wrote:
> 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.
>
I acked and pushed this patch 2 days ago, and probably my email has been 
lost forever, so I did bad review, please sent fix as new patch :(

Original patch pushed d81260ef60b64c312e3a164e90ac4faad75c5d82