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

[John Dennis]

On 07/20/2012 12:34 PM, Petr Viktorin wrote:
> On 07/20/2012 05:59 PM, John Dennis wrote:
>>>> 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.
>>>
>>> Updated. I also added the necessary lint exception.
>>> I'm curious as to why it works that way, though. Normally, to put
>>> methods in a class, you would use a base class/mixin. Why this dynamic
>>> magic?
>>
>> Good question. I don't like dynamic magic either, it makes static code
>> reading difficult and diminishes the value of static code
>> analysis/checking. Jason who originally wrote the framework used dynamic
>> magic a fair amount, including the initialization of the logging methods
>> in the plugins. When I wrote the log manager I kept Jason's existing
>> strategy (how many things do change at once?). In hindsight a mixin
>> would have been a better solution, something we should probably fix down
>> the road.
>
> Thinking more about this, composition would probably be cleaner than
> inheritance here (i.e. using self.logger.warn(...) instead of mixing the
> functionality into the class itself).
> Well, one day the time to fix it will come.
>
>> Everything looks good, although there might be one minor thing that
>> needs fixing. Shouldn't the logging setup be done first? That way any
>> code that executes has the ability to emit a message. For example what
>> if validate_options() wants to emit a message?
>>
>
> That's a good question.
> If we set up logging before validating the options, we'll log all
> invocations, including those with misspelled option names and forgotten
> "sudo"s. Do we want that?
>

Sure, hard to say. Why don't we leave it the way it is now and down the
road if it shows up as an issue we'll tweak it then.

ACK.

-- 
John Dennis <jdennis at redhat.com>

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