On 10/29/2015 01:28 PM, Tomas Babej wrote:


On 10/29/2015 01:10 PM, Petr Vobornik wrote:
On 10/29/2015 11:19 AM, Martin Babinsky wrote:
Petr^2 and Tomas were not happy by the way
https://fedorahosted.org/freeipa/ticket/5175 was handled initially, so
here is a patch that tries to amend some of the issues.




IMHO the original text was good.

Tomas, why is the huge blob thing in exception bad?

Issues with new code/text.

1. it is supported but not for domain level != 0:
self.log.error("Using replica files to set up IPA replicas is not "
+                           "supported."
+            )

2. format() is not needed:
+            self.log.info(
+                "To create a replica, you must promote an existing "
+                "IPA client.".format(domain_level=domain_level)
+            )

3. I don't like that the exception text says 'requires', which might
imply something to be done - lower domain level - which is not possible.
"allowed only" might be better.


Just changing RuntimeError to InvalidDomainLevelError would be fine with
me since the MIN_DOMAIN_LEVEL was already changed to DOMAIN_LEVEL_0.


I was fine with the old text too. In my opinion exceptions should not be
used to handle detailed instructions to the user, they should be used to
briefly summarize the gist of the error that occurred.

If not bad practice, it's at least quite unconventional. There are
better mechanisms to handle the detailed instructions spanning over
multiple lines (with newlines hardcoded) down to the user, such as using
the logger with sufficient level.

So I would approach this issue by just dumping the formatted
UNSUPPORTED_DOMAIN_LEVEL_TEMPLATE down the logger at the error level and
then raising the InvalidDomainLevelError exception with the brief reasoning.

Tomas

OK i seemed to misunderstand your concerns. Attaching updated patch.

--
Martin^3 Babinsky
From d6ac6712f78daeeb0a2b1a6eea518692d23ae9e9 Mon Sep 17 00:00:00 2001
From: Martin Babinsky <mbabi...@redhat.com>
Date: Thu, 29 Oct 2015 14:53:25 +0100
Subject: [PATCH] ipa-replica-prepare: domain level check improvements

ipa-replica-prepare command is disabled in non-zero domain-level. Instead of
raising and exception with the whole message instructing the user to promote
replicas from enrolled clients in level 1+ topologies, the exception itself
contains only a brief informative message and the rest is logged at error
level.

https://fedorahosted.org/freeipa/ticket/5175
---
 ipaserver/install/ipa_replica_prepare.py | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/ipaserver/install/ipa_replica_prepare.py b/ipaserver/install/ipa_replica_prepare.py
index 8998bc094e22ba9a95d528b09f73b2884d78462f..327deed772561f2b8403aa46cdd3398055703840 100644
--- a/ipaserver/install/ipa_replica_prepare.py
+++ b/ipaserver/install/ipa_replica_prepare.py
@@ -175,7 +175,7 @@ class ReplicaPrepare(admintool.AdminTool):
         api.bootstrap(in_server=True)
         api.finalize()
 
-        self.check_domainlevel(api)
+        self.check_for_supported_domain_level()
 
         if api.env.host == self.replica_fqdn:
             raise admintool.ScriptError("You can't create a replica on itself")
@@ -690,12 +690,25 @@ class ReplicaPrepare(admintool.AdminTool):
             '-o', ca_file
         ])
 
-    def check_domainlevel(self, api):
+    def check_for_supported_domain_level(self):
+        """
+        check if we are in 0-level topology. If not, raise an error pointing
+        the user to the replica promotion pathway
+        """
+
         domain_level = dsinstance.get_domain_level(api)
         if domain_level > DOMAIN_LEVEL_0:
-            raise RuntimeError(
+            self.log.error(
                 UNSUPPORTED_DOMAIN_LEVEL_TEMPLATE.format(
                     command_name=self.command_name,
                     domain_level=DOMAIN_LEVEL_0,
-                    curr_domain_level=domain_level)
+                    curr_domain_level=domain_level
+                )
+            )
+            raise errors.InvalidDomainLevelError(
+                reason="'{command}' is allowed only in domain level "
+                "{prep_domain_level}".format(
+                    command=self.command_name,
+                    prep_domain_level=DOMAIN_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