Skip to content
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

Improve error message when installing an offline plugin #28298

Conversation

Projects
None yet
5 participants
@mayya-sharipova
Copy link
Contributor

commented Jan 18, 2018

Provide more actionable error message when installing an offline plugin
in the plugins directory, and the plugins directory for the node
contains plugin distribution.

Closes #27401

@jasontedor
Copy link
Member

left a comment

I think we can detect this situation sooner and much more clearly (e.g., in install). Would you take another approach?

distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java Outdated
}
// TODO: no jars should be an error
// TODO: verify the classname exists in one of the jars!
} catch (IllegalStateException e) {

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jan 29, 2018

Member

I do not think this is right, we could catch an IllegalStateException for JAR hell and really we are only trying to detect when the offline plugin is contained in the plugins folder.

@mayya-sharipova mayya-sharipova force-pushed the mayya-sharipova:improve-error-message-for-installing-offline-plugins branch Jan 29, 2018

@rjernst

This comment has been minimized.

Copy link
Member

commented Jan 31, 2018

I think downloadZip would be the best place to detect this. Once the URL is created, see if it is a file URL, and if it is, convert it to a local path, then see if the path is prefixed by the plugins dir?

@mayya-sharipova mayya-sharipova force-pushed the mayya-sharipova:improve-error-message-for-installing-offline-plugins branch Feb 1, 2018

@jasontedor

This comment has been minimized.

Copy link
Member

commented Mar 14, 2018

@mayya-sharipova Would you be able to incorporate the feedback here and iterate on this PR?

@mayya-sharipova

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2018

@jasontedor Thanks Jason, I have already incorporated it. I followed the last suggestion from Ryan.

@jasontedor

This comment has been minimized.

Copy link
Member

commented Mar 14, 2018

@mayya-sharipova I see, again we see the problem with force pushing to a pull request; I do not think anyone knew that new commits were pushed here. Please ping when you push commits. Also, would you resolve the existing merge conflicts?

@jasontedor
Copy link
Member

left a comment

Thank you @mayya-sharipova, the change looks good. I left one comment.

distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java Outdated
Path pluginsFile = Paths.get(url.getFile());
if (pluginsFile.startsWith(pluginsDir)) {
throw new IllegalStateException("Installation failed! " +
"Make sure `plugins` directory doesn't contain the plugin distribution! " );

This comment has been minimized.

Copy link
@jasontedor

jasontedor Mar 14, 2018

Member

I think we can have a more actionable error message here like: "the plugins directory [" + pluginsDir + "] can not contain the plugin distribution [" + pluginsFile + "]; move the distribution to an alternate location"

@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Mar 14, 2018

@mayya-sharipova mayya-sharipova force-pushed the mayya-sharipova:improve-error-message-for-installing-offline-plugins branch Mar 14, 2018

Improve error message for installing plugin
Provide more actionable error message when installing an offline plugin
in the plugins directory, and the `plugins` directory for the node
contains plugin distribution.

Closes #27401

@mayya-sharipova mayya-sharipova force-pushed the mayya-sharipova:improve-error-message-for-installing-offline-plugins branch to 68c91ac Mar 14, 2018

@mayya-sharipova mayya-sharipova merged commit 0cc1ffd into elastic:master Mar 14, 2018

2 checks passed

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

@mayya-sharipova mayya-sharipova deleted the mayya-sharipova:improve-error-message-for-installing-offline-plugins branch Mar 14, 2018

mayya-sharipova added a commit that referenced this pull request Mar 14, 2018

Improve error message for installing plugin (#28298)
Provide more actionable error message when installing an offline plugin
in the plugins directory, and the `plugins` directory for the node
contains plugin distribution.

Closes #27401
@jasontedor

This comment has been minimized.

Copy link
Member

commented Mar 14, 2018

@mayya-sharipova I’m confused. I left a comment about the user-facing message but I do not see that incorporated here nor a response to that comment although this change has been merged?

@jasontedor

This comment has been minimized.

Copy link
Member

commented Mar 14, 2018

Oh I see that you force pushed! Please prefer to push additional commits instead of force pushing. There are generally very few situations in which we need to force push.

mayya-sharipova added a commit that referenced this pull request Mar 15, 2018

Revert "Improve error message for installing plugin (#28298)"
This reverts commit 0cc1ffd

The reason is that Windows test are failing,
because of the incorrect path for the plugin

mayya-sharipova added a commit that referenced this pull request Mar 15, 2018

Revert "Improve error message for installing plugin (#28298)"
This reverts commit 6284b7e.

The reason is that Windows test are failing,
because of the incorrect path for the plugin

martijnvg added a commit that referenced this pull request Mar 16, 2018

Merge remote-tracking branch 'es/6.x' into ccr-6.x
* es/6.x: (89 commits)
  Clarify requirements of strict date formats. (#29090)
  Clarify that dates are always rendered as strings. (#29093)
  [Docs] Fix link to Grok patterns (#29088)
  Fix starting on Windows from another drive (#29086)
  Use removeTask instead of finishTask in PersistentTasksClusterService (#29055)
  Added minimal docs for reindex api in java-api docs
  Allow overriding JVM options in Windows service (#29044)
  Clarify how to set compiler and runtime JDKs (#29101)
  Fix Parsing Bug with Update By Query for Stored Scripts (#29039)
  TEST: write ops should execute under shard permit (#28966)
  [DOCS] Add X-Pack upgrade details (#29038)
  Revert "Improve error message for installing plugin (#28298)"
  Docs: HighLevelRestClient#exists (#29073)
  Validate regular expressions in dynamic templates. (#29013)
  [Tests] Fix GetResultTests and DocumentFieldTests failures (#29083)
  Reenable LiveVersionMapTests.testRamBytesUsed on Java 9. (#29063)
  Mute failing GetResultTests and DocumentFieldTests
  [Docs] Fix Java Api index administration usage (#28260)
  Improve error message for installing plugin (#28298)
  Decouple XContentBuilder from BytesReference (#28972)
  ...

martijnvg added a commit that referenced this pull request Mar 16, 2018

Merge remote-tracking branch 'es/master' into ccr
* es/master: (97 commits)
  Clarify requirements of strict date formats. (#29090)
  Clarify that dates are always rendered as strings. (#29093)
  Compilation fix for #29067
  [Docs] Fix link to Grok patterns (#29088)
  Store offsets in index prefix fields when stored in the parent field (#29067)
  Fix starting on Windows from another drive (#29086)
  Use removeTask instead of finishTask in PersistentTasksClusterService (#29055)
  Added minimal docs for reindex api in java-api docs
  Allow overriding JVM options in Windows service (#29044)
  Clarify how to set compiler and runtime JDKs (#29101)
  CLI: Close subcommands in MultiCommand (#28954)
  TEST: write ops should execute under shard permit (#28966)
  [DOCS] Add X-Pack upgrade details (#29038)
  Revert "Improve error message for installing plugin (#28298)"
  Docs: HighLevelRestClient#exists (#29073)
  Validate regular expressions in dynamic templates. (#29013)
  [Tests] Fix GetResultTests and DocumentFieldTests failures (#29083)
  Reenable LiveVersionMapTests.testRamBytesUsed on Java 9. (#29063)
  Mute failing GetResultTests and DocumentFieldTests
  Improve error message for installing plugin (#28298)
  ...

@colings86 colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.