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.
From 4c9b23de694da8e26e87ce5974b1a7061f6087da Mon Sep 17 00:00:00 2001
From: Martin Basti <mba...@redhat.com>
Date: Mon, 26 Oct 2015 17:56:57 +0100
Subject: [PATCH] Domain levels: use constants rather than hardcoded values

Added constants for domain levels
DOMAIN_LEVEL_0 = 0
DOMAIN_LEVEL_1 = 1

This allows to search for domain level easier in code.
---
 install/tools/ipa-ca-install               | 3 ++-
 install/tools/ipa-replica-manage           | 9 +++++----
 ipalib/constants.py                        | 8 ++++++--
 ipaserver/install/cainstance.py            | 2 +-
 ipaserver/install/dsinstance.py            | 2 +-
 ipaserver/install/ipa_kra_install.py       | 3 ++-
 ipaserver/install/ipa_replica_prepare.py   | 8 ++++----
 ipaserver/install/server/replicainstall.py | 8 ++++----
 8 files changed, 25 insertions(+), 18 deletions(-)

diff --git a/install/tools/ipa-ca-install b/install/tools/ipa-ca-install
index 9d3e9327319a6b133347b7d6f093b8eb9a149328..7f4c62176b4f1715f567d684a0d3ffd74d07576e 100755
--- a/install/tools/ipa-ca-install
+++ b/install/tools/ipa-ca-install
@@ -33,6 +33,7 @@ from ipaserver.install import cainstance, custodiainstance, service
 from ipapython import dogtag
 from ipapython import version
 from ipalib import api
+from ipalib.constants import DOMAIN_LEVEL_0
 from ipapython.dn import DN
 from ipapython.config import IPAOptionParser
 from ipapython.ipa_log_manager import *
@@ -108,7 +109,7 @@ def get_dirman_password():
 
 def install_replica(safe_options, options, filename):
     domain_level = dsinstance.get_domain_level(api)
-    if domain_level > 0:
+    if domain_level > DOMAIN_LEVEL_0:
         options.promote = True
     else:
         options.promote = False
diff --git a/install/tools/ipa-replica-manage b/install/tools/ipa-replica-manage
index b8337bb76cc66f56dda34d877d4400214c0aa90e..a1b3c87ba598c39532244113c91fbdcec944a215 100755
--- a/install/tools/ipa-replica-manage
+++ b/install/tools/ipa-replica-manage
@@ -37,7 +37,7 @@ from ipaserver.install import bindinstance, cainstance, certs
 from ipaserver.install import opendnssecinstance, dnskeysyncinstance
 from ipapython import version, ipaldap
 from ipalib import api, errors, util
-from ipalib.constants import CACERT
+from ipalib.constants import CACERT, DOMAIN_LEVEL_0
 from ipalib.util import create_topology_graph, get_topology_connection_errors
 from ipapython.ipa_log_manager import *
 from ipapython.dn import DN
@@ -809,7 +809,8 @@ def del_master_managed(realm, hostname, options):
 
 def del_master_direct(realm, hostname, options):
     """
-    Removing of master for realm without managed topology (domain level < 1)
+    Removing of master for realm without managed topology
+    (domain level < DOMAIN_LEVEL_1)
     """
 
     force_del = False
