[Freeipa-devel] [PATCH 0335] Freeipa domain levels naming

[Martin Basti]

On 23.10.2015 15:36, Tomas Babej wrote:
>
> On 10/23/2015 02:07 PM, Petr Spacek wrote:
>> On 23.10.2015 13:53, Martin Basti wrote:
>>>
>>> On 23.10.2015 13:53, Tomas Babej wrote:
>>>> On 10/23/2015 01:51 PM, Martin Basti wrote:
>>>>> On 23.10.2015 13:49, Tomas Babej wrote:
>>>>>> On 10/23/2015 12:49 PM, Martin Basti wrote:
>>>>>>> On 23.10.2015 09:34, Martin Basti wrote:
>>>>>>>> On 23.10.2015 09:31, Tomas Babej wrote:
>>>>>>>>> On 10/22/2015 05:49 PM, Simo Sorce wrote:
>>>>>>>>>> On 22/10/15 11:29, Martin Basti wrote:
>>>>>>>>>>> Hello all,
>>>>>>>>>>>
>>>>>>>>>>> in current master branch we have mixed usage of literals 0, 1 and
>>>>>>>>>>> constants MIN_DOMAIN_LEVEL, MAX_DOMAIN_LEVEL, and it is quite mess.
>>>>>>>>>>>
>>>>>>>>>>> I suggest to use names for domain levels:
>>>>>>>>>>>
>>>>>>>>>>> COMPAT_DOMAIN_LEVEL = 0
>>>>>>>>>>> PROMOTION_DOMAIN_LEVEL = 1
>>>>>>>>>>> UBER_NEW_FEATURE_DOMAIN_LEVEL = 2
>>>>>>>>>>>
>>>>>>>>>>> MIN_DOMAIN_LEVEL = COMPAT_DOMAIN_LEVEL (=0)
>>>>>>>>>>> MAX_DOMAIN_LEVEL = UBER_NEW_FEATURE_DOMAIN_LEVEL (=2)
>>>>>>>>>>>
>>>>>>>>>>> Benefits:
>>>>>>>>>>> * ability to grep it in code
>>>>>>>>>> Call them DOMAIN_LEVEL_0 and DOMAIN_LEVEL_1
>>>>>>>>>>
>>>>>>>>>>> * better readability in code
>>>>>>>>>> Sure, but random names are not appropriate imo
>>>>>>>>> I'm with you guys on this, it's a good idea. Let's go with the
>>>>>>>>> DOMAIN_LEVEL_X naming though, it will be probably easier to remember.
>>>>>>>>>
>>>>>>>>> One thing to add to the discussion - MIN/MAX_DOMAIN_LEVEL denotes only
>>>>>>>>> the minimal/maximal domain level supported by the given IPA server,
>>>>>>>>> not
>>>>>>>>> the minimal/maximal domain level ever shipped by FreeIPA project.
>>>>>>>>>
>>>>>>>>> Currently, those two coincide, but in general they might be
>>>>>>>>> different if
>>>>>>>>> we ever raise the minimal level a decide to deprecate, say, domain
>>>>>>>>> level
>>>>>>>>> 0 or 1. It's a subtle but important difference.
>>>>>>>>>
>>>>>>>>> Tomas
>>>>>>>> Thank you all for your opinion,
>>>>>>>>
>>>>>>>> I will implement DOMAIN_LEVEL_X constants and send patch.
>>>>>>>>
>>>>>>>> Thanks.
>>>>>>>> Martin^2
>>>>>>>>
>>>>>>> Patch attached.
>>>>>>> O hope, I did not miss anything hardcoded.
>>>>>> I think we can safely hardcode domain levels as numbers in the error
>>>>>> messages. Substituting {dl} for constants.DOMAIN_LEVEL_0 in the error
>>>>>> message makes little sense to me, as the value of
>>>>>> constants.DOMAIN_LEVEL_0 will not ever change.
>>>>> However, with substituting is easier to find occurrences of domain
>>>>> levels in comments and messages in case of refactoring.
>>>> In case of refactoring of what? All the error messages containing
>>>> reference to a domain level 0? :)
>>> Exactly! :)
>> Tomas, that one day we decide to drop support for domain level 0. We will have
>> to hunt down all references to it and using constant for help messages etc.
>> might be handy because it will show up in results of grep, so we do not forget
>> to wipe domain level 0 from texts.
>>
> I understand all that, but those error messages are in the conditional
> blocks that already include greppable references to the DOMAIN_LEVEL_0:
>
> So, to continue this bikeshedding of the highest possible level :)
>
> <bikeshed>
>
> +if current != constants.DOMAIN_LEVEL_0:
>        raise RuntimeError(
>           "You cannot use a replica file to join a replica when the "
>            "domain is above level 0. Please join the system to the "
>            "domain is above level {dl}. Please join the system to the "
>            "domain by running ipa-client-install first, the try again "
> -         "without a replica file."
> +         "without a replica file.".format(dl=constants.DOMAIN_LEVEL_0)
>        )
>
> and
>
> -if current == 0:
> +if current == constants.DOMAIN_LEVEL_0:
>       raise RuntimeError(
>           "You must provide a file generated by ipa-replica-prepare to "
> -        "create a replica when the domain is at level 0."
> +        "create a replica when the domain is at level {dl}.".format(
> +            dl=constants.DOMAIN_LEVEL_0)
>        )
>
> I guess we would remove the whole blocks in these cases, hence
> formatters are needless complication of the code here.
>
> </bikeshed>
>
>> Nitpick just to make you feel that I read the patch (does not warrant a NACK):
>> I would rather see conditions in form of (<= or >=) instead of (< or >)
>> because now you have to grep for domain level +- 1 to get all references to
>> particular domain level :-D
>>
>> Nevermind, this is just a nitpick.
>>

Patch with removed changes in error messages is attached.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0335.2-Domain-levels-use-constants-rather-than-hardcoded-va.patch
Type: text/x-patch
Size: 8746 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151026/e801ffd6/attachment.bin>