plugins: add clean command to remove inactive plugins/dependencies#16998
plugins: add clean command to remove inactive plugins/dependencies#16998yaauie wants to merge 4 commits intoelastic:mainfrom
clean command to remove inactive plugins/dependencies#16998Conversation
donoghuc
left a comment
There was a problem hiding this comment.
Seems pretty straight forward! I like partitioning by plugin gem vs supporting gem dep.
> ~~~
> ╭─{ rye@perhaps:~/src/elastic/logstash@main (pluginmanager-clean-command ✘) }
> ╰─● bin/logstash-plugin remove logstash-integration-aws
> Using system java: /Users/rye/.jenv/shims/java
> Resolving dependencies......
> Resolving dependencies......
> Successfully removed logstash-integration-aws
> [success (00:00:06)]
>
> ╭─{ rye@perhaps:~/src/elastic/logstash@main (pluginmanager-clean-command ✘) }
> ╰─● bin/logstash-plugin clean
> Using system java: /Users/rye/.jenv/shims/java
> cleaned inactive plugin logstash-integration-aws-7.1.8 (java)
> cleaned inactive dependency aws-eventstream (1.3.0)
> cleaned inactive dependency aws-partitions (1.1043.0)
> cleaned inactive dependency aws-sdk-cloudfront (1.109.0)
> cleaned inactive dependency aws-sdk-cloudwatch (1.109.0)
> cleaned inactive dependency aws-sdk-core (3.217.0)
> cleaned inactive dependency aws-sdk-kms (1.97.0)
> cleaned inactive dependency aws-sdk-resourcegroups (1.77.0)
> cleaned inactive dependency aws-sdk-s3 (1.179.0)
> cleaned inactive dependency aws-sdk-sns (1.94.0)
> cleaned inactive dependency aws-sdk-sqs (1.91.0)
> cleaned inactive dependency aws-sigv4 (1.11.0)
> cleaned inactive dependency jar-dependencies (0.4.1)
> cleaned inactive dependency jmespath (1.6.2)
> [success]
> ~~~
a9896fb to
131e22c
Compare
donoghuc
left a comment
There was a problem hiding this comment.
I'm a bit concerned about something... When i do a clean build (check out this code) and run ./gradlew --console=plain clean bootstrap assemble installDefaultGems to get a fresh build. Running the clean command a couple times removes some gems we actually need.... Once that happens I get messages about jar-dependencies not being able to be loaded etc. @yaauie Is this a problem with out I'm building/testing? Or is is this an issue we need to dig further in to?
➜ logstash git:(c4196a9676) ✗ bin/logstash-plugin clean
Using system java: /Users/cas/.jenv/shims/java
cleaned inactive dependency public_suffix (6.0.1)
cleaned inactive dependency ruby-maven (3.9.3)
➜ logstash git:(c4196a9676) ✗ bin/logstash-plugin clean
Using system java: /Users/cas/.jenv/shims/java
cleaned inactive dependency jar-dependencies (0.4.1)
➜ logstash git:(c4196a9676) ✗ bin/logstash-plugin clean
Using system java: /Users/cas/.jenv/shims/java
That said I did some additional manual testing with two example gems a and b:
➜ logstash git:(c4196a9676) ✗ cat a.gemspec
Gem::Specification.new do |s|
s.name = 'a'
s.version = '0.1.1'
s.summary = "A depends on B"
s.authors = ["Test"]
s.files = [__FILE__]
s.add_runtime_dependency "winrm", ">= 0.1"
s.metadata = { "logstash_plugin" => "true" }
end
➜ logstash git:(c4196a9676) ✗ cat b.gemspec
Gem::Specification.new do |s|
s.name = 'b'
s.version = '0.1.1'
s.summary = "B - no dependencies"
s.authors = ["Test"]
s.files = [__FILE__]
s.add_runtime_dependency "erubi", ">= 0"
s.metadata = { "logstash_plugin" => "true" }
endI did combinations of adding/removing and cleaning and everything I could come up with worked as expected!
That is in fact concerning. We're just proxying to bundler, so I don't see why that dependency should get cleaned. I'll dig in. |
lib/pluginmanager/clean.rb
Outdated
| %w( | ||
| full_gem_path | ||
| loaded_from | ||
| spec_file | ||
| cache_file | ||
| build_info_file | ||
| doc_dir | ||
| ).map { |attr| spec.public_send(attr) } | ||
| .compact |
There was a problem hiding this comment.
Probably reads easier, and avoids the metaprogramming indirection of using Object#public_send:
| %w( | |
| full_gem_path | |
| loaded_from | |
| spec_file | |
| cache_file | |
| build_info_file | |
| doc_dir | |
| ).map { |attr| spec.public_send(attr) } | |
| .compact | |
| [ | |
| spec.full_gem_path, | |
| spec.loaded_from, | |
| spec.spec_file, | |
| spec.cache_file, | |
| spec.build_info_file, | |
| spec.doc_dir, | |
| ].compact |
|
I've been questioning the utility of making this a stand alone command. I feel like as a consumer of |
lib/pluginmanager/clean.rb
Outdated
| $stderr.puts("]") | ||
| else | ||
| verb = "cleaned" | ||
| FileUtils.rm_rf(file_list) |
There was a problem hiding this comment.
Removing files on disk seems like the most severe option. I get that bundle clean was not doing exactly what we want, i would think the next level of granularity would be to collect a list of innactive gems then use rubygems to uninstall each (for example ./vendor/jruby/bin/gem uninstall [orphaned-gem].
There was a problem hiding this comment.
It is, and that is effectively what bundle clean does under the hood.
The trouble is that we are invoking a modified Bundler, which is the same Bundler that is managing the dependencies of the current process, and I don't have confidence that invoking the gem binary from jruby will invoke rubygems as-configured to remove the gems from our vendored location without also having side-effects.
I agree, in principle, but I don't know if anyone in the wild has a realistic expectation for the plugins that have been removed to be still present on disk (e.g., airgapped environments where they want to be able to re-install the plugins). I think this stems from us conflating two closely related things, notably:
Our Because NOTE: the I had considered adding a |
|
Correct me if i'm wrong, but it seems like our recent addition of the ability to remove multiple deps in a single invocation of |
|
What is the practical effect and risk of not having a
|
|
cc @jsvd |
|
💚 Build Succeeded
History
|
| Gem::Uninstaller.new(gem_spec.name, removal_options.merge(version: gem_spec.version)).uninstall | ||
| end | ||
| rescue Gem::InstallError => e | ||
| fail "Failed to uninstall `#{gem_spec.full_name}`" |
There was a problem hiding this comment.
Do we want to add anything from the error object here? e.message? Maybe more at debug level?
donoghuc
left a comment
There was a problem hiding this comment.
Love the refactor to use Gem uninistall instead of deleting files out of band. Open question on whether we make this a top level command or just do it after any option that can orphan deps.
|
This pull request does not have a backport label. Could you fix it @yaauie? 🙏
|
|
|
|
Closing in favor of #17203 |
Release notes
bin/logstash-plugin cleancommand to allow the plugin manager to prune orphaned dependenciesWhat does this PR do?
adds
bin/logstash-plugin cleancommand to allow the plugin manager to prune orphaned dependenciesWhy is it important/What is the impact to the user?
cleanoption allows us to build a true subset of the dependency graph after removing plugins that are not needed in a minimalized artifactChecklist
[ ] 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 worksThere are existing tests aroundLogStash::Bunder#invoke!(clean: true)Author's Checklist
mainis lifted.How to test this PR locally
logstash-integration-aws)bin/logstash-plugin remove logstash-integration-awscleancommandbin/logstash-plugin cleanvendorLogs