[Freeipa-devel] [PATCH] 0040 Move install script error handling to a common function

[Petr Viktorin]

On 05/29/2012 12:46 PM, Martin Kosek wrote:
> On Tue, 2012-05-22 at 15:45 +0200, Petr Viktorin wrote:
>> On 2012-04-23 17:05, John Dennis wrote:
>>> On 04/23/2012 05:19 AM, Petr Viktorin wrote:
>>>> This fixes https://fedorahosted.org/freeipa/ticket/2071 (Add final debug
>>>> message in installers).
>>>>
>>>> I submitted an earlier version of this patch before (0014), but it was
>>>> too much to include in 2.2. Hopefully now there's more space for
>>>> restructuring. I think it's better to start a new thread with this
>>>> approach.
>>>>
>>>> The try/except blocks at the end of installers/management scripts are
>>>> replaced by a call to a common function, which includes the final
>>>> message.
>>>> For each specific error, the error handlers in all scripts was almost
>>>> the same, but each script handled a different selection of errors.
>>>> Instead of having this copy/pasted code (with subtle differences
>>>> creeping in over time), this patch consolidates it all in one place.
>>>
>>> I like this approach much better than the earlier patch, great, thanks.
>>> I'm a big fan of calling into common code instead of copying code to my
>>> mind the refactoring to utilize common code is great approach. I also
>>> like the fact the logging configuration is not modified after it's
>>> established.
>>>
>>> At some point we may want to revist how the log messages are generated.
>>> For example should all communication to the console pass through the
>>> console handler? Is there a logger established for the script? Should
>>> the format of messages emitted to the console be altered? Should all
>>> command line utilities accept the both the verbose and debug flag? Etc.
>>> But for now this is fantastic start in the right direction.
>>>
>>> I have not installed and exercised the patch so I can't comment on any
>>> runtime time issues that might be present, but from code inspection only
>>> it has my ACK.
>>>
>>>
>> Thanks John!
>> Yes, this is just a start.
>>
>>
>> Patch rebased to curent master
>>
>
> The patch needs another rebase.
>
> Besides that, the approach is fine (and removes a lot of redundant code)
> but I found several issues with the patches:

Thanks for the review!

I was just looking into the issue. It missed that when logging is set up 
via api.bootstrap, by default the same logging level is set in both the 
log and console handlers. This makes it impossible to log only to the 
file -- INFO messages go to both file and console, and lower-level ones 
don't appear at al.
Fixing this will require changes to the logging setup of individual 
commands, and possibly the logging mechanism itself, which is more than 
I want to include in this patch.

Most scripts where this happens are not top-level installers (they're 
ipa-compliance, ipa-replica-conncheck, ipa-replica-manage, 
ipa-replica-prepare, ipa-ldap-updater). Since the ticket only asks for 
installers, I reverted the changes in these. Fixing them is left to 
ticket #2652.
So, issues 2-3 & 5-10 that you found are avoided.
I'll keep the cases around to test further work.


I changed ipa-server-certinstall to set up logging explicitly.


> 1) ipa-server-install: Fails when option parser error is raised:
>
> # ipa-server-install -p foo
> Usage: ipa-server-install [options]
>
> ipa-server-install: error: DS admin password: Password must be at least
> 8 characters long
> Traceback (most recent call last):
>    File "/sbin/ipa-server-install", line 1131, in<module>
>      if not success and installation_cleanup:
> NameError: name 'success' is not defined

Fixed

> 4) ipa-ca-install emits failed_message when option error is detected:
>
> # ipa-ca-install
> Usage: ipa-ca-install [options] REPLICA_FILE
>
> ipa-ca-install: error: you must provide a file generated by
> ipa-replica-prepare
>
>>>> Your system may be partly configured.
>>>> Run /usr/sbin/ipa-server-install --uninstall to clean up.

The attached patch skips printing the message on SystemExit, as it's 
done without the patch.

>
> [ipa-csreplica-manage] also has a wrong operation_name. This is what
happens when a
> script name is hard-coded and it is not detected automatically, e.g.
> from __file__.

I'll keep that in mind for #2652. Perhaps I'm trying too hard to avoid 
magic.

> Martin
>


-- 
Petr³
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0040-03-Move-install-script-error-handling-to-a-common-funct.patch
Type: text/x-patch
Size: 28899 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120530/13490963/attachment.bin>