QA: Create a test plugin with configuration #12787
Conversation
In order to test the way plugins with configuration are installed and removed we need a plugin with configuration in the repository. The simplest way to get one is to make an "example" plugin. In the process of making this example it became aparent that cat actions were difficult to create outside of the org.elasticsearch.rest.action.cat package because key methods in AbstractCatAction were package private. This makes them protected and uses them to create the example configured plugin. Relates to elastic#12717 but is only one step of many to close it.
nik9000
added
>test
|
This is step 0 in removing shield from the integration tests - it creates a shield lookalike that we can use to test the configuration installation and removal. |
|
@rjernst so merged. |
| @Override | ||
| protected void configure() { | ||
| bind(ExamplePluginConfiguration.class).asEagerSingleton(); | ||
| Multibinder<AbstractCatAction> catActionMultibinder = Multibinder.newSetBinder(binder(), AbstractCatAction.class); |
There was a problem hiding this comment.
It's interesting that this is the way we add cat actions...I wonder if this should be explicit registration like we have for other extension points, rather than having to know to setup a multibinder and what class to bind to. Not relevant for this PR, but I think it would be a good issue to spinoff?
There was a problem hiding this comment.
I don't like the whole registration thing because it means there are two ways to do everything. I'd prefer to just do it the Multibinder way - maybe make some sugar to make that less ceremonial.
There was a problem hiding this comment.
No, there are only two ways for sets/maps, because of how multibinder works. I think it is more confusing to have some things bound using a binder, and others with registration/settings. Classes that ES controls should be registered, and set. If there was a way to just not allow multiple multibinders to work I would say we should do that, but I don't think such a thing exists.
There was a problem hiding this comment.
think it is more confusing to have some things bound using a binder, and others with registration/settings.
Yeah - but we're based on Guice. I think that hiding Guice under the covers is just asking for trouble.
Preventing multiple Multibinders from working would just handcuff plugin authors.
There was a problem hiding this comment.
+1 on registration directly via the RestActionModule
I don't think multiple multibindings is supported, but tbh I wouldn't do that if it is/was. By exposing the extension points as registration methods on the Module you communicate what can be extended and what not, what accepts multiple implementation of X and what just replaces a singleton of X, etc...
There was a problem hiding this comment.
But we don't have well defined extension points.
This is not a valid argument for not making the system better with well defined extension points. How can we get to a point where we do have well defined extension points, if we don't start somewhere?
I went and started doing this with onModule - and it requires so much more ceremony!
How so? A Set, a method, and a couple lines in configure?
There was a problem hiding this comment.
Also, we should really move this discussion to another issue. I think this PR is fine as is, this was just a suggestion for a follow up/thought.
There was a problem hiding this comment.
This is not a valid argument for not making the system better with well defined extension points. How can we get to a point where we do have well defined extension points, if we don't start somewhere?
I guess my argument is that Multibinder works regardless of how much work we put into supporting it but onModule only works if we make it work and we aren't doing much with it.
How so? A Set, a method, and a couple lines in configure?
I'll push the commit that gets it working in a minute - its just a few lists and methods to add to them. Compared to the 2 lines in the plugin. I guess the hard part for the plugin author is to onModule the right module. I had to do more debugging Elasticsearch to get the onModule thing working then I did the Multibinder - Guice just worked.
I just don't get why we wrap Guice here - its built for this kind of thing.
There was a problem hiding this comment.
Also, we should really move this discussion to another issue. I think this PR is fine as is, this was just a suggestion for a follow up/thought.
Fair enough. I can merge this and then send another pull request to get onModule working for it and we can go there?
|
LGTM |
QA: Add configuration to the jvm-example plugin
In order to test the way plugins with configuration are installed and removed
we need a plugin with configuration in the repository. The simplest way to
get one is to make an "example" plugin.
In the process of making this example it became aparent that cat actions were
difficult to create outside of the org.elasticsearch.rest.action.cat package
because key methods in AbstractCatAction were package private. This makes them
protected and uses them to create the example configured plugin.
Relates to #12717 but is only one step of many to close it.