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

[Rob Crittenden]

Petr Viktorin wrote:
> 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

Yeah, I remember the agreed-on behavior. I just re-tested and it works 
fine, so I must've been having a really bad day yesterday.

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

Ok with me.


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


ACK. Pushed all three to master.

rob