@@ -1354,8 +1355,8 @@ def set_DNA_range(hostname, range, realm, dirman_passwd, next_range=False,
             sys.exit("Updating range failed: %s" % e)
 
 def has_managed_topology():
-    domainlevel = api.Command['domainlevel_get']().get('result', 0)
-    return domainlevel > 0
+    domainlevel = api.Command['domainlevel_get']().get('result', DOMAIN_LEVEL_0)
+    return domainlevel > DOMAIN_LEVEL_0
 
 def exit_on_managed_topology(what):
     sys.exit("{0} is deprecated with managed IPA replication topology. "
diff --git a/ipalib/constants.py b/ipalib/constants.py
index b3642bc8575b7f770e1ef58c1bb8508833ecb7dc..fc0560ba4fe746f11e8ff3175508ace2e50c3187 100644
--- a/ipalib/constants.py
+++ b/ipalib/constants.py
@@ -234,8 +234,12 @@ LDAP_GENERALIZED_TIME_FORMAT = "%Y%m%d%H%M%SZ"
 IPA_ANCHOR_PREFIX = ':IPA:'
 SID_ANCHOR_PREFIX = ':SID:'
 
-MIN_DOMAIN_LEVEL = 0
-MAX_DOMAIN_LEVEL = 1
+# domains levels
+DOMAIN_LEVEL_0 = 0  # compat
+DOMAIN_LEVEL_1 = 1  # replica promotion, topology plugin
+
+MIN_DOMAIN_LEVEL = DOMAIN_LEVEL_0
+MAX_DOMAIN_LEVEL = DOMAIN_LEVEL_1
 
 # Constants used in generation of replication agreements and as topology
 # defaults
diff --git a/ipaserver/install/cainstance.py b/ipaserver/install/cainstance.py
index 22155cac335666889e090843af04a9449d16e68d..f9315f4f0b5fc83934424a5ed4c4fc577504b907 100644
--- a/ipaserver/install/cainstance.py
+++ b/ipaserver/install/cainstance.py
@@ -1530,7 +1530,7 @@ class CAInstance(DogtagInstance):
                           ca_type=None):
         """Creates a replica CA, creating a local DS backend and using
         the topology plugin to manage replication.
-        Requires domain_level >=1 and custodia on the master.
+        Requires domain_level >= DOMAIN_LEVEL_1 and custodia on the master.
         """
         self.ds_port = DEFAULT_DSPORT
         self.master_host = master_host
diff --git a/ipaserver/install/dsinstance.py b/ipaserver/install/dsinstance.py
index 2952836d63070f664ea061b6a4703546023457af..15b23a8704091fbcf54e5be52562f6da2eeb1d73 100644
--- a/ipaserver/install/dsinstance.py
+++ b/ipaserver/install/dsinstance.py
@@ -175,7 +175,7 @@ def get_domain_level(api=api):
     try:
         entry = conn.get_entry(dn, ['ipaDomainLevel'])
     except errors.NotFound:
-        return 0
+        return constants.DOMAIN_LEVEL_0
     return int(entry.single_value['ipaDomainLevel'])
 
 
diff --git a/ipaserver/install/ipa_kra_install.py b/ipaserver/install/ipa_kra_install.py
index 069c872782c12271568791dcc31a9067e3cd9cf1..1ae361edc3df3c572a5a8d6900ba5425300443c1 100755
--- a/ipaserver/install/ipa_kra_install.py
+++ b/ipaserver/install/ipa_kra_install.py
@@ -24,6 +24,7 @@ import tempfile
 
 from textwrap import dedent
 from ipalib import api
+from ipalib.constants import DOMAIN_LEVEL_0
 from ipaplatform import services
 from ipaplatform.paths import paths
 from ipapython import admintool
@@ -138,7 +139,7 @@ class KRAInstaller(KRAInstall):
 
         if self.installing_replica:
             domain_level = dsinstance.get_domain_level(api)
-            if domain_level > 0:
+            if domain_level > DOMAIN_LEVEL_0:
                 self.options.promote = True
                 return
 
diff --git a/ipaserver/install/ipa_replica_prepare.py b/ipaserver/install/ipa_replica_prepare.py
index c573428ed59147cbfe22944787726fc817284680..8998bc094e22ba9a95d528b09f73b2884d78462f 100644
--- a/ipaserver/install/ipa_replica_prepare.py
+++ b/ipaserver/install/ipa_replica_prepare.py
@@ -41,12 +41,12 @@ from ipapython import version
 from ipalib import api
 from ipalib import errors
 from ipaplatform.paths import paths
-from ipalib.constants import CACERT, MIN_DOMAIN_LEVEL
+from ipalib.constants import CACERT, DOMAIN_LEVEL_0
 
 
 UNSUPPORTED_DOMAIN_LEVEL_TEMPLATE = """
 Replica creation using '{command_name}' to generate replica file
-is supported only in {min_domain_level}-level IPA domain.
+is supported only in {domain_level}-level IPA domain.
 
 The current IPA domain level is {curr_domain_level} and thus the replica must
 be created by promoting an existing IPA client.
@@ -692,10 +692,10 @@ class ReplicaPrepare(admintool.AdminTool):
 
     def check_domainlevel(self, api):
         domain_level = dsinstance.get_domain_level(api)
-        if domain_level > MIN_DOMAIN_LEVEL:
+        if domain_level > DOMAIN_LEVEL_0:
             raise RuntimeError(
                 UNSUPPORTED_DOMAIN_LEVEL_TEMPLATE.format(
                     command_name=self.command_name,
-                    min_domain_level=MIN_DOMAIN_LEVEL,
+                    domain_level=DOMAIN_LEVEL_0,
                     curr_domain_level=domain_level)
             )
diff --git a/ipaserver/install/server/replicainstall.py b/ipaserver/install/server/replicainstall.py
index f07daead5cfc8ae8b6c4e2b69d3e9043feab1f69..b01df9526bb3a7dce7abca67f85fc54cd95f6218 100644
--- a/ipaserver/install/server/replicainstall.py
+++ b/ipaserver/install/server/replicainstall.py
@@ -530,9 +530,9 @@ def install_check(installer):
         except errors.NotFound:
             # If we're joining an older master, domain entry is not
             # available
-            current = 0
+            current = constants.DOMAIN_LEVEL_0
 
-        if current != 0:
+        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 "
@@ -858,9 +858,9 @@ def promote_check(installer):
         except errors.NotFound:
             # If we're joining an older master, domain entry is not
             # available
-            current = 0
+            current = constants.DOMAIN_LEVEL_0
 
-        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."
-- 
2.4.3

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