[Freeipa-devel] [PATCHES] 0117-0118 Port ipa-replica-prepare to the admintool framework

[John Dennis]

On 01/03/2013 08:00 AM, Petr Viktorin wrote:
> Hello,
>
> The first patch implements logging-related changes to the admintool
> framework and ipa-ldap-updater (the only thing ported to it so far).
> The design document is at http://freeipa.org/page/V3/Logging_and_output
>
> John, I decided to go ahead and put an explicit "logger" attribute on
> the tool class rather than adding debug, info, warn. etc methods
> dynamically using log_mgr.get_logger. I believe it's the cleanest solution.
> We had a discussion about this in this thread:
> https://www.redhat.com/archives/freeipa-devel/2012-July/msg00223.html; I
> didn't get a reaction to my conclusion so I'm letting you know in case
> you have more to say.

I'm fine with not directly adding the debug, info, warn etc. methods, 
that practice was historical dating back to the days of Jason. However I 
do think it's useful to use a named logger and not the global 
root_logger. I'd prefer we got away from using the root_logger, it's 
continued existence is historical as well and the idea was over time we 
would slowly eliminate it's usage. FWIW the log_mgr.get_logger() is 
still useful for what you want to do.

     def get_logger(self, who, bind_logger_names=False)

If you don't set bind_logger_names to True (and pass the class instance 
as who) you won't get the offensive debug, info, etc. methods added to 
the class instance. But it still does all the other bookeeping.

The 'who' in this instance could be either the name of the admin tool or 
the class instance.

Also I'd prefer using the attribute 'log' rather than 'logger'. That 
would make it consistent with code which does already use get_logger() 
passing a class instance because it's adds a 'log' attribute which is 
the logger. Also 'log' is twice as succinct than 'logger' (shorter line 
lengths).

Thus if you do:

   log_mgr.get_logger(self)

I think you'll get exactly what you want. A logger named for the class 
and being able to say

   self.log.debug()
   self.log.error()

inside the class.

In summary, just drop the True from the get_logger() call.

>
>
>
> The second patch ports ipa-replica-prepare to the framework. (I chose
> ipa-replica-prepare because there was a bug filed against its error
> handling, something the framework should take care of.)
> As far as Git can tell, it's a complete rewrite, so it might be hard to
> do a review. I have several smaller patches that make it easier to see
> what gets moved where. Please say I'm wrong, but as I understand, broken
> commits aren't allowed in the FreeIPA repo so I can only present the
> squashed patch for review.
> To get the smaller commits, do `git fetch
> git://github.com/encukou/freeipa.git
> replica-prepare:pviktori-replica-prepare`.
>
> Part of: https://fedorahosted.org/freeipa/ticket/2652
> Fixes: https://fedorahosted.org/freeipa/ticket/3285
>


-- 
John Dennis <jdennis at redhat.com>

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/