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

Deprecate uninstall for Plugin Manager #6042

Closed
wants to merge 1 commit into from

Conversation

suyograo
Copy link
Contributor

@suyograo suyograo commented Oct 13, 2016

Adds remove and deprecates uninstall to be consistent with rest of Elastic stack

Fixes #6041

@@ -25,7 +25,6 @@ gem "docker-api", "1.31.0", :group => :development
gem "pleaserun"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ugh, not intentional

context "when the plugin isn't installed" do
it "fails to remove it" do
result = logstash.run_command_in_path("bin/logstash-plugin remove logstash-filter-qatest")
expect(result.stderr).to match(/ERROR: Uninstall Aborted, message: This plugin has not been previously installed, aborting/)
Copy link
Member

Choose a reason for hiding this comment

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

Uninstall Aborted -> Remove Aborted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

expect(logstash).to have_installed?("logstash-filter-qatest")

result = logstash.run_command_in_path("bin/logstash-plugin remove logstash-filter-qatest")
expect(result.stdout).to match(/^Uninstalling logstash-filter-qatest/)
Copy link
Member

Choose a reason for hiding this comment

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

Uninstalling -> Removing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Member

@pickypg pickypg left a comment

Choose a reason for hiding this comment

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

Minor issues that will likely prevent it from building, but otherwise LGTM.

Copy link
Contributor

@ph ph left a comment

Choose a reason for hiding this comment

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

Just a simple comment to make sure we don't have any duplicated code around. Other than that it look good to me.

@@ -1,13 +1,16 @@
# encoding: utf-8
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could make the uninstall inherits the remove command, override #execute and call super? This would make sure don't have any duplicated code around.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just want to have something real quick for 5.0. I understand it is dup code, but I don't want to break stuff

Copy link
Contributor

Choose a reason for hiding this comment

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

Good for me, we will remove it in 5.1? correct?

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

@ph
Copy link
Contributor

ph commented Oct 13, 2016

LGTM

Adds remove and deprecates uninstall

Fixes elastic#6041
@elasticsearch-bot
Copy link

Suyog Rao merged this into the following branches!

Branch Commits
master 44a52ff
5.0 ae379e7
5.x 9c5e558

elasticsearch-bot pushed a commit that referenced this pull request Oct 13, 2016
Adds remove and deprecates uninstall

Fixes #6041

Fixes #6042
elasticsearch-bot pushed a commit that referenced this pull request Oct 13, 2016
Adds remove and deprecates uninstall

Fixes #6041

Fixes #6042
@suyograo suyograo mentioned this pull request Oct 13, 2016
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

5 participants