Petr Viktorin wrote:
On 03/26/2012 05:35 PM, Petr Viktorin wrote:
On 03/26/2012 04:54 PM, Rob Crittenden wrote:

Some minor compliants.

Ideally, there would be a routine that sets up the logging and handles
command-line arguments in some uniform way (which is also needed before
logging setup to detect ipa-server-install --uninstall).
The original patch did the common logging setup, and I hacked around the
install/uninstall problem too.
I guess I overdid it when I simplified the patch.
I'm somewhat confused about the scope, so bear with me as I clarify what
you mean.

If you abort the installation you get this somewhat unnerving error:

Continue to configure the system with these values? [no]:
ipa : ERROR ipa-server-install failed, SystemExit: Installation aborted
Installation aborted

ipa-ldap-updater is the same:

# ipa-ldap-updater
[2012-03-26T14:53:41Z ipa] <ERROR>: ipa-ldap-updater failed, SystemExit:
IPA is not configured on this system.
IPA is not configured on this system.

and ipa-upgradeconfig

$ ipa-upgradeconfig
[2012-03-26T14:54:05Z ipa] <ERROR>: ipa-upgradeconfig failed,
You must be root to run this script.

You must be root to run this script.

I'm guessing that the issue is that the log file isn't opened yet.
It would be nice if the logging would be confined to just the log.

If I understand you correctly, the code should check if logging has been
configured already, and if not, skip displaying the message?

When uninstalling you get the message 'ipa-server-install successful'.
This is a little odd as well.

ipa-server-install is the name of the command. Wontfix for now, unless
you disagree strongly.

Updated patch: only log if logging has been configured (detected by
looking at the root logger's handlers), and changed the message to “The
ipa-server-install command has succeeded/failed”.

Works much better thanks. Just one request. When you created final_log() you show less information than you did in earlier patches. It is nice seeing the SystemExit failure. Can you do something like this (basically cut-n-pasted from v05)?

diff --git a/ipaserver/install/ b/ipaserver/install/installutils.
index 851b58d..ca82a1b 100644
--- a/ipaserver/install/
+++ b/ipaserver/install/
@@ -721,15 +721,15 @@ def script_context(operation_name):
         # Only log if logging was already configured
# TODO: Do this properly (e.g. configure logging before the try/except)
         if log_mgr.handlers.keys() != ['console']:
-  , operation_name)
     except BaseException, e:
         if isinstance(e, SystemExit) and (e.code is None or e.code == 0):
             # Not an error after all
-            final_log('The %s command was successful')
+            final_log('The %s command was successful' % operation_name)
-            final_log('The %s command failed')
+ final_log('%s failed, %s: %s' % (operation_name, type(e).__name__,
         final_log('The %s command was successful')

This looks like:

2012-03-30T20:56:53Z INFO ipa-dns-install failed, SystemExit:
DNS is already configured in this IPA server.


Freeipa-devel mailing list

Reply via email to