[Freeipa-devel] [PATCH] 0056 Framework for admin/install tools, with ipa-ldap-updater

[John Dennis]

Overall I really like the approach, good work.

I have not applied and run the patch, so my comments are based only on 
code reading.

I have just a small things which might need changing.

One of the ideas in the log manager is to easily support per class 
logging (borrowed from Java). This allows you to enable/disable or 
otherwise configure logging per logical code unit as opposed to a big 
global hammer (i.e. root logger). It also allows for the class name 
(logger name) to show up in the log output which can be really handy so 
you know which class is emitting a message (we don't currently turn this 
on but it's a trival one-line change to the log format spec.).

Each significant class should initialize it's own logging, which is very 
easy to do with this line at the top of a class's __init__()

     log_mgr.get_logger(self, True)

so instead of:

     self.logger.info()

you would say:

     self.info()

A fair amount of the code in the framework is doing this now, but the 
install code was never cleaned up. That was left for another day, I 
guess that day is here.

Also each log method (i.e. info(), warn(), error(), etc) should pass 
it's format substitutions as parameters rather than preformatting the 
message before submitting it to the logger. This is a performance 
optimization because the logger delays building the message until it 
knows the message will actually be emitted and not discarded (common 
case with debug messages).

So instead of:

     self.debug("the contents of this big dict is: %s" % my_dict)

You would write:

     self.debug("the contents of this big dict is: %s", my_dict)

A small different in coding, but a lot less work at run time.

I'm not sure the following message handling is optimal

+        message = '\n'.join(
+            "The hostname resolves to the localhost address 
(127.0.0.1/::1)",
+            "Please change your /etc/hosts file so that the hostname",
+            "resolves to the ip address of your network interface.",
+            "",
+            "Please fix your /etc/hosts file and restart the setup 
program",


I'm not a big fan of individual lines of text in the source that are 
glued together or are emitted individually via a series of print 
statements. I'd rather see multi-line text as multi-line text using 
Python's multi-line string operator (e.g. ''' or """). That way you can 
see the text as it's meant to appear, but there is a bigger reason.

Shouldn't this message be internationalized? (e.g. wrapped in _()). When 
a message is internationalized it's even more important for multi-line 
text to appear in it entirety to give translators the greatest context 
and latitude for their translation rather than arbitrarily splitting it 
up into fragments according to English norms (fragments that might not 
even translate in another language).

Also, a quick glance suggests a number of the messages need i18n treatment.

Overall though, I really love the approach, great work!



-- 
John Dennis <jdennis at redhat.com>

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