Petr Viktorin wrote:
On 03/30/2012 11:00 PM, Rob Crittenden wrote:
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,
SystemExit:
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/installutils.py
b/ipaserver/install/installutils.
py
index 851b58d..ca82a1b 100644
--- a/ipaserver/install/installutils.py
+++ b/ipaserver/install/installutils.py
@@ -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']:
- root_logger.info(template, operation_name)
+ root_logger.info(template)
try:
yield
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)
else:
- final_log('The %s command failed')
+ final_log('%s failed, %s: %s' % (operation_name, type(e).__name__,
e))
raise
else:
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.

rob

Fixed.


Hate to do this to you but I've found a few more issues. I basically went down the list and ran all the commands in various conditions.

Some don't open any logs at all so the output gets written twice, like ipa-replica-prepare and ipa-replica-manage:

# ipa-replica-manage del foo
Directory Manager password:

ipa: INFO: The ipa-replica-manage command failed, SystemExit: 'pony.greyoak.com' has no replication agreement for 'foo'
'pony.greyoak.com' has no replication agreement for 'foo'

Same with ipa-csreplica-manage.

# ipa-replica-prepare foo
Directory Manager (existing master) password:

ipa: INFO: The ipa-replica-prepare command failed, SystemExit:
The password provided is incorrect for LDAP server pony.greyoak.com

The password provided is incorrect for LDAP server pony.greyoak.com

rob

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to