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

[Petr Viktorin]

On 04/12/2012 01:30 PM, Petr Viktorin wrote:
> 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.
>

I read through log_manager, and found that I can do this more cleanly 
with remove_handler.

John, is this a good use of the API?

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