On 07/17/2013 01:48 PM, Jan Cholasta wrote: > 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.
In this case, it at least uses options from IPA server configuration (api.env) - which I think will be the case for most avices of this kind. > >> >> 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 > Pushed to master. Martin _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
