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

[Rob Crittenden]

Petr Viktorin wrote:
> On 01/28/2013 04:36 PM, Petr Viktorin wrote:
>> On 01/04/2013 02:43 PM, Petr Viktorin wrote:
>>> On 01/03/2013 02:56 PM, John Dennis wrote:
>>>> 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.
>>>>
>>>
>>> Thanks! Yes, this works better. Updated patches attached.
>>>
>>
>>
>> Here is patch 117 rebased to current master.
>>
>
> Rebased again.

Just a few minor points.

Patch 117:

The n-v-r should be -14.

ipa-ldap-updater is no longer runable as non-root. Was this intentional?

Patch 118:

Seems to work as it did though as a side effect of the new logging some 
things are displayed that we may want to suppress, specifically:

request 'https://dart.example.com:8443/ca/ee/ca/profileSubmitSSLClient'

I think changing the log level to DEBUG is probably the way to go.

While you're at it you might consider replacing the ipa_replica_prepare 
remove_file() with the one in installutils. They differ slightly in 
implementation but basically do the same thing.

rob