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

[Petr Viktorin]

On 01/31/2013 04:54 PM, Rob Crittenden wrote:
> 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.

Fixed, thanks for the catch.

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

`ipa-ldap-updater /some/file` works for me. You just can't --upgrade or 
run the plugins as non-root (and as the help says, --plugins is implied 
when no files are given).
See http://www.redhat.com/archives/freeipa-devel/2012-June/msg00109.html

> 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.

I traced that to the dogtag module. Removing it in separate patch since 
it will affect other code (though I don't think it will cause trouble).

> 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.

Done, thanks.

-- 
Petr³

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0117-05-Better-logging-for-AdminTool-and-ipa-ldap-updater.patch
Type: text/x-patch
Size: 15394 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130201/8a955e4d/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0118-03-Port-ipa-replica-prepare-to-the-admintool-framework.patch
Type: text/x-patch
Size: 45265 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130201/8a955e4d/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0175-Make-ipapython.dogtag-log-requests-at-debug-level-no.patch
Type: text/x-patch
Size: 917 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130201/8a955e4d/attachment-0002.bin>