Re: [hudson-dev] [Hudson-Dev] Re: Questions and ideas for JSR330 plugins

[Henrik Lynggaard Hansen <henrik@xxxxxxx>]

2012/1/29 Jason Dillon <jason@xxxxxxxxxxxx>:
> If you want to provide a PluginContext that class should really be limited in the components it actually exposes, and really should only encapsulate things which you couldn't normally inject otherwise.
>
> For other components, if your plugin depends on them, well then @Inject them into your plugin.
>
> If you have PluginContext including everything but the kitchen sink and a way to avoid @Injecting specific bits as needed you will just complicate the API.   This god-object model which Hudson was originally designed around (ie. Hudson.getInstance()) is an evil component design anti-pattern and you'd be better off not trying to do the same thing with a PluginContext... which it sort of sounds like you are going down the path of.

Then I haven't explained myself well enough, because I don't want to
create a god object. My initial idea with the PluginContext was to
create a small concise API which could be used by plugins for getting
their context (not a Hudson instance), and for me that involved
- show me where to log (provide a logger)
- show me where I can write files (provide a "File-like" object
pointing to a directory)
- show me the Hudson version, and maybe other metadata like hudson home
- show me which other plugins exists on this instance

Nothing more, anything else goes in other components to inject. I was
myself contemplating whether the above should be just one injection
(PluginContext), or if it should be 3-4 smaller ones. I don't have a
strong opinion on this, so if it is a much better design to have 3-4
then let us have that. I just started the discussion with a
pluginContext because it should be a recognisable concept.

> Also, on the use of Loggers... IMO these have zero business being injected or accessed by something which has been injected.  I can not think of any reason why I would want/need to augment the name of a logger dynamically based on some  plugin context.  If you do want to provide some context, then use a reasonable logging framework like Slf4j which provides a MDC to attach additional meaningful context around logger objects w/o needing to go muck with how loggers are created or what names they have been configured with.

> In almost every situation something like:
>
> private static final Logger log = LoggerFactory.getLogger(This.class);
>
> or for an extensible class:
>
> protected final Logger log = LoggerFactory.getLogger(getClass());
>
> is IMO *highly* preferable over any other method to construct/access a logger instance.

That way of instantiating the Logger might be the easiest may to
handle things from a technical point of view, but from a people point
of view it is horrible:

- As a Hudson administrator wanting to change a log level,I need to
know the (multiple) class names being used. I should not have to do
this., I already know the plugin name, so just let me do
"hudson.plugin.jobrevision=DEBUG" instead of
"com.thalesgroup.hudson.plugins.jobrevision.JobRevision=DEBUG"
- As a plugin developer I would need to document relevant classes to
enable logging for in case of problems, as I cannot expect user to
read the javadoc or source code

> On Jan 26, 2012, at 12:35 PM, Henrik Lynggaard Hansen wrote:
>> However  I haven't found a good overview of which concreate resources
>> hudson provides which can be injected. do we have some documentation
>> which highlights these ?
>
> Any @Extension can be injected, or you can get a collection of them if there are more than one.  Or any other component which has been configured to @Inject via standard JSR-330 specification can be injected.  For the integration that is there, last I looked at it (which hasn't been in a while), most things are adorned with @Named so that Sisu can automatically determine the Guice bindings.  Otherwise a Guice Module would exist which maps the components to their proper bindings.

I did not know that I could inject the extensions, so thank you for
the explanation. So it looks like
http://wiki.hudson-ci.org/display/HUDSON/Extension+points is the best
documentation for such an overview although it doesn't explain how to
inject stuff like the document storage used by maven 3 plugins.

>> * A configured Logger of our own making. This way we can control the
>> logger name and provide a standard naming convention, so that users
>> don't need to hunt around for class names.Secondly haivng it be a
>> Hudson class, we would separate plugins from the underlying framework
>> and hopefully avoid the flurry of 3. party frameworks
>
> See my comments above.  Pick a logging framework that doesn't suck (ie. Sfl4j) and just use it.  Its part of the plugin API IMO.  The sonatype-donated bits didn't use JUL which was used elsewhere because I *ucking hate JUL and flat out wont' use it.  It too verbose, too difficult to configure, too confusing what fine/finer/finest mean, etc.  Its crap crap crap.

Firstly see my above too :-) While I do not share your view on JUL,
the choice of logging framework is not my point. By having creating a
1 class façade to the logging and providing that as a injectable
resource I solve 3 problems .

* Hudson gets to pick the logger name, which solves the human problem
mentioned above
* Plugin developers are shielded from any changes we might make to the
choice of logging framework.
* Plugin developers are not tempted to introduce their own favorite framework

>> * Ability to query Hudson version and other metadata, plus
>
> There are already existing components to do this, you can simply inject them.  Some of the api might be poor, but its there.

Great, then we only need to make them easier to find :-)

>> better/cleaner way of finding other plugins.
>
> What use case is there for a plugin to find other plugins?
>
> A plugin might depend on components provided by another plugin, or may expose an interface for other plugins to participate with it, but I don't see why a plugin would need to know know about other plugins specifically.  In fact I think that would lead towards even more brittle plugins.

One example could be: Building a status overview plugin (think wall
display etc.) I want to know if the claim plugin is present, so that I
can show the claim information if available.Right now the code for
that would be something like

if (Hudson.getInstance().getPlugin != null) {
  ClaimCause cause =.....
}

So the functionality is essentially there, so this idea was more to
chip away at the god object.

The new functionality should hopefully not lead to more brittle
plugins, but I think it is important that the author of the claim
plugin didn't need to do anything in his plugin.

Best Regards
Henrik