[Freeipa-devel] [PATCH] 0014 Add final debug message in installers

[Rob Crittenden]

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