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.
From 32b3e9cb274c478432a30c7c1ad7744fdcf67047 Mon Sep 17 00:00:00 2001
From: Martin Basti <mba...@redhat.com>
Date: Fri, 23 Oct 2015 12:33:56 +0200
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 | 15 ++++++++-------
 8 files changed, 29 insertions(+), 21 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..ea141e12f01db6349f031cf1acb41be95f648a60 100644
--- a/ipaserver/install/server/replicainstall.py
+++ b/ipaserver/install/server/replicainstall.py
@@ -530,14 +530,14 @@ 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 "
+                "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)
             )
 
         # Detect if current level is out of supported range
@@ -858,12 +858,13 @@ 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."
+                "create a replica when the domain is at level {dl}.".format(
+                    dl=constants.DOMAIN_LEVEL_0)
             )
 
         # Detect if current level is out of supported range
-- 
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