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
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
