Merge pull request #11427 from chrisroberts/fix/auto-hook-creation

Update dynamic action hooks implementation

lib/vagrant/action.rb

@@ -2,6 +2,7 @@ require 'vagrant/action/builder'
 
 module Vagrant
   module Action
+    autoload :Hook,        'vagrant/action/hook'
     autoload :Runner,      'vagrant/action/runner'
     autoload :Warden,      'vagrant/action/warden'
 

lib/vagrant/action/warden.rb

@@ -48,20 +48,42 @@ module Vagrant
           action = @actions.shift
           @logger.info("Calling IN action: #{action}")
 
-          if !action.is_a?(Proc) && env[:hook]
-            hook_name = action.class.name.split("::").last.
-              gsub(/([a-z])([A-Z])/, '\1_\2').gsub('-', '_').downcase
+          actions = [action]
+
+          # If this is an action class, set name to check for any defined hooks
+          if !action.is_a?(Proc)
+            hook_name = action.class.name
+            @logger.debug("Performing hooks lookup on #{hook_name} (Action: #{action} - class: #{action.class})")
+            hooks = Vagrant.plugin("2").manager.find_action_hooks(hook_name)
+
+            if !hooks.empty?
+              @logger.debug("Found #{hooks.count} hooks defined for #{hook_name} (Action: #{action})")
+              # Create a new builder for the current action so we can
+              # properly apply the hooks with the correct ordering
+              b = Builder.new
+              b.stack << action
+
+              hooks.each do |hook_proc|
+                hook = Hook.new.tap { |h| hook_proc.call(h) }
+                hook.apply(b)
+              end
+
+              actions = b.stack.map { |a| finalize_action(a, @env) }
+            end
           end
 
-          env[:hook].call("before_#{hook_name}".to_sym) if hook_name
-          @stack.unshift(action).first.call(env)
-          env[:hook].call("after_#{hook_name}".to_sym) if hook_name
+          @logger.info("Calling BEGIN action: #{action}")
+          actions.each do |local_action|
+            @logger.info("Calling IN action: #{local_action} (root: #{action})")
+            @stack.unshift(local_action).first.call(env)
+            raise Errors::VagrantInterrupt if env[:interrupted]
+            @logger.info("Calling OUT action: #{local_action} (root: #{action})")
+          end
 
-          raise Errors::VagrantInterrupt if env[:interrupted]
-          @logger.info("Calling OUT action: #{action}")
-        rescue SystemExit
-          # This means that an "exit" or "abort" was called. In these cases,
-          # we just exit immediately.
+          @logger.info("Calling COMPLETE action: #{action}")
+        rescue SystemExit, NoMemoryError
+          # This means that an "exit" or "abort" was called, or we have run out
+          # of memory. In these cases, we just exit immediately.
           raise
         rescue Exception => e
           # We guard this so that the Warden only outputs this once for

lib/vagrant/plugin/v2/manager.rb

@@ -28,6 +28,55 @@ module Vagrant
           result
         end
 
+        # Find all hooks that are applicable for the given key. This
+        # lookup does not include hooks which are defined for ALL_ACTIONS.
+        # Key lookups will match on either string or symbol values. The
+        # provided keys is broken down into multiple parts for lookups,
+        # which allows defining hooks with an entire namespaced name,
+        # or a short suffx. For example:
+        #
+        #  Assume we are given an action class
+        #    key = Vagrant::Action::Builtin::SyncedFolders
+        #
+        #  The list of keys that will be checked for hooks:
+        #    ["Vagrant::Action::Builtin::SyncedFolders", "vagrant_action_builtin_synced_folders",
+        #     "Action::Builtin::SyncedFolders", "action_builtin_synced_folders",
+        #     "Builtin::SyncedFolders", "builtin_synced_folders",
+        #     "SyncedFolders", "synced_folders"]
+        #
+        # @param key [Class, String] key Key for hook lookups
+        # @return [Array<Proc>]
+        def find_action_hooks(key)
+          result = []
+
+          generate_hook_keys(key).each do |k|
+            @registered.each do |plugin|
+              result += plugin.components.action_hooks[k]
+              result += plugin.components.action_hooks[k.to_sym]
+            end
+          end
+
+          result
+        end
+
+        # Generate all valid lookup keys for given key
+        #
+        # @param [Class, String] key Base key for generation
+        # @return [Array<String>] all valid keys
+        def generate_hook_keys(key)
+          key = key.is_a?(Class) ? key.name : key.to_s
+          parts = key.split("::")
+          [].tap do |keys|
+            until parts.empty?
+              x = parts.join("::")
+              keys << x
+              y = x.gsub(/([a-z])([A-Z])/, '\1_\2').gsub('::', '_').downcase
+              keys << y if x != y
+              parts.shift
+            end
+          end
+        end
+
         # This returns all the registered commands.
         #
         # @return [Registry<Symbol, Array<Proc, Hash>>]

