From a736178d599eba43c8fd7459a972b50aa47e6e97 Mon Sep 17 00:00:00 2001 From: Ry Biesemeyer Date: Wed, 5 Mar 2025 20:38:59 -0800 Subject: [PATCH] Pluginmanager install preserve (#17267) * tests: integration tests for pluginmanager install --preserve * fix regression where pluginmanager's install --preserve flag didn't --- lib/pluginmanager/install.rb | 4 +- qa/integration/specs/cli/install_spec.rb | 69 +++++++++++++++++-- .../specs/cli/pluginmanager_spec_helper.rb | 33 +++++++++ 3 files changed, 98 insertions(+), 8 deletions(-) diff --git a/lib/pluginmanager/install.rb b/lib/pluginmanager/install.rb index e1a351398..d0392f76b 100644 --- a/lib/pluginmanager/install.rb +++ b/lib/pluginmanager/install.rb @@ -214,7 +214,9 @@ class LogStash::PluginManager::Install < LogStash::PluginManager::Command plugin_gem = gemfile.find(plugin) if preserve? puts("Preserving Gemfile gem options for plugin #{plugin}") if plugin_gem && !plugin_gem.options.empty? - gemfile.update(plugin, version, options) + # if the plugin exists and no version was specified, keep the existing requirements + requirements = (plugin_gem && version.nil? ? plugin_gem.requirements : [version]).compact + gemfile.update(plugin, *requirements, options) else gemfile.overwrite(plugin, version, options) end diff --git a/qa/integration/specs/cli/install_spec.rb b/qa/integration/specs/cli/install_spec.rb index c3b55f926..0463fc57d 100644 --- a/qa/integration/specs/cli/install_spec.rb +++ b/qa/integration/specs/cli/install_spec.rb @@ -165,11 +165,11 @@ describe "CLI > logstash-plugin install" do context "rubygems hosted plugin" do include_context "pluginmanager validation helpers" - shared_examples("overwriting existing") do + shared_context("install over existing") do before(:each) do aggregate_failures("precheck") do expect("#{plugin_name}-#{existing_plugin_version}").to_not be_installed_gem - expect("#{plugin_name}-#{specified_plugin_version}").to_not be_installed_gem + expect("#{plugin_name}").to_not be_installed_gem end aggregate_failures("setup") do execute = @logstash_plugin.install(plugin_name, version: existing_plugin_version) @@ -178,9 +178,12 @@ describe "CLI > logstash-plugin install" do expect(execute.exit_code).to eq(0) expect("#{plugin_name}-#{existing_plugin_version}").to be_installed_gem - expect("#{plugin_name}-#{specified_plugin_version}").to_not be_installed_gem + expect(plugin_name).to be_in_gemfile.with_requirements(existing_plugin_version) end end + end + shared_examples("overwriting existing with explicit version") do + include_context "install over existing" it "installs the specified version and removes the pre-existing one" do execute = @logstash_plugin.install(plugin_name, version: specified_plugin_version) @@ -197,20 +200,72 @@ describe "CLI > logstash-plugin install" do end end - context "when installing over an older version" do + context "when installing over an older version using --version" do let(:plugin_name) { "logstash-filter-qatest" } let(:existing_plugin_version) { "0.1.0" } let(:specified_plugin_version) { "0.1.1" } - include_examples "overwriting existing" + include_examples "overwriting existing with explicit version" end - context "when installing over a newer version" do + context "when installing over a newer version using --version" do let(:plugin_name) { "logstash-filter-qatest" } let(:existing_plugin_version) { "0.1.0" } let(:specified_plugin_version) { "0.1.1" } - include_examples "overwriting existing" + include_examples "overwriting existing with explicit version" + end + + context "when installing over existing without --version" do + let(:plugin_name) { "logstash-filter-qatest" } + let(:existing_plugin_version) { "0.1.0" } + + include_context "install over existing" + + context "with --preserve" do + it "succeeds without changing the requirements in the Gemfile" do + execute = @logstash_plugin.install(plugin_name, preserve: true) + + aggregate_failures("command execution") do + expect(execute.stderr_and_stdout).to match(INSTALLATION_SUCCESS_RE) + expect(execute.exit_code).to eq(0) + end + + installed = @logstash_plugin.list(verbose: true) + expect(installed.stderr_and_stdout).to match(/#{Regexp.escape plugin_name}/) + + # we want to ensure that the act of installing an already-installed plugin + # does not change its requirements in the gemfile, and leaves the previously-installed + # version in-tact. + expect(plugin_name).to be_in_gemfile.with_requirements(existing_plugin_version) + expect("#{plugin_name}-#{existing_plugin_version}").to be_installed_gem + end + end + + context "without --preserve" do + # this spec is OBSERVED behaviour, which I believe to be undesirable. + it "succeeds and removes the version requirement from the Gemfile" do + execute = @logstash_plugin.install(plugin_name) + + aggregate_failures("command execution") do + expect(execute.stderr_and_stdout).to match(INSTALLATION_SUCCESS_RE) + expect(execute.exit_code).to eq(0) + end + + installed = @logstash_plugin.list(plugin_name, verbose: true) + expect(installed.stderr_and_stdout).to match(/#{Regexp.escape plugin_name}/) + + # This is the potentially-undesirable surprising behaviour, specified here + # as a means of documentation, not a promise of future behavior. + expect(plugin_name).to be_in_gemfile.without_requirements + + # we expect _a_ version of the plugin to be installed, but cannot be opinionated + # about which version was installed because bundler won't necessarily re-resolve + # the dependency graph to get us an upgrade since the no-requirements dependency + # is still met (but it MAY do so if also installing plugins that are not present). + expect("#{plugin_name}").to be_installed_gem + end + end end context "installing plugin that isn't present" do diff --git a/qa/integration/specs/cli/pluginmanager_spec_helper.rb b/qa/integration/specs/cli/pluginmanager_spec_helper.rb index 6270f7933..4a1b99982 100644 --- a/qa/integration/specs/cli/pluginmanager_spec_helper.rb +++ b/qa/integration/specs/cli/pluginmanager_spec_helper.rb @@ -1,4 +1,5 @@ require 'pathname' +require 'bundler/definition' shared_context "pluginmanager validation helpers" do @@ -40,12 +41,44 @@ shared_context "pluginmanager validation helpers" do end end + matcher :be_in_gemfile do + match do |actual| + common(actual) + @dep && (@version_requirements.nil? || @version_requirements == @dep.requirement) + end + define_method :common do |actual| + @definition = Bundler::Definition.build(logstash_gemfile, logstash_gemfile_lock, false) + @dep = @definition.dependencies.find { |s| s.name == actual } + end + chain :with_requirements do |version_requirements| + @version_requirements = Gem::Requirement.create(version_requirements) + end + chain :without_requirements do + @version_requirements = Gem::Requirement.default + end + failure_message do |actual| + if @dep.nil? + "expected the gem to be in the gemspec, but it wasn't (#{@definition.dependencies.map(&:name)})" + else + "expected the `#{actual}` gem to have requirements `#{@version_requirements}`, but they were `#{@dep.requirement}`" + end + end + end + def logstash_home return super() if defined?(super) return @logstash.logstash_home if @logstash fail("no @logstash, so we can't get logstash_home") end + def logstash_gemfile + Pathname.new(logstash_home) / "Gemfile" + end + + def logstash_gemfile_lock + Pathname.new(logstash_home) / "Gemfile.lock" + end + def logstash_gemdir pathname_base = (Pathname.new(logstash_home) / "vendor" / "bundle" / "jruby") candidate_dirs = pathname_base.glob("[0-9]*")