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

Plugins: Add support for platform specific plugins #24265

Merged
merged 4 commits into from Apr 27, 2017

Conversation

Projects
None yet
2 participants
@rjernst
Member

rjernst commented Apr 22, 2017

This commit adds support for plugins having a platform specific variant.
It also adds unit tests for all official and maven urls.

Plugins: Add support for platform specific plugins
This commit adds support for plugins having a platform specific variant.
It also adds unit tests for all official and maven urls.
@jasontedor

I left some comments, the main one is about the redirects.

if (urlExists(terminal, platformUrl)) {
return platformUrl;
}
return String.format(Locale.ROOT, "%1$s/%2$s-%3$s.zip", baseUrl, pluginId, version);

This comment has been minimized.

@jasontedor

jasontedor Apr 26, 2017

Member

It looks like we do not need the argument index modifiers in any of the format strings in this method anymore?

@SuppressForbidden(reason = "Make HEAD request using URLConnection.connect()")
boolean urlExists(Terminal terminal, String urlString) throws IOException {
terminal.println(VERBOSE, "Checking if url exists: " + urlString);
assert urlString.startsWith("https://") : "Only http urls can be checked";

This comment has been minimized.

@jasontedor

jasontedor Apr 26, 2017

Member

The assert message should be https not http!

This comment has been minimized.

@jasontedor

jasontedor Apr 26, 2017

Member

And maybe it's better to construct the URL and just assert that "https".equals(url.getProtocol())?

terminal.println(VERBOSE, "Checking if url exists: " + urlString);
assert urlString.startsWith("https://") : "Only http urls can be checked";
URL url = new URL(urlString);
HttpURLConnection urlConnection = (HttpURLConnection)url.openConnection();

This comment has been minimized.

@jasontedor

jasontedor Apr 26, 2017

Member

Nit: space between the cast operator and the target.

urlConnection.addRequestProperty("User-Agent", "elasticsearch-plugin-installer");
urlConnection.setRequestMethod("HEAD");
urlConnection.connect();
return urlConnection.getResponseCode() == 200;

This comment has been minimized.

@jasontedor

jasontedor Apr 26, 2017

Member

What if there's a redirect?

This comment has been minimized.

@rjernst

rjernst Apr 26, 2017

Member

We don't support redirects right now. The code in downloadZip does a direct connection, no redirect handling.

This comment has been minimized.

@jasontedor

jasontedor Apr 27, 2017

Member

Ah, okay. Then I'm good with it as-is; we can consider redirects separately (if ever).

if (urlExists(terminal, platformUrl)) {
return platformUrl;
}
return String.format(Locale.ROOT, "%1$s/%2$s-%3$s.zip", baseUrl, artifactId, version);

This comment has been minimized.

@jasontedor

jasontedor Apr 26, 2017

Member

Same thing here, I don't think the argument index modifiers are needed?

@jasontedor

LGTM.

@rjernst rjernst merged commit cdcc75d into elastic:master Apr 27, 2017

2 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci Build finished.
Details

@rjernst rjernst deleted the rjernst:plugin_platforms branch Apr 27, 2017

rjernst added a commit that referenced this pull request Apr 27, 2017

Plugins: Add support for platform specific plugins (#24265)
This commit adds support for plugins having a platform specific variant.
It also adds unit tests for all official and maven urls.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment