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

[Petr Viktorin]

On 04/10/2012 10:41 PM, Rob Crittenden wrote:
> 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

When the utility sets logging to console, the extra log message gets 
printed out there. I agree this isn't optimal.
Attached patch removes the console log handler before logging the result.

-- 
Petr³
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0014-08-Add-final-debug-message-in-installers.patch
Type: text/x-patch
Size: 30351 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120412/203d7752/attachment.bin>