plugins/guests/darwin/cap/mount_vmware_shared_folder.rb

@@ -4,6 +4,22 @@ module VagrantPlugins
   module GuestDarwin
     module Cap
       class MountVmwareSharedFolder
+        # Entry point for hook to called delayed actions
+        # which finalizing the synced folders setup on
+        # the guest
+        def self.write_apfs_firmlinks(env)
+          if env && env[:machine] && delayed = apfs_firmlinks_delayed.delete(env[:machine].id)
+            delayed.call
+          end
+        end
+
+        # @return [Hash] storage location for delayed actions
+        def self.apfs_firmlinks_delayed
+          if !@_apfs_firmlinks
+            @_apfs_firmlinks = {}
+          end
+          @_apfs_firmlinks
+        end
 
         # we seem to be unable to ask 'mount -t vmhgfs' to mount the roots
         # of specific shares, so instead we symlink from what is already
@@ -61,17 +77,14 @@ module VagrantPlugins
 
               # If we haven't already added our hook to apply firmlinks, do it now
               if @apply_firmlinks[machine.id][:content].empty?
-                Plugin.action_hook(:apfs_firmlinks, :after_synced_folders) do |hook|
-                  action = proc { |*_|
-                    content = @apply_firmlinks[machine.id][:content].join("\n")
-                    # Write out the synthetic file
-                    comm.sudo("echo -e #{content.inspect} > /etc/synthetic.conf")
-                    if @apply_firmlinks[:bootstrap]
-                      # Re-bootstrap the root container to pick up firmlink updates
-                      comm.sudo("/System/Library/Filesystems/apfs.fs/Contents/Resources/apfs.util -B")
-                    end
-                  }
-                  hook.prepend(action)
+                apfs_firmlinks_delayed[machine.id] = proc do
+                  content = @apply_firmlinks[machine.id][:content].join("\n")
+                  # Write out the synthetic file
+                  comm.sudo("echo -e #{content.inspect} > /etc/synthetic.conf")
+                  if @apply_firmlinks[:bootstrap]
+                    # Re-bootstrap the root container to pick up firmlink updates
+                    comm.sudo("/System/Library/Filesystems/apfs.fs/Contents/Resources/apfs.util -B")
+                  end
                 end
               end
               @apply_firmlinks[machine.id][:content] << share_line

plugins/guests/darwin/plugin.rb

@@ -6,6 +6,11 @@ module VagrantPlugins
       name "Darwin guest"
       description "Darwin guest support."
 
+      action_hook(:apfs_firmlinks, :synced_folders) do |hook|
+        require_relative "cap/mount_vmware_shared_folder"
+        hook.append(Cap::MountVmwareSharedFolder.method(:write_apfs_firmlinks))
+      end
+
       guest(:darwin, :bsd)  do
         require_relative "guest"
         Guest

test/unit/plugins/guests/darwin/cap/mount_vmware_shared_folder_test.rb

@@ -39,10 +39,6 @@ describe "VagrantPlugins::GuestDarwin::Cap::MountVmwareSharedFolder" do
         expect(communicator).to receive(:test).with(/synthetic\.conf/)
       end
 
