From 916655dbd3dfd49fd38ba58b918deb0c909046eb Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Mon, 8 Jun 2020 17:15:32 -0700 Subject: [PATCH 1/2] Fix trigger matching on `:all` special value Updates the type to do a proper comparison when checking for the `:all` special value as well as applied ignores. Fixes #11599 --- lib/vagrant/plugin/v2/trigger.rb | 5 +- plugins/kernel_v2/config/vm_trigger.rb | 6 +- .../kernel_v2/config/vm_trigger_test.rb | 2 +- test/unit/vagrant/plugin/v2/trigger_test.rb | 61 ++++++++++++++++--- 4 files changed, 60 insertions(+), 14 deletions(-) diff --git a/lib/vagrant/plugin/v2/trigger.rb b/lib/vagrant/plugin/v2/trigger.rb index 0a6919d2c..fda810d11 100644 --- a/lib/vagrant/plugin/v2/trigger.rb +++ b/lib/vagrant/plugin/v2/trigger.rb @@ -72,13 +72,13 @@ module Vagrant if stage == :before triggers = config.before_triggers.select do |t| - (t.command == :all && !t.ignore.include?(name)) || + (t.command.respond_to?(:to_sym) && t.command.to_sym == :all && !t.ignore.include?(name.to_sym)) || (type == :hook && matched_hook?(t.command, name)) || nameify(t.command) == name end elsif stage == :after triggers = config.after_triggers.select do |t| - (t.command == :all && !t.ignore.include?(name)) || + (t.command.respond_to?(:to_sym) && t.command.to_sym == :all && !t.ignore.include?(name.to_sym)) || (type == :hook && matched_hook?(t.command, name)) || nameify(t.command) == name end @@ -89,6 +89,7 @@ module Vagrant type: type, guest_name: guest end + filter_triggers(triggers, guest, type) end diff --git a/plugins/kernel_v2/config/vm_trigger.rb b/plugins/kernel_v2/config/vm_trigger.rb index 182ea3d8a..18f9e3d65 100644 --- a/plugins/kernel_v2/config/vm_trigger.rb +++ b/plugins/kernel_v2/config/vm_trigger.rb @@ -114,7 +114,11 @@ module VagrantPlugins # Internal options @id = SecureRandom.uuid - @command = command.to_s + if command.respond_to?(:to_sym) + @command = command.to_sym + else + @command = command + end @ruby_block = UNSET_VALUE @logger.debug("Trigger defined for: #{command}") diff --git a/test/unit/plugins/kernel_v2/config/vm_trigger_test.rb b/test/unit/plugins/kernel_v2/config/vm_trigger_test.rb index 6fbba226f..6735e219f 100644 --- a/test/unit/plugins/kernel_v2/config/vm_trigger_test.rb +++ b/test/unit/plugins/kernel_v2/config/vm_trigger_test.rb @@ -53,7 +53,7 @@ describe VagrantPlugins::Kernel_V2::VagrantConfigTrigger do it "sets a command" do subject.finalize! - expect(subject.command).to eq(command.to_s) + expect(subject.command).to eq(command) end it "uses default error behavior" do diff --git a/test/unit/vagrant/plugin/v2/trigger_test.rb b/test/unit/vagrant/plugin/v2/trigger_test.rb index 6a4fff094..42b4d541e 100644 --- a/test/unit/vagrant/plugin/v2/trigger_test.rb +++ b/test/unit/vagrant/plugin/v2/trigger_test.rb @@ -23,19 +23,19 @@ describe Vagrant::Plugin::V2::Trigger do ui: ui, } } - let(:triggers) { VagrantPlugins::Kernel_V2::TriggerConfig.new } + let(:triggers) { + @triggers ||= VagrantPlugins::Kernel_V2::TriggerConfig.new.tap do |triggers| + triggers.before(:up, hash_block) + triggers.before(:destroy, hash_block) + triggers.before(:halt, hash_block_two) + triggers.after(:up, hash_block) + triggers.after(:destroy, hash_block) + triggers.finalize! + end + } let(:hash_block) { {info: "hi", run: {inline: "echo 'hi'"}} } let(:hash_block_two) { {warn: "WARNING!!", run_remote: {inline: "echo 'hi'"}} } - before do - triggers.before(:up, hash_block) - triggers.before(:destroy, hash_block) - triggers.before(:halt, hash_block_two) - triggers.after(:up, hash_block) - triggers.after(:destroy, hash_block) - triggers.finalize! - end - let(:subject) { described_class.new(env, triggers, machine, ui) } describe "#fire" do @@ -87,6 +87,47 @@ describe Vagrant::Plugin::V2::Trigger do subject.find(:halt, :before, "guest", :action) end + context "with :all special value" do + let(:triggers) { VagrantPlugins::Kernel_V2::TriggerConfig.new } + let(:ignores) { [] } + + before do + triggers.before(:all, hash_block.merge(ignore: ignores)) + triggers.after(:all, hash_block.merge(ignore: ignores)) + triggers.finalize! + end + + [:destroy, :halt, :provision, :reload, :resume, :suspend, :up].each do |supported_action| + it "should locate trigger when before #{supported_action} action is requested" do + expect(subject.find(supported_action, :before, "guest", :action)).not_to be_empty + end + + it "should locate trigger when after #{supported_action} action is requested" do + expect(subject.find(supported_action, :after, "guest", :action)).not_to be_empty + end + end + + context "with ignores" do + let(:ignores) { [:halt, :up] } + + it "should not locate trigger when before command is ignored" do + expect(subject.find(:up, :before, "guest", :action)).to be_empty + end + + it "should not locate trigger when after command is ignored" do + expect(subject.find(:halt, :after, "guest", :action)).to be_empty + end + + it "should locate trigger when before command is not ignored" do + expect(subject.find(:provision, :before, "guest", :action)).not_to be_empty + end + + it "should locate trigger when after command is not ignored" do + expect(subject.find(:provision, :after, "guest", :action)).not_to be_empty + end + end + end + context "with hook type" do before do triggers.before(:environment_load, hash_block.merge(type: :hook)) From f26440ee38f2417dffe143582ac99da957bba45e Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Fri, 12 Jun 2020 17:09:31 -0700 Subject: [PATCH 2/2] Only allow the all special value to be matched when requested This prevents the all special value from being matched on the non-defined raw action names when the typed triggers support is enabled. --- lib/vagrant/action/builder.rb | 8 ++++---- lib/vagrant/action/builtin/trigger.rb | 5 +++-- lib/vagrant/plugin/v2/trigger.rb | 10 +++++----- test/unit/vagrant/action/builder_test.rb | 2 +- test/unit/vagrant/action/builtin/trigger_test.rb | 4 ++-- test/unit/vagrant/plugin/v2/trigger_test.rb | 12 ++++++------ 6 files changed, 21 insertions(+), 20 deletions(-) diff --git a/lib/vagrant/action/builder.rb b/lib/vagrant/action/builder.rb index 356e3dace..074d512b1 100644 --- a/lib/vagrant/action/builder.rb +++ b/lib/vagrant/action/builder.rb @@ -252,18 +252,18 @@ module Vagrant # and after specific provider actions (like :up, :halt, etc) and # are different from true action triggers if env[:triggers] - if !env[:triggers].find(env[:raw_action_name], :before, machine_name, :action).empty? + if !env[:triggers].find(env[:raw_action_name], :before, machine_name, :action, all: true).empty? hook.prepend(Vagrant::Action::Builtin::Trigger, - env[:raw_action_name], env[:triggers], :before, :action) + env[:raw_action_name], env[:triggers], :before, :action, all: true) end - if !env[:triggers].find(env[:raw_action_name], :after, machine_name, :action).empty? + if !env[:triggers].find(env[:raw_action_name], :after, machine_name, :action, all: true).empty? # NOTE: These after triggers need to be delayed before running to # allow the rest of the call stack to complete before being # run. The delayed action is prepended to the stack (not appended) # to ensure it is called first, which results in it properly # waiting for everything to finish before itself completing. builder = self.class.build(Vagrant::Action::Builtin::Trigger, - env[:raw_action_name], env[:triggers], :after, :action) + env[:raw_action_name], env[:triggers], :after, :action, all: true) hook.prepend(Vagrant::Action::Builtin::Delayed, builder) end end diff --git a/lib/vagrant/action/builtin/trigger.rb b/lib/vagrant/action/builtin/trigger.rb index 0591ea7b4..2fcde82ad 100644 --- a/lib/vagrant/action/builtin/trigger.rb +++ b/lib/vagrant/action/builtin/trigger.rb @@ -8,13 +8,14 @@ module Vagrant # @param [Vagrant::Plugin::V2::Triger] triggers Trigger object # @param [Symbol] timing When trigger should fire (:before/:after) # @param [Symbol] type Type of trigger - def initialize(app, env, name, triggers, timing, type=:action) + def initialize(app, env, name, triggers, timing, type=:action, all: false) @app = app @env = env @triggers = triggers @name = name @timing = timing @type = type + @all = all if ![:before, :after].include?(timing) raise ArgumentError, @@ -26,7 +27,7 @@ module Vagrant machine = env[:machine] machine_name = machine.name if machine - @triggers.fire(@name, @timing, machine_name, @type) + @triggers.fire(@name, @timing, machine_name, @type, all: @all) # Carry on @app.call(env) end diff --git a/lib/vagrant/plugin/v2/trigger.rb b/lib/vagrant/plugin/v2/trigger.rb index fda810d11..99d6ac053 100644 --- a/lib/vagrant/plugin/v2/trigger.rb +++ b/lib/vagrant/plugin/v2/trigger.rb @@ -37,7 +37,7 @@ module Vagrant # @param [Symbol] stage :before or :after # @param [String] guest The guest that invoked firing the triggers # @param [Symbol] type Type of trigger to fire (:action, :hook, :command) - def fire(name, stage, guest, type) + def fire(name, stage, guest, type, all: false) if community_plugin_detected? @logger.warn("Community plugin `vagrant-triggers detected, so core triggers will not fire") return @@ -50,7 +50,7 @@ module Vagrant name = name.to_sym # get all triggers matching action - triggers = find(name, stage, guest, type) + triggers = find(name, stage, guest, type, all: all) if !triggers.empty? @logger.info("Firing trigger for #{type} #{name} on guest #{guest}") @@ -66,19 +66,19 @@ module Vagrant # @param [String] guest The guest that invoked firing the triggers # @param [Symbol] type Type of trigger to fire # @return [Array] - def find(name, stage, guest, type) + def find(name, stage, guest, type, all: false) triggers = nil name = nameify(name) if stage == :before triggers = config.before_triggers.select do |t| - (t.command.respond_to?(:to_sym) && t.command.to_sym == :all && !t.ignore.include?(name.to_sym)) || + (all && t.command.respond_to?(:to_sym) && t.command.to_sym == :all && !t.ignore.include?(name.to_sym)) || (type == :hook && matched_hook?(t.command, name)) || nameify(t.command) == name end elsif stage == :after triggers = config.after_triggers.select do |t| - (t.command.respond_to?(:to_sym) && t.command.to_sym == :all && !t.ignore.include?(name.to_sym)) || + (all && t.command.respond_to?(:to_sym) && t.command.to_sym == :all && !t.ignore.include?(name.to_sym)) || (type == :hook && matched_hook?(t.command, name)) || nameify(t.command) == name end diff --git a/test/unit/vagrant/action/builder_test.rb b/test/unit/vagrant/action/builder_test.rb index 93bf6ec2d..cf87f375e 100644 --- a/test/unit/vagrant/action/builder_test.rb +++ b/test/unit/vagrant/action/builder_test.rb @@ -674,7 +674,7 @@ describe Vagrant::Action::Builder do context "when trigger has been defined for raw action" do before do - expect(triggers).to receive(:find).with(raw_action_name, timing, nil, :action). + expect(triggers).to receive(:find).with(raw_action_name, timing, nil, :action, all: true). and_return([true]) end diff --git a/test/unit/vagrant/action/builtin/trigger_test.rb b/test/unit/vagrant/action/builtin/trigger_test.rb index a389c944c..43b853b7c 100644 --- a/test/unit/vagrant/action/builtin/trigger_test.rb +++ b/test/unit/vagrant/action/builtin/trigger_test.rb @@ -28,7 +28,7 @@ describe Vagrant::Action::Builtin::Trigger do end it "should fire triggers without machine name" do - expect(triggers).to receive(:fire).with(name, timing, nil, type) + expect(triggers).to receive(:fire).with(name, timing, nil, type, anything) subject.call(env) end @@ -36,7 +36,7 @@ describe Vagrant::Action::Builtin::Trigger do let(:machine) { double("machine", name: "machine-name") } it "should include machine name when firing triggers" do - expect(triggers).to receive(:fire).with(name, timing, "machine-name", type) + expect(triggers).to receive(:fire).with(name, timing, "machine-name", type, anything) subject.call(env) end end diff --git a/test/unit/vagrant/plugin/v2/trigger_test.rb b/test/unit/vagrant/plugin/v2/trigger_test.rb index 42b4d541e..811609021 100644 --- a/test/unit/vagrant/plugin/v2/trigger_test.rb +++ b/test/unit/vagrant/plugin/v2/trigger_test.rb @@ -99,11 +99,11 @@ describe Vagrant::Plugin::V2::Trigger do [:destroy, :halt, :provision, :reload, :resume, :suspend, :up].each do |supported_action| it "should locate trigger when before #{supported_action} action is requested" do - expect(subject.find(supported_action, :before, "guest", :action)).not_to be_empty + expect(subject.find(supported_action, :before, "guest", :action, all: true)).not_to be_empty end it "should locate trigger when after #{supported_action} action is requested" do - expect(subject.find(supported_action, :after, "guest", :action)).not_to be_empty + expect(subject.find(supported_action, :after, "guest", :action, all: true)).not_to be_empty end end @@ -111,19 +111,19 @@ describe Vagrant::Plugin::V2::Trigger do let(:ignores) { [:halt, :up] } it "should not locate trigger when before command is ignored" do - expect(subject.find(:up, :before, "guest", :action)).to be_empty + expect(subject.find(:up, :before, "guest", :action, all: true)).to be_empty end it "should not locate trigger when after command is ignored" do - expect(subject.find(:halt, :after, "guest", :action)).to be_empty + expect(subject.find(:halt, :after, "guest", :action, all: true)).to be_empty end it "should locate trigger when before command is not ignored" do - expect(subject.find(:provision, :before, "guest", :action)).not_to be_empty + expect(subject.find(:provision, :before, "guest", :action, all: true)).not_to be_empty end it "should locate trigger when after command is not ignored" do - expect(subject.find(:provision, :after, "guest", :action)).not_to be_empty + expect(subject.find(:provision, :after, "guest", :action, all: true)).not_to be_empty end end end