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

plugins: improve remove command to support multiple plugins #17030

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

yaauie
Copy link
Member

@yaauie yaauie commented Feb 6, 2025

Release notes

The plugin manager bin/logstash-plugin's remove command now allows multiple plugins to be removed in a single call. This simplifies removing plugins that depend on other plugins, so that the caller no longer needs to perform the removals in dependency order.

What does this PR do?

Removal works by optimistically removing all specified plugin-gems from a mutable gemspec, then validating that doing so does not leave plugin-gems with orphaned runtime dependencies, saving the result and succeeding only if no conflicts were created.

╭─{ rye@perhaps:~/src/elastic/logstash@main (pluginmanager-remove-multiple ✘) }
╰─● bin/logstash-plugin remove logstash-input-syslog logstash-filter-grok
Using system java: /Users/rye/.jenv/shims/java
Resolving dependencies......
Successfully removed logstash-input-syslog
Successfully removed logstash-filter-grok
[success (00:00:05)]

Why is it important/What is the impact to the user?

Because the plugin manager (rightly) refuses to remove plugins that are depended on by other plugins and the plugin manager has historically only allowed one plugin to be removed at a time, the caller removing many plugins has needed to be aware of dependency order.

By allowing a list of plugins to be provided, the plugin manager is able to remove the requirement for dependency-order, and is able to handle the removals in a single execution.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • [ ] I have made corresponding changes to the documentation
  • [ ] I have made corresponding change to the default configuration files (and/or docker env variables)
  • I have added tests that prove my fix is effective or that my feature works

Author's Checklist

  • TODO: file separate docs issue/PR

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

Removal works by _optimistically_ removing all specified plugin-gems from
a mutable gemspec, then validating that doing so does not leave plugin-gems
with orphaned dependencies, saving the result and succeeding only if no
conflicts were created.

> ~~~
> ╭─{ rye@perhaps:~/src/elastic/logstash@main (pluginmanager-remove-multiple ✘) }
> ╰─● bin/logstash-plugin remove logstash-input-syslog logstash-filter-grok
> Using system java: /Users/rye/.jenv/shims/java
> Resolving dependencies......
> Successfully removed logstash-input-syslog
> Successfully removed logstash-filter-grok
> [success (00:00:05)]
~~~
Copy link
Member

@donoghuc donoghuc left a comment

Choose a reason for hiding this comment

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

I know you are still working on some tests, had a couple suggestions in the mean time :)

lib/pluginmanager/bundler/logstash_uninstall.rb Outdated Show resolved Hide resolved
lib/pluginmanager/bundler/logstash_uninstall.rb Outdated Show resolved Hide resolved
lib/pluginmanager/bundler/logstash_uninstall.rb Outdated Show resolved Hide resolved
lib/pluginmanager/bundler/logstash_uninstall.rb Outdated Show resolved Hide resolved
Copy link

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@donoghuc
Copy link
Member

donoghuc commented Feb 7, 2025

Refactors based on initial review look great!

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.

3 participants