-      it "should register an action hook" do
-        expect(VagrantPlugins::GuestDarwin::Plugin).to receive(:action_hook).with(:apfs_firmlinks, :after_synced_folders)
-      end
-
       context "with guest path within existing directory" do
         let(:guestpath) { "/Users/vagrant/workspace" }
 
@@ -77,10 +73,6 @@ describe "VagrantPlugins::GuestDarwin::Cap::MountVmwareSharedFolder" do
           expect(communicator).to receive(:sudo).with(%r{ln -s .+/System/Volumes/Data.+})
         end
 
-        it "should register an action hook" do
-          expect(VagrantPlugins::GuestDarwin::Plugin).to receive(:action_hook).with(:apfs_firmlinks, :after_synced_folders)
-        end
-
         context "when firmlink is provided by the system" do
           before { expect(described_class).to receive(:system_firmlink?).and_return(true) }
 
@@ -164,4 +156,55 @@ describe "VagrantPlugins::GuestDarwin::Cap::MountVmwareSharedFolder" do
       end
     end
   end
+
+  describe ".write_apfs_firmlinks" do
+    let(:env) { nil }
+    let(:action) { double("action", call: nil) }
+    let(:machine) { double("machine", id: machine_id) }
+    let(:machine_id) { double("machine_id") }
+    let(:delayed) { {} }
+
+    context "when env is nil" do
+      it "should be a no-op" do
+        expect(described_class).not_to receive(:apfs_firmlinks_delayed)
+        described_class.write_apfs_firmlinks(env)
+      end
+    end
+
+    context "when env is empty hash" do
+      let(:env) { {} }
+
+      it "should be a no-op" do
+        expect(described_class).not_to receive(:apfs_firmlinks_delayed)
+        described_class.write_apfs_firmlinks(env)
+      end
+    end
+
+    context "when machine is defined within env" do
+      let(:env) { {machine: machine} }
+
+      it "should request the stored delayed actions" do
+        expect(described_class).to receive(:apfs_firmlinks_delayed).and_return(delayed)
+        described_class.write_apfs_firmlinks(env)
+      end
+    end
+
+    context "when delayed action is stored for machine" do
+      let(:env) { {machine: machine} }
+
+      before { described_class.apfs_firmlinks_delayed[machine_id] = action }
+      after { described_class.apfs_firmlinks_delayed.clear }
+
+      it "should call the delayed action" do
+        expect(action).to receive(:call)
+        described_class.write_apfs_firmlinks(env)
+      end
+
+      it "should remove the action after calling" do
+        expect(action).to receive(:call)
+        described_class.write_apfs_firmlinks(env)
+        expect(delayed).to be_empty
+      end
+    end
+  end
 end

test/unit/vagrant/action/warden_test.rb

@@ -7,6 +7,7 @@ describe Vagrant::Action::Warden do
     end
 
     def call(env)
+      env[:data] << 1 if env[:data]
       @app.call(env)
     end
 
@@ -21,6 +22,7 @@ describe Vagrant::Action::Warden do
     end
 
     def call(env)
+      env[:data] << 2 if env[:data]
       @app.call(env)
     end
 
@@ -90,35 +92,146 @@ describe Vagrant::Action::Warden do
     expect(data.key?(:recover)).not_to be
   end
 
-  context "when hook is defined" do
-    let(:hook) { double("hook") }
+  it "does not do a recovery sequence if NoMemoryError is raised" do
+    error_proc = Proc.new { raise NoMemoryError }
 
-    before do
-      data[:hook] = hook
-      allow(hook).to receive(:call)
+    instance = described_class.new([ExitAction, error_proc], data)
+
+    # The SystemExit should come through
+    expect { instance.call(data) }.to raise_error(NoMemoryError)
+
+    # The recover should not have been called
+    expect(data.key?(:recover)).not_to be
+  end
+
+  describe "dynamic action hooks" do
+    let(:data) { {data: []} }
+    let(:hook_action_name) { :action_two }
+
+    let(:plugin) do
+      h_name = hook_action_name
+      @plugin ||= Class.new(Vagrant.plugin("2")) do
+        name "Test Plugin"
+        action_hook(:before_test, h_name) do |hook|
+          hook.prepend(proc{ |env| env[:data] << :first })
+        end
+      end
     end
 
