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
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
