Merge pull request #11688 from chrisroberts/f-trigger-all

Fix trigger matching on `:all` special value

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

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

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 == :all && !t.ignore.include?(name)) ||
+              (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 == :all && !t.ignore.include?(name)) ||
+              (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
@@ -89,6 +89,7 @@ module Vagrant
               type: type,
               guest_name: guest
           end
+
           filter_triggers(triggers, guest, type)
         end
 

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}")

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

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
 

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

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, 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, all: true)).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, all: true)).to be_empty
+        end
+
+        it "should not locate trigger when after command is ignored" do
+          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, 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, all: true)).not_to be_empty
+        end
+      end
+    end
+
     context "with hook type" do
       before do
         triggers.before(:environment_load, hash_block.merge(type: :hook))