-    it "should receive a before hook call" do
-      expect(hook).to receive(:call).with(:before_action_one)
-      described_class.new([ActionOne], data).call(data)
+    before { plugin }
+
+    after do
+      Vagrant.plugin("2").manager.unregister(@plugin) if @plugin
+      @plugin = nil
     end
 
-    it "should receive an after hook call" do
-      expect(hook).to receive(:call).with(:after_action_one)
-      described_class.new([ActionOne], data).call(data)
+    it "should call hook before running action" do
+      instance = described_class.new([ActionTwo], data)
+      instance.call(data)
+      expect(data[:data].first).to eq(:first)
+      expect(data[:data].last).to eq(2)
     end
 
-    it "should not receive any hook calls for proc instances" do
-      expect(hook).not_to receive(:call)
-      described_class.new([proc{|*_| :testing }], data).call(data)
+    context "when hook is appending to action" do
+      let(:plugin) do
+        @plugin ||= Class.new(Vagrant.plugin("2")) do
+          name "Test Plugin"
+          action_hook(:before_test, :action_two) do |hook|
+            hook.append(proc{ |env| env[:data] << :first })
+          end
+        end
+      end
+
+      it "should call hook after action when action is nested" do
+        instance = described_class.new([described_class.new([ActionTwo], data), ActionOne], data)
+        instance.call(data)
+        expect(data[:data][0]).to eq(2)
+        expect(data[:data][1]).to eq(:first)
+        expect(data[:data][2]).to eq(1)
+      end
     end
 
-    it "should receive before and after calls for each class" do
-      expect(hook).to receive(:call).with(:before_action_one)
-      expect(hook).to receive(:call).with(:after_action_one)
-      expect(hook).to receive(:call).with(:before_action_two)
-      expect(hook).to receive(:call).with(:after_action_two)
-      described_class.new([ActionOne, proc{|*_| :testing }, ActionTwo], data).call(data)
+    context "when hook uses class name" do
+      let(:hook_action_name) { "ActionTwo" }
+
+      it "should execute the hook" do
+        instance = described_class.new([ActionTwo], data)
+        instance.call(data)
+        expect(data[:data]).to include(:first)
+      end
+    end
+
+    context "when action includes a namespace" do
+      module Vagrant
+        module Test
+          class ActionTest
+            def initialize(app, env)
+              @app = app
+            end
+
+            def call(env)
+              env[:data] << :test if env[:data]
+              @app.call(env)
+            end
+          end
+        end
+      end
+
+      let(:instance) { described_class.new([Vagrant::Test::ActionTest], data) }
+
+      context "when hook uses short snake case name" do
+        let(:hook_action_name) { :action_test }
+
+        it "should execute the hook" do
+          instance.call(data)
+          expect(data[:data]).to include(:first)
+        end
+      end
+
+      context "when hook uses partial snake case name" do
+        let(:hook_action_name) { :test_action_test }
+
+        it "should execute the hook" do
+          instance.call(data)
+          expect(data[:data]).to include(:first)
+        end
+      end
+
+      context "when hook uses full snake case name" do
+        let(:hook_action_name) { :vagrant_test_action_test }
+
+        it "should execute the hook" do
+          instance.call(data)
+          expect(data[:data]).to include(:first)
+        end
+      end
+
+      context "when hook uses short class name" do
+        let(:hook_action_name) { "ActionTest" }
+
+        it "should execute the hook" do
+          instance.call(data)
+          expect(data[:data]).to include(:first)
+        end
+      end
+
+      context "when hook uses partial namespace class name" do
+        let(:hook_action_name) { "Test::ActionTest" }
+
+        it "should execute the hook" do
+          instance.call(data)
+          expect(data[:data]).to include(:first)
+        end
+      end
+
+      context "when hook uses full namespace class name" do
+        let(:hook_action_name) { "Vagrant::Test::ActionTest" }
+
+        it "should execute the hook" do
+          instance.call(data)
+          expect(data[:data]).to include(:first)
+        end
+      end
     end
   end
 end

