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

2013-02-01 Thread 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

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

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

2013-01-31 Thread 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

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

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

2013-01-30 Thread Petr Viktorin 

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.


--
Petr³
From 00afb162ead46f0e248aa78f4466d6eeb44ad890 Mon Sep 17 00:00:00 2001
From: Petr Viktorin 
Date: Tue, 18 Dec 2012 06:24:04 -0500
Subject: [PATCH] Better logging for AdminTool and ipa-ldap-updater

- Automatically add a "Logging and output options" group with the --quiet,
--verbose, --log-file options.
- Set up logging based on these options; details are in the setup_logging
docstring and in the design document.
- Don't bind log methods as individual methods of the class. This means one
less linter exception.
- Make the help for command line options consistent with optparse's --help and
--version options.

Design document: http://freeipa.org/page/V3/Logging_and_output
---
 freeipa.spec.in   |5 +-
 ipapython/admintool.py|  120 
 ipaserver/install/ipa_ldap_updater.py |   48 ++---
 make-lint |1 -
 4 files changed, 116 insertions(+), 58 deletions(-)

diff --git a/freeipa.spec.in b/freeipa.spec.in
index d875183e601cb5c464b650879c7faa7d7b872559..ffee7146ce0b22ec840e7e1e957bf9de6cd2e567 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -438,7 +438,7 @@ fi
 %posttrans server
 # This must be run in posttrans so that updates from previous
 # execution that may no longer be shipped are not applied.
-/usr/sbin/ipa-ldap-updater --upgrade >/dev/null || :
+/usr/sbin/ipa-ldap-updater --upgrade --quiet || :
 
 %preun server
 if [ $1 = 0 ]; then
@@ -769,6 +769,9 @@ fi
 %ghost %attr(0644,root,apache) %config(noreplace) %{_sysconfdir}/ipa/ca.crt
 
 %changelog
+* Tue Jan 29 2013 Petr Viktorin  - 3.0.99-13
+- Use ipa-ldap-updater --quiet instead of redirecting to /dev/null
+
 * Tue Jan 29 2013 Rob Crittenden  - 3.0.99-13
 - Set certmonger minimum version to 0.65 for NSS locking during
   renewal
diff --git a/ipapython/admintool.py b/ipapython/admintool.py
index b644516dd2afd0d373734767feee4d65b3fbfd5b..5bc21516562ac2a8c6790ea3421628a4c7db9797 100644
--- a/ipapython/admintool.py
+++ b/ipapython/admintool.py
@@ -76,14 +76,15 @@ class AdminTool(object):
 Class attributes to define in subclasses:
 command_name - shown in logs
 log_file_name - if None, logging is to stderr only
-needs_root - if true, non-root users can't run the tool
 usage - text shown in help
+
+See the setup_logging method for more info on logging.
 """
 command_name = None
 log_file_name = None
-needs_root = False
 usage = None
 
+log = None
 _option_parsers = dict()
 
 @classmethod
@@ -95,10 +96,23 @@ class AdminTool(object):
 cls.add_options(parser)
 
 @classmethod
-def add_o

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

2013-01-28 Thread Petr Viktorin 

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.

--
Petr³
From 764e2c2f295f056570cf30ce2553024e5d5e2a1a Mon Sep 17 00:00:00 2001
From: Petr Viktorin 
Date: Tue, 18 Dec 2012 06:24:04 -0500
Subject: [PATCH] Better logging for AdminTool and ipa-ldap-updater

- Automatically add a "Logging and output options" group with the --quiet,
--verbose, --log-file options.
- Set up logging based on these options; details are in the setup_logging
docstring and in the design document.
- Don't bind log methods as individual methods of the class. This means one
less linter exception.
- Make the help for command line options consistent with optparse's --help and
--version options.

Design document: http://freeipa.org/page/V3/Logging_and_output
---
 freeipa.spec.in   |5 +-
 ipapython/admintool.py|  120 
 ipaserver/install/ipa_ldap_updater.py |   48 ++---
 make-lint |1 -
 4 files changed, 116 insertions(+), 58 deletions(-)

diff --git a/freeipa.spec.in b/freeipa.spec.in
index 189c7b9220ee1b3f47bc37078f7c80922922a8ad..e17a626b86962a88f1561ef64f667af579cc2430 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -438,7 +438,7 @@ fi
 %posttrans server
 # This must be run in posttrans so that updates from previous
 # execution that may no longer be shipped are not applied.
-/usr/sbin/ipa-ldap-updater --upgrade >/dev/null || :
+/usr/sbin/ipa-ldap-updater --upgrade --quiet || :
 
 %preun server
 if [ $1 = 0 ]; then
@@ -769,6 +769,9 @@ fi
 %ghost %attr(0644,root,apache) %config(noreplace) %{_sysconfdir}/ipa/ca.crt
 
 %changelog
+* Thu Jan 24 2013 Petr Viktorin  - 3.0.99-13
+- Use ipa-ldap-updater --quiet instead of redirecting to /dev/null
+
 * Thu Jan 24 2013 Rob Crittenden  - 3.0.99-12
 - Add certmonger condrestart to server post scriptlet
 - Make certmonger a (pre) Requires on the server subpackage
diff --git a/ipapython/admintool.py b/ipapython/admintool.py
index b644516dd2afd0d373734767feee4d65b3fbfd5b..5bc21516562ac2a8c6790ea3421628a4c7db9797 100644
--- a/ipapython/admintool.py
+++ b/ipapython/admintool.py
@@ -76,14 +76,15 @@ class AdminTool(object):
 Class attributes to define in subclasses:
 command_name - shown in logs
 log_file_name - if None, logging is to stderr only
-needs_root - if true, non-root users can't run the tool
 usage - text shown in help
+
+See the setup_logging method for more info on logging.
 """
 command_name = None
 log_file_name = None
-needs_root = False
 usage = None
 
+log = None
 _option_parsers = dict()
 
 @classmethod
@@ -95,10 +96,23 @@ class AdminTool(object):
 cls.add_options(parser)
 
 @classmethod
-def add_options(cls, parser):
-

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

2013-01-03 Thread 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 

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

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel