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.
> 

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to