test/unit/vagrant/plugin/v2/manager_test.rb

@@ -11,6 +11,122 @@ describe Vagrant::Plugin::V2::Manager do
     p
   end
 
+  describe "#generate_hook_keys" do
+    it "should return array with one value" do
+      expect(subject.generate_hook_keys(:test_value)).to eq(["test_value"])
+    end
+
+    it "should return array with two values when key is camel cased" do
+      result = subject.generate_hook_keys("TestValue")
+      expect(result.size).to eq(2)
+      expect(result).to include("TestValue")
+      expect(result).to include("test_value")
+    end
+
+    it "should handle class/module value" do
+      result = subject.generate_hook_keys(Vagrant)
+      expect(result.size).to eq(2)
+      expect(result).to include("Vagrant")
+      expect(result).to include("vagrant")
+    end
+
+    it "should handle namespaced value" do
+      result = subject.generate_hook_keys(Vagrant::Plugin)
+      expect(result.size).to eq(4)
+      expect(result).to include("Vagrant::Plugin")
+      expect(result).to include("Plugin")
+      expect(result).to include("vagrant_plugin")
+      expect(result).to include("plugin")
+    end
+  end
+
+  describe "#find_action_hooks" do
+    let(:hook_name) { "Vagrant::Plugin" }
+
+    before do
+      h_name = hook_name
+      pA = plugin do |p|
+        p.action_hook(:test, h_name) { "hook_called" }
+      end
+      subject.register(pA)
+    end
+
+    it "should find hook with full namespace" do
+      hooks = subject.find_action_hooks(Vagrant::Plugin)
+      expect(hooks).not_to be_empty
+      expect(hooks.first.call).to eq("hook_called")
+    end
+
+    it "should not find hook with short class name" do
+      hooks = subject.find_action_hooks("Plugin")
+      expect(hooks).to be_empty
+    end
+
+    it "should not find hook with full snake cased name" do
+      hooks = subject.find_action_hooks(:vagrant_plugin)
+      expect(hooks).to be_empty
+    end
+
+    it "should not find hook with short snake cased name" do
+      hooks = subject.find_action_hooks("plugin")
+      expect(hooks).to be_empty
+    end
+
+    context "when hook uses full snake cased name" do
+      let(:hook_name) { :vagrant_plugin }
+
+      it "should find hook with full namespace" do
+        hooks = subject.find_action_hooks(Vagrant::Plugin)
+        expect(hooks).not_to be_empty
+        expect(hooks.first.call).to eq("hook_called")
+      end
+
+      it "should find hook with full snake cased name" do
+        hooks = subject.find_action_hooks(:vagrant_plugin)
+        expect(hooks).not_to be_empty
+        expect(hooks.first.call).to eq("hook_called")
+      end
+
+      it "should not find hook with short class name" do
+        hooks = subject.find_action_hooks("Plugin")
+        expect(hooks).to be_empty
+      end
+
+      it "should not find hook with short snake cased name" do
+        hooks = subject.find_action_hooks("plugin")
+        expect(hooks).to be_empty
+      end
+    end
+
+    context "when hook uses short snake cased name" do
+      let(:hook_name) { :plugin }
+
+      it "should find hook with full namespace" do
+        hooks = subject.find_action_hooks(Vagrant::Plugin)
+        expect(hooks).not_to be_empty
+        expect(hooks.first.call).to eq("hook_called")
+      end
+
+      it "should find hook with short class name" do
+        hooks = subject.find_action_hooks("Plugin")
+        expect(hooks).not_to be_empty
+        expect(hooks.first.call).to eq("hook_called")
+      end
+
+      it "should find hook with short snake cased name" do
+        hooks = subject.find_action_hooks("plugin")
+        expect(hooks).not_to be_empty
+        expect(hooks.first.call).to eq("hook_called")
+      end
+
+      it "should not find hook with full snake cased name" do
+        hooks = subject.find_action_hooks(:vagrant_plugin)
+        expect(hooks).to be_empty
+      end
+    end
+
+  end
+
   describe "#action_hooks" do
     it "should contain globally registered hooks" do
       pA = plugin do |p|