[Freeipa-devel] [PATCH 0072] Provide ipa-client-advise tool

[Tomas Babej]

On Tuesday 16 of July 2013 14:10:44 Jan Cholasta wrote:
> On 21.6.2013 11:45, Tomas Babej wrote:
> > Newly added features:
> >
> >   - options propagated to plugins
> >   - made plugin content creation more comfortable, now 3 classes of
> > output are
> >     available (debug, comment, command)
> >
> > Now pretty much everything that comes into my mind is addressed, so
> > please have a look
> > at the current implementation.
> 
> The patch needs a rebase.
> 
> +    class AdviceLogger(object):
> 
> Please don't use nested classes. If you want AdviceLogger to be 
> private-ish, you can rename it to _AdviceLogger.
> 
> Also I think AdviceLogger is a little bit misleading name, I would 
> prefer AdviceOutput or something like that.
> 

Fixed.

> Functionally the patch is OK, but I have some second thoughts about the 
> design. I'm not sure if using API plugins is truly the right thing to 
> do, as advises seem to be pretty much orthogonal to the rest of our API. 
> There are some negative side effects, such as initializing the API every 
> time ipa-advise is run, for each and every advice, which takes some 
> time, so there is a short but noticable delay.

What do you mean by that API is initialized for each and every advice?

AFAIK, the advice plugins are all imported at once, the the API is initialized.

They are imported only in the API 'advise' context, so no performance decrease
for the rest of the framework.

> What are the benefits of 
> using API plugins for this, besides code reuse? (I'm not saying this 
> must be changed, just give it some thought, using something simpler 
> might be better.)

Code reuse is one thing. Also, ability to call the IPA commands from
within the plugins is the second factor. To allow that we would have to
inicialize the API anyway.

Also some important constants which can be leveraged by the plugins are
contained in api.env namespace.

Taking into consideration that running ipa-advise is more of a 
one-time thing, I am willing to sacrifice a bit of delay in 
favour of these advantages.

Updated patch attached.

Tomas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130717/e56c7fc2/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0072-6-Provide-ipa-advise-tool.patch
Type: text/x-patch
Size: 20580 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130717/e56c7fc2/attachment.bin>