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

[Martin Kosek]

On Wed, 2012-05-30 at 13:04 +0200, Petr Viktorin wrote:
> 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
> >
> 
> 

Ok, lets handle the rest in #2652. The updated patch looks good, I did
not found any other blocking issue - ACK.

I rebased the patch and pushed to master.

Martin