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

[Jan Cholasta]

On 17.7.2013 13:13, Tomas Babej wrote:
>  > + 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.

Thanks.

>
>  > 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?

For example when you run "ipa-advise config-fedora-authconfig", all of 
the ipalib and advise plugins are initialized. Seems like an overkill 
just to print 6 lines of text.

>
> 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.

... which could be done on-demand when it is actually needed.

>
> 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.

OK.

I still think that it's rather strange to pretend that advices are part 
of our API when they don't actually contribute anything to the API, but 
that's more of a structural problem, not a problem with your patch.

>
> Updated patch attached.

ACK.

Honza

-- 
Jan Cholasta