-
Notifications
You must be signed in to change notification settings - Fork 24.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use 'name' from plugin descriptor file to determine plugin name #12775
Conversation
Some open questions I have with the original issue which are not addressed by this PR so far:
|
String pluginName = props.getProperty("plugin.name"); | ||
if (pluginName == null) { | ||
throw new IllegalArgumentException("Property [plugin.name] is missing for plugin [" + name + "]"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if in that case we should not fallback to the previous convention (use zip name as the plugin name) and just "warn" the user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I wasn't too sure about the original issues goals here, so I didn't elaborate any further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the whole point of this PR is to add this new parameter and make it required, so I don't think we should fall back? Otherwise the leniency that we are trying to fix is still here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In particular, since we now require that plugins have exactly the same version as elasticsearch, plugins will have to be re-packaged anyway, so I don't think bw compat is a concern?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a big deal. I was just thinking of community plugin developers but it's not hard for them to fix. We should may be add this in BWC doc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also check the name matches that in jvm plugins. As a follow up, we should at least remove description from jvm Plugin interface, and possibly also name (possibly a little harder, just requires passing around PluginInfo instead of Plugin I think).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I just checked and removing name and description from the Plugin interface should be easy. The only thing to think about is what to give for those properties when plugins are loaded from the classpath (plugin.types). I think here the name should just be the classname, and description something like "plugin loaded from classpath"? I don't know what other info we really have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rmuir added element to the enforcer plugin, but I'm not sure what check to insert in pluginservice and how that service actually works. Can this be added in a follow up PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rjernst Ryan, not sure what you mean by jvm plugins and the checks your suggested, can this be done outside of this PR? Otherwise where do I need to look?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rjernst since you already seem to have some additional checks in mind, could you open that in a follow up issue or let me know what really needs to be done in this PR.
Some thoughts that come to mind. Now we know that if the user set a
And that So it does not make sense anymore to support this We should have: # download from elastic.co, maven central or github
bin/plugin install foo
# download from a URI (URL)
bin/plugin install http://link.to/foo.zip
# download from a URI (file)
bin/plugin install file:.target/releases/foo.zip @spinscale @clintongormley WDYT? |
Hrm, @dadoonet Can we be explicit instead of having to infer what was passed as the argument to install? What you suggest would require even more heuristics. Instead, why don't we remove the fallback to trying as a url, change http:// to require using --url, and add a --file that requires a local path? |
@@ -95,6 +95,10 @@ public static PluginInfo readFromProperties(Path dir) throws IOException { | |||
if (version == null) { | |||
throw new IllegalArgumentException("Property [version] is missing for plugin [" + name + "]"); | |||
} | |||
String pluginName = props.getProperty("plugin.name"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm, can we just call this "name"? None of the other settings are prefixed with plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, changed the property to just 'name'.
Also, when we download a plugin, if it was downloaded by name, we should verify the name matches? |
I agree with being explicit:
|
@spinscale Does CLI allows that? I think we might need to do something like: # download from elastic.co, maven central or github
bin/plugin install foo
# download from a URI (URL)
bin/plugin install http://link.to/foo.zip --url
# download from a URI (file)
bin/plugin install .target/releases/foo.zip --file |
the last two syntaxes are really confusing IMO, one can change to allow for that, but it doesnt feel nice |
If we can't easily support the version in #12775 (comment) then let's use the version in #12775 (comment) The heuristic would simply be: anything matching this regex is a URL |
Wasn't @spinscale talking about the odd syntax of adding options after the thing the option is pointing to? Surely having command line parameter that takes an argument is supported by the CliTool? We should simply not use heuristics; they are not needed, and only lead to confusing errors. |
@rjernst yes he was. @dadoonet was not sure if the CLI parser we use would support that style of argument parsing. I'm saying that, if it doesn't (and is not easy to fix) then we should rather use the simple heuristic than the confusing syntax. But I agree, I can't see why the CLI parser wouldn't support it. |
Looks good me to regarding support... One last thing though:
Do we really need a |
@spinscale Thanks for that suggestions, looking at the implementation details and given that |
@spinscale I like this approach. In this case we shoudln't provide So to confirm, the logic would be:
|
But that means we should remove |
@rjernst depening on our impl, logic would be
and yes, we could totally drop the --url in that case and just have a single argument, the identifier, being a name, local path or URL |
Why would the inner if be necessary? |
@rjernst sure, but you might want to use paths like |
We should not do that. That is leniency. Using a local file is expert, let's just stick to a uri, and not have to deal with |
It's also no different than what users already had to do, using --url. We can just remove --url, since we can always clearly tell if something is a uri. |
valid argument, +1 |
pluginUrl = new URL(name); | ||
name = null; | ||
} catch (MalformedURLException e) { | ||
// ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems strange - maybe explain why its ok to ignore this here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do plugin install [nameOrUrl]
and the argument doesn't parse to a valid url, we go on treating it as on of the other three options of a plugin name (e.g. analysis-icu, user/repo etc...). I can update the comment to make this clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok - yeah. Please update the comment. It'll help people just scanning the code.
@cbuescher I think this file needs also to be updated with your change: https://github.com/elastic/elasticsearch/blob/master/docs/plugins/plugin-script.asciidoc#custom-url-or-file-system |
Updated the PR, rebased and added comments, changes in logging suggested by @nik9000 and changed docs. |
|
||
plugin install elasticsearch/shield/latest | ||
|
||
plugin install lmenezes/elasticsearch-kopf | ||
|
||
plugin install http://download.elasticsearch.org/elasticsearch/elasticsearch-analysis-kuromoji/elasticsearch-analysis-kuromoji-2.7.0.zip | ||
|
||
plugin install file:///path/to/plugin/elasticsearch-analysis-kuromoji-2.7.0.zip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometimes non java devs are confused by that and I don't think //
is required. May be this?
plugin install file:/path/to/plugin/elasticsearch-analysis-kuromoji-2.7.0.zip
or
plugin install file:./path/to/plugin/elasticsearch-analysis-kuromoji-2.7.0.zip
Or may be adding a windows user example like this (not tested) ?
plugin install file:c:/path/to/plugin/elasticsearch-analysis-kuromoji-2.7.0.zip
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestions, will test this.
I left a small comment but the change looks good to me. |
@cbuescher I merged #12879 so you can now change pom.xml files to use maven artifactId instead of setting manually |
@dadoonet thanks, I updated the poms using the artifactId instead of hardcoding the name, added some documentation and rebased. I'll sqash this again before merging with master. |
I left one small comment. I think we are really close now! |
@dadoonet thanks for the review, didn't know |
LGTM! |
…lugin name At the moment, when installing from an url, a user provides the plugin name on the command line like: * bin/plugin install [plugin-name] --url [url] This can lead to problems when picking an already existing name from another plugin, and can potentially overwrite plugins already installed with that name. This, this PR introduces a mandatory `name` property to the plugin descriptor file which replaces the name formerly provided by the user. With the addition of the `name` property to the plugin descriptor file, the user does not need to specify the plugin name any longer when installing from a file or url. Because of this, all arguments to `plugin install` command are now either treated as a symbolic name, a URL or a file without the need to specify this with an explicit option. The new syntax for `plugin install` is now: bin/plugin install [name or url] * downloads official plugin bin/plugin install analysis-kuromoji * downloads github plugin bin/plugin install lmenezes/elasticsearch-kopf * install from URL or file bin/plugin install http://link.to/foo.zip bin/plugin install file:/path/to/foo.zip If the argument does not parse to a valid URL, it is assumed to be a name and the download location is resolved like before. Regardless of the source location of the plugin, it is extracted to a temporary directory and the `name` property from the descriptor file is used to determine the final install location. Relates to elastic#12715
@dadoonet Rebased and squashed, will merge this to master so it can be tested there before we might move this to the branch. |
Use 'name' from plugin descriptor file to determine plugin name
Also backported to 2.0 branch with d236192 |
@clintongormley should this be labeled |
Update #12768 ? |
At the moment, when installing from an url, a user provides a plugin name on the command line like:
This can lead to confusion when picking an already existing name from another plugin, which might even overwrite plugins already installed with that name.
In order to remedy this, this PR introduces an additional mandatory
name
property to the plugin descriptor file which overwrites the name provided by the user after the plugin was loaded and the descriptor file is unpacked.Relates to #12715