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

PluginManager: Dont leave leftover files on unsuccessful installs #12851

Conversation

spinscale
Copy link
Contributor

If the plugin manager cannot successfully install a plugin, ensure
that every directory is cleaned up again. This includes

plugins/foo
config/foo
bin/foo

Closes #12749

@spinscale spinscale added review :Core/Infra/Plugins Plugin API and infrastructure v2.0.0 labels Aug 13, 2015
@@ -208,17 +218,53 @@ private void extract(PluginHandle pluginHandle, Terminal terminal, Path pluginFi
IOUtils.rm(tmp, pluginFile);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be here use tryToDeletePath() instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, did that

@dadoonet
Copy link
Member

Left 2 small comments. It looks good to me.

@@ -183,6 +183,16 @@ private void extract(PluginHandle pluginHandle, Terminal terminal, Path pluginFi
throw new IOException("plugin directory " + extractLocation.toAbsolutePath() + " already exists. To update the plugin, uninstall it first using 'remove " + pluginHandle.name + "' command");
}

Path pluginConfigDirectory = pluginHandle.configDir(environment);
if (Files.exists(pluginConfigDirectory) && !Files.isDirectory(pluginConfigDirectory)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be checking if the directory is writable here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not needed, one test is covering this already and returning an appropriate exception

@spinscale spinscale force-pushed the 1508-issue-12749-plugin-manager-verify-write-permissions-on-all-dirs branch from 19ff4a1 to f7a012f Compare September 1, 2015 12:32
@spinscale
Copy link
Contributor Author

fixed the review comments, rebased against master with a fair share of changes and only run the logic to copy bin and config dirs if actually needed, otherwise exit early

please give it another look


Path sourceConfigDirectory = extractLocation.resolve("config");
Path destConfigDirectory = pluginHandle.configDir(environment);
boolean needToCopyConfigDirectory = Files.exists(sourceConfigDirectory);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you create this needToCopyConfigDirectory instead of adding documentation, right?
If not, may be:

if (Files.exists(sourceConfigDirectory)) {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, we were testing in the past that sourceConfigDirectory is actually a dir and not only an existing file.
Did you change that on purpose? Or should it be if (Files.isDirectory(sourceConfigDirectory)) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I prefer well named variables instead of comments. In this example though it is not too useful. There used to be two conditions, but now that it is only one, it's not that helpful anymore

@dadoonet
Copy link
Member

dadoonet commented Sep 3, 2015

@spinscale I left some comments.

if (needToCopyBinDirectory) {
if (Files.exists(destPluginBinDirectory) && !Files.isDirectory(destPluginBinDirectory)) {
tryToDeletePath(terminal, extractLocation);
throw new IOException("plugin bin directory " + destPluginBinDirectory + " is not a directory");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean to fail here after deleting the path? Maybe WARN and then delete the path and fail if we can't delete it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! I see now. You don't do mean to fail. You aren't deleting the destPluginBinDirectory, you are deleting the extractLocation to clean up. Got it! +1

@nik9000
Copy link
Member

nik9000 commented Sep 15, 2015

@spinscale, there are a few comments around more you could do here but I think its an improvement as is. Its worth merging, I think.

I love the tests. Very descriptive method names help a ton.

If you want to add the tests for sourceConfigPath being a file instead of a dir I'd cause that to be an "abort fast" thing. Its a pretty broken plugin that does it.

@spinscale spinscale force-pushed the 1508-issue-12749-plugin-manager-verify-write-permissions-on-all-dirs branch from f7a012f to 7986974 Compare October 7, 2015 11:07
@spinscale
Copy link
Contributor Author

@dadoonet updated your last comment and rebased against master, mind to take another view?

@nik9000
Copy link
Member

nik9000 commented Oct 7, 2015

LGTM

If the plugin manager cannot successfully install a plugin, ensure
that every directory is cleaned up again. This includes

plugins/foo
config/foo
bin/foo

Closes elastic#12749
@spinscale spinscale force-pushed the 1508-issue-12749-plugin-manager-verify-write-permissions-on-all-dirs branch from 7986974 to 70b2d90 Compare October 8, 2015 08:05
@spinscale spinscale merged commit 70b2d90 into elastic:master Oct 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants