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

Add purge option to remove plugin CLI #24981

Merged
merged 4 commits into from Jun 1, 2017

Conversation

Projects
None yet
4 participants
@jasontedor
Copy link
Member

commented May 31, 2017

By default, the remove plugin CLI command preserves configuration files. This is so that if a user is upgrading the plugin (which is done by first removing the old version and then installing the new version) they do not lose their configuration file. Yet, there are circumstances where preserving the configuration file is not desired. This commit adds a purge option to the remove plugin CLI command.

Add purge option to remove plugin CLI
By default, the remove plugin CLI command preserves configuration
files. This is so that if a user is upgrading the plugin (which is done
by first removing the old version and then installing the new version)
they do not lose their configuration file. Yet, there are circumstances
where preserving the configuration file is not desired. This commit adds
a purge option to the remove plugin CLI command.
@@ -59,9 +59,13 @@ public void setUp() throws Exception {
}

static MockTerminal removePlugin(String name, Path home) throws Exception {

This comment has been minimized.

Copy link
@nik9000

nik9000 May 31, 2017

Contributor

I'd prefer to add the argument and pass false all the places that call this unless there are far more callers that I can see.

This comment has been minimized.

Copy link
@nik9000

nik9000 May 31, 2017

Contributor

In fact, I bet a few places should have randomBoolean() instead of false.

jasontedor added some commits Jun 1, 2017

@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Jun 1, 2017

@nik9000 I addressed your suggestion. I pushed a bigger change though which is to allow purge to be used after a plugin is removed to cleanup lingering config files. This necessitated removing the marker file that we drop in the specific plugin directory into the root plugin directory. Since this is a bigger change, can you take another look?

@nik9000

nik9000 approved these changes Jun 1, 2017

Copy link
Contributor

left a comment

Makes sense to me. I think this breaks backwards compatibility with previous failure markers but that that is probably ok. I hope folks don't leave half removed plugins laying around for too long.

@jasontedor jasontedor merged commit 9b4a189 into elastic:master Jun 1, 2017

1 check was pending

elasticsearch-ci Build started sha1 is merged.
Details

jasontedor added a commit that referenced this pull request Jun 1, 2017

Add purge option to remove plugin CLI
By default, the remove plugin CLI command preserves configuration
files. This is so that if a user is upgrading the plugin (which is done
by first removing the old version and then installing the new version)
they do not lose their configuration file. Yet, there are circumstances
where preserving the configuration file is not desired. This commit adds
a purge option to the remove plugin CLI command.

Relates #24981
@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Jun 1, 2017

Thanks @nik9000.

@jasontedor jasontedor deleted the jasontedor:remove-plugin-purge branch Jun 1, 2017

@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Jun 1, 2017

I think this breaks backwards compatibility with previous failure markers but that that is probably ok. I hope folks don't leave half removed plugins laying around for too long.

Indeed.

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.