From 80a7c8a0cba3d9f44f69f7a716166f101f0bef33 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Wed, 6 Feb 2013 14:21:31 -0800 Subject: [PATCH 1/8] Hook class --- lib/vagrant/action/hook.rb | 53 +++++++++++++++++++++++++++ lib/vagrant/action/warden.rb | 6 +-- test/unit/vagrant/action/hook_test.rb | 52 ++++++++++++++++++++++++++ 3 files changed, 108 insertions(+), 3 deletions(-) create mode 100644 lib/vagrant/action/hook.rb create mode 100644 test/unit/vagrant/action/hook_test.rb diff --git a/lib/vagrant/action/hook.rb b/lib/vagrant/action/hook.rb new file mode 100644 index 000000000..ef8f2992d --- /dev/null +++ b/lib/vagrant/action/hook.rb @@ -0,0 +1,53 @@ +module Vagrant + module Action + # This class manages hooks into existing {Builder} stacks, and lets you + # add and remove middleware classes. This is the primary method by which + # plugins can hook into built-in middleware stacks. + class Hook + # This is a hash of the middleware to prepend to a certain + # other middleware. + # + # @return [Hash>] + attr_reader :before_hooks + + # This is a hash of the middleware to append to a certain other + # middleware. + # + # @return [Hash>] + attr_reader :after_hooks + + # This is a list of the hooks to just prepend to the beginning + # + # @return [Array] + attr_reader :prepend_hooks + + # This is a list of the hooks to just append to the end + # + # @return [Array] + attr_reader :append_hooks + + def initialize + @before_hooks = Hash.new { |h, k| h[k] = [] } + @after_hooks = Hash.new { |h, k| h[k] = [] } + @prepend_hooks = [] + @append_hooks = [] + end + + def before(existing, new) + @before_hooks[existing] << new + end + + def after(existing, new) + @after_hooks[existing] << new + end + + def append(new) + @append_hooks << new + end + + def prepend(new) + @prepend_hooks << new + end + end + end +end diff --git a/lib/vagrant/action/warden.rb b/lib/vagrant/action/warden.rb index 5629a13f2..d01b88caf 100644 --- a/lib/vagrant/action/warden.rb +++ b/lib/vagrant/action/warden.rb @@ -16,9 +16,9 @@ module Vagrant attr_accessor :actions, :stack def initialize(actions, env) - @stack = [] - @actions = actions.map { |m| finalize_action(m, env) } - @logger = Log4r::Logger.new("vagrant::action::warden") + @stack = [] + @actions = actions.map { |m| finalize_action(m, env) } + @logger = Log4r::Logger.new("vagrant::action::warden") @last_error = nil end diff --git a/test/unit/vagrant/action/hook_test.rb b/test/unit/vagrant/action/hook_test.rb new file mode 100644 index 000000000..f944c3a54 --- /dev/null +++ b/test/unit/vagrant/action/hook_test.rb @@ -0,0 +1,52 @@ +require File.expand_path("../../../base", __FILE__) + +require "vagrant/action/hook" + +describe Vagrant::Action::Hook do + describe "defaults" do + its("after_hooks") { should be_empty } + its("before_hooks") { should be_empty } + its("append_hooks") { should be_empty } + its("prepend_hooks") { should be_empty } + end + + describe "before hooks" do + let(:existing) { "foo" } + + it "should append them" do + subject.before(existing, 1) + subject.before(existing, 2) + + subject.before_hooks[existing].should == [1, 2] + end + end + + describe "after hooks" do + let(:existing) { "foo" } + + it "should append them" do + subject.after(existing, 1) + subject.after(existing, 2) + + subject.after_hooks[existing].should == [1, 2] + end + end + + describe "append" do + it "should make a list" do + subject.append(1) + subject.append(2) + + subject.append_hooks.should == [1, 2] + end + end + + describe "prepend" do + it "should make a list" do + subject.prepend(1) + subject.prepend(2) + + subject.prepend_hooks.should == [1, 2] + end + end +end From e822aac931e82606948b147e5d8aba8e11a66a71 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Wed, 6 Feb 2013 14:25:36 -0800 Subject: [PATCH 2/8] Convert builder tests to use rspec "subjects" --- test/unit/vagrant/action/builder_test.rb | 83 ++++++++++++------------ 1 file changed, 41 insertions(+), 42 deletions(-) diff --git a/test/unit/vagrant/action/builder_test.rb b/test/unit/vagrant/action/builder_test.rb index ee00e0818..b3d65410b 100644 --- a/test/unit/vagrant/action/builder_test.rb +++ b/test/unit/vagrant/action/builder_test.rb @@ -2,7 +2,6 @@ require File.expand_path("../../../base", __FILE__) describe Vagrant::Action::Builder do let(:data) { { :data => [] } } - let(:instance) { described_class.new } # This returns a proc that can be used with the builder # that simply appends data to an array in the env. @@ -15,8 +14,8 @@ describe Vagrant::Action::Builder do data = {} proc = Proc.new { |env| env[:data] = true } - instance = described_class.build(proc) - instance.call(data) + subject = described_class.build(proc) + subject.call(data) data[:data].should == true end @@ -27,8 +26,8 @@ describe Vagrant::Action::Builder do data = {} proc = Proc.new { |env| env[:data] = true } - instance.use proc - instance.call(data) + subject.use proc + subject.call(data) data[:data].should == true end @@ -38,9 +37,9 @@ describe Vagrant::Action::Builder do proc1 = Proc.new { |env| env[:one] = true } proc2 = Proc.new { |env| env[:two] = true } - instance.use proc1 - instance.use proc2 - instance.call(data) + subject.use proc1 + subject.use proc2 + subject.call(data) data[:one].should == true data[:two].should == true @@ -66,9 +65,9 @@ describe Vagrant::Action::Builder do context "inserting" do it "can insert at an index" do - instance.use appender_proc(1) - instance.insert(0, appender_proc(2)) - instance.call(data) + subject.use appender_proc(1) + subject.insert(0, appender_proc(2)) + subject.call(data) data[:data].should == [2, 1] end @@ -78,48 +77,48 @@ describe Vagrant::Action::Builder do bar_proc = appender_proc(2) def bar_proc.name; :bar; end - instance.use appender_proc(1) - instance.use bar_proc - instance.insert_before :bar, appender_proc(3) - instance.call(data) + subject.use appender_proc(1) + subject.use bar_proc + subject.insert_before :bar, appender_proc(3) + subject.call(data) data[:data].should == [1, 3, 2] end it "can insert next to a previous object" do proc2 = appender_proc(2) - instance.use appender_proc(1) - instance.use proc2 - instance.insert(proc2, appender_proc(3)) - instance.call(data) + subject.use appender_proc(1) + subject.use proc2 + subject.insert(proc2, appender_proc(3)) + subject.call(data) data[:data].should == [1, 3, 2] end it "can insert before" do - instance.use appender_proc(1) - instance.insert_before 0, appender_proc(2) - instance.call(data) + subject.use appender_proc(1) + subject.insert_before 0, appender_proc(2) + subject.call(data) data[:data].should == [2, 1] end it "can insert after" do - instance.use appender_proc(1) - instance.use appender_proc(3) - instance.insert_after 0, appender_proc(2) - instance.call(data) + subject.use appender_proc(1) + subject.use appender_proc(3) + subject.insert_after 0, appender_proc(2) + subject.call(data) data[:data].should == [1, 2, 3] end it "raises an exception if an invalid object given for insert" do - expect { instance.insert "object", appender_proc(1) }. + expect { subject.insert "object", appender_proc(1) }. to raise_error(RuntimeError) end it "raises an exception if an invalid object given for insert_after" do - expect { instance.insert_after "object", appender_proc(1) }. + expect { subject.insert_after "object", appender_proc(1) }. to raise_error(RuntimeError) end end @@ -129,9 +128,9 @@ describe Vagrant::Action::Builder do proc1 = appender_proc(1) proc2 = appender_proc(2) - instance.use proc1 - instance.replace proc1, proc2 - instance.call(data) + subject.use proc1 + subject.replace proc1, proc2 + subject.call(data) data[:data].should == [2] end @@ -140,9 +139,9 @@ describe Vagrant::Action::Builder do proc1 = appender_proc(1) proc2 = appender_proc(2) - instance.use proc1 - instance.replace 0, proc2 - instance.call(data) + subject.use proc1 + subject.replace 0, proc2 + subject.call(data) data[:data].should == [2] end @@ -152,10 +151,10 @@ describe Vagrant::Action::Builder do it "can delete by object" do proc1 = appender_proc(1) - instance.use proc1 - instance.use appender_proc(2) - instance.delete proc1 - instance.call(data) + subject.use proc1 + subject.use appender_proc(2) + subject.delete proc1 + subject.call(data) data[:data].should == [2] end @@ -163,10 +162,10 @@ describe Vagrant::Action::Builder do it "can delete by index" do proc1 = appender_proc(1) - instance.use proc1 - instance.use appender_proc(2) - instance.delete 0 - instance.call(data) + subject.use proc1 + subject.use appender_proc(2) + subject.delete 0 + subject.call(data) data[:data].should == [2] end From 9251b880f5eb533c45ca14462df342d3b190186e Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Wed, 6 Feb 2013 14:30:15 -0800 Subject: [PATCH 3/8] Addition docs on the hook class --- lib/vagrant/action/hook.rb | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/lib/vagrant/action/hook.rb b/lib/vagrant/action/hook.rb index ef8f2992d..0d89048e3 100644 --- a/lib/vagrant/action/hook.rb +++ b/lib/vagrant/action/hook.rb @@ -33,18 +33,34 @@ module Vagrant @append_hooks = [] end + # Add a middleware before an existing middleware. + # + # @param [Class] existing The existing middleware. + # @param [Class] new The new middleware. def before(existing, new) @before_hooks[existing] << new end + # Add a middleware after an existing middleware. + # + # @param [Class] existing The existing middleware. + # @param [Class] new The new middleware. def after(existing, new) @after_hooks[existing] << new end + # Append a middleware to the end of the stack. Note that if the + # middleware sequence ends early, then the new middleware won't + # be run. + # + # @param [Class] new The middleware to append. def append(new) @append_hooks << new end + # Prepend a middleware to the beginning of the stack. + # + # @param [Class] new The new middleware to prepend. def prepend(new) @prepend_hooks << new end From d720205810916d3dbdd46f189fc89a40650f0165 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Wed, 6 Feb 2013 15:06:13 -0800 Subject: [PATCH 4/8] Builder supports action hooks --- lib/vagrant/action/builder.rb | 48 ++++++++++++++++++------ test/unit/vagrant/action/builder_test.rb | 23 ++++++++++++ 2 files changed, 60 insertions(+), 11 deletions(-) diff --git a/lib/vagrant/action/builder.rb b/lib/vagrant/action/builder.rb index 23d099a5d..5e21adb5c 100644 --- a/lib/vagrant/action/builder.rb +++ b/lib/vagrant/action/builder.rb @@ -17,6 +17,12 @@ module Vagrant # Vagrant::Action.run(app) # class Builder + # This is the stack of middlewares added. This should NOT be used + # directly. + # + # @return [Array] + attr_reader :stack + # This is a shortcut for a middleware sequence with only one item # in it. For a description of the arguments and the documentation, please # see {#use} instead. @@ -26,6 +32,18 @@ module Vagrant new.use(middleware, *args, &block) end + def initialize + @stack = [] + end + + # Implement a custom copy that copies the stack variable over so that + # we don't clobber that. + def initialize_copy(original) + super + + @stack = original.stack.dup + end + # Returns a mergeable version of the builder. If `use` is called with # the return value of this method, then the stack will merge, instead # of being treated as a separate single middleware. @@ -109,19 +127,27 @@ module Vagrant # @param [Vagrant::Action::Environment] env The action environment # @return [Object] A callable object def to_app(env) + app_stack = nil + + # If we have action hooks, then we apply them + if env[:action_hooks] + builder = self.dup + + # Apply all the hooks to the new builder instance + env[:action_hooks].each do |hook| + hook.apply(builder) + end + + # The stack is now the result of the new builder + app_stack = builder.stack.dup + end + + # If we don't have a stack then default to using our own + app_stack ||= stack.dup + # Wrap the middleware stack with the Warden to provide a consistent # and predictable behavior upon exceptions. - Warden.new(stack.dup, env) - end - - protected - - # Returns the current stack of middlewares. You probably won't - # need to use this directly, and it's recommended that you don't. - # - # @return [Array] - def stack - @stack ||= [] + Warden.new(app_stack, env) end end end diff --git a/test/unit/vagrant/action/builder_test.rb b/test/unit/vagrant/action/builder_test.rb index b3d65410b..a4fed47d5 100644 --- a/test/unit/vagrant/action/builder_test.rb +++ b/test/unit/vagrant/action/builder_test.rb @@ -9,6 +9,13 @@ describe Vagrant::Action::Builder do Proc.new { |env| env[:data] << data } end + context "copying" do + it "should copy the stack" do + copy = subject.dup + copy.stack.object_id.should_not == subject.stack.object_id + end + end + context "build" do it "should provide build as a shortcut for basic sequences" do data = {} @@ -170,4 +177,20 @@ describe Vagrant::Action::Builder do data[:data].should == [2] end end + + describe "action hooks" do + it "applies them properly" do + hook = double("hook") + hook.stub(:apply) do |builder| + builder.use appender_proc(2) + end + + data[:action_hooks] = [hook] + + subject.use appender_proc(1) + subject.call(data) + + data[:data].should == [1, 2] + end + end end From 83bba789a447469767992ecd1dfb5e7961daf55a Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Wed, 6 Feb 2013 15:21:34 -0800 Subject: [PATCH 5/8] Hook#apply --- lib/vagrant/action/hook.rb | 34 +++++++++++++++++++++++++++ test/unit/vagrant/action/hook_test.rb | 16 +++++++++++++ 2 files changed, 50 insertions(+) diff --git a/lib/vagrant/action/hook.rb b/lib/vagrant/action/hook.rb index 0d89048e3..e7ac56cc8 100644 --- a/lib/vagrant/action/hook.rb +++ b/lib/vagrant/action/hook.rb @@ -64,6 +64,40 @@ module Vagrant def prepend(new) @prepend_hooks << new end + + # This applies the given hook to a builder. This should not be + # called directly. + # + # @param [Builder] builder + def apply(builder) + # Prepends first + @prepend_hooks.each do |klass| + builder.insert(0, klass) + end + + # Appends + @append_hooks.each do |klass| + builder.use(klass) + end + + # Before hooks + @before_hooks.each do |key, list| + next if !builder.index(key) + + list.each do |klass| + builder.insert_before(key, klass) + end + end + + # After hooks + @after_hooks.each do |key, list| + next if !builder.index(key) + + list.each do |klass| + builder.insert_after(key, klass) + end + end + end end end end diff --git a/test/unit/vagrant/action/hook_test.rb b/test/unit/vagrant/action/hook_test.rb index f944c3a54..27a22a271 100644 --- a/test/unit/vagrant/action/hook_test.rb +++ b/test/unit/vagrant/action/hook_test.rb @@ -1,5 +1,6 @@ require File.expand_path("../../../base", __FILE__) +require "vagrant/action/builder" require "vagrant/action/hook" describe Vagrant::Action::Hook do @@ -49,4 +50,19 @@ describe Vagrant::Action::Hook do subject.prepend_hooks.should == [1, 2] end end + + describe "applying" do + let(:builder) { Vagrant::Action::Builder.new } + + it "should build the proper stack" do + subject.prepend("1") + subject.append("9") + subject.after("1", "2") + subject.before("9", "8") + + subject.apply(builder) + + builder.stack.map(&:first).should == %w[1 2 8 9] + end + end end From aa7193471fe145b7ad85625ff6584246fb3c0f01 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Wed, 6 Feb 2013 15:27:14 -0800 Subject: [PATCH 6/8] Plugins can define action hooks via action_hook --- lib/vagrant/plugin/v2/components.rb | 7 +++++++ lib/vagrant/plugin/v2/plugin.rb | 12 +++--------- test/unit/vagrant/plugin/v2/plugin_test.rb | 2 +- 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/lib/vagrant/plugin/v2/components.rb b/lib/vagrant/plugin/v2/components.rb index 2301656da..a9a7e50f7 100644 --- a/lib/vagrant/plugin/v2/components.rb +++ b/lib/vagrant/plugin/v2/components.rb @@ -6,12 +6,19 @@ module Vagrant # components, and the actual container of those components. This # removes a bit of state overhead from the plugin class itself. class Components + # This contains all the action hooks. + # + # @return [Array] + attr_reader :action_hooks + # This contains all the configuration plugins by scope. # # @return [Hash] attr_reader :configs def initialize + @action_hooks = [] + # Create the configs hash which defaults to a registry @configs = Hash.new { |h, k| h[k] = Registry.new } end diff --git a/lib/vagrant/plugin/v2/plugin.rb b/lib/vagrant/plugin/v2/plugin.rb index ff5c38100..2246895dc 100644 --- a/lib/vagrant/plugin/v2/plugin.rb +++ b/lib/vagrant/plugin/v2/plugin.rb @@ -65,18 +65,12 @@ module Vagrant # is run. This allows plugin authors to hook into things like VM # bootup, VM provisioning, etc. # - # @param [Symbol] name Name of the action. + # @param [String] name Name of the action. # @return [Array] List of the hooks for the given action. def self.action_hook(name, &block) - # Get the list of hooks for the given hook name - data[:action_hooks] ||= {} - hooks = data[:action_hooks][name.to_sym] ||= [] + # The name is currently not used but we want it for the future. - # Return the list if we don't have a block - return hooks if !block_given? - - # Otherwise add the block to the list of hooks for this action. - hooks << block + components.action_hooks << block end # Defines additional command line commands available by key. The key diff --git a/test/unit/vagrant/plugin/v2/plugin_test.rb b/test/unit/vagrant/plugin/v2/plugin_test.rb index b2bd24487..935936920 100644 --- a/test/unit/vagrant/plugin/v2/plugin_test.rb +++ b/test/unit/vagrant/plugin/v2/plugin_test.rb @@ -29,7 +29,7 @@ describe Vagrant::Plugin::V2::Plugin do action_hook("foo") { "bar" } end - hooks = plugin.action_hook("foo") + hooks = plugin.components.action_hooks hooks.length.should == 1 hooks[0].call.should == "bar" end From b20dcc9eb857efa2dc02b3eae1d266c51a6af073 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Wed, 6 Feb 2013 15:37:12 -0800 Subject: [PATCH 7/8] Setup hooks in the runner --- lib/vagrant/action/runner.rb | 13 +++++++++++++ lib/vagrant/plugin/v2/manager.rb | 11 +++++++++++ 2 files changed, 24 insertions(+) diff --git a/lib/vagrant/action/runner.rb b/lib/vagrant/action/runner.rb index b92d2726c..b2306e12a 100644 --- a/lib/vagrant/action/runner.rb +++ b/lib/vagrant/action/runner.rb @@ -27,6 +27,19 @@ module Vagrant environment.merge!(@lazy_globals.call) if @lazy_globals environment.merge!(options || {}) + # Setup the action hooks + hooks = Vagrant.plugin("2").manager.action_hooks + if !hooks.empty? + @logger.info("Preparing hooks for middleware sequence...") + env[:action_hooks] = hooks.map do |hook_proc| + Hook.new.tap do |h| + hook_proc.call(h) + end + end + + @logger.info("#{env[:action_hooks].length} hooks defined.") + end + # Run the action chain in a busy block, marking the environment as # interrupted if a SIGINT occurs, and exiting cleanly once the # chain has been run. diff --git a/lib/vagrant/plugin/v2/manager.rb b/lib/vagrant/plugin/v2/manager.rb index 2a743c59d..f8ae7c940 100644 --- a/lib/vagrant/plugin/v2/manager.rb +++ b/lib/vagrant/plugin/v2/manager.rb @@ -14,6 +14,17 @@ module Vagrant @registered = [] end + # This returns all the action hooks. + # + # @return [Array] + def action_hooks + [].tap do |result| + @registered.each do |plugin| + result += plugin.components.action_hooks + end + end + end + # This returns all the registered commands. # # @return [Hash] From 352fec03598a93a28e4ed9a19a91cba409b490dc Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Wed, 6 Feb 2013 15:42:48 -0800 Subject: [PATCH 8/8] Fix some issues to get hooks working --- lib/vagrant/action/runner.rb | 5 +++-- lib/vagrant/plugin/v2/manager.rb | 10 ++++++---- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/lib/vagrant/action/runner.rb b/lib/vagrant/action/runner.rb index b2306e12a..7309a4010 100644 --- a/lib/vagrant/action/runner.rb +++ b/lib/vagrant/action/runner.rb @@ -1,5 +1,6 @@ require 'log4r' +require 'vagrant/action/hook' require 'vagrant/util/busy' # TODO: @@ -31,13 +32,13 @@ module Vagrant hooks = Vagrant.plugin("2").manager.action_hooks if !hooks.empty? @logger.info("Preparing hooks for middleware sequence...") - env[:action_hooks] = hooks.map do |hook_proc| + environment[:action_hooks] = hooks.map do |hook_proc| Hook.new.tap do |h| hook_proc.call(h) end end - @logger.info("#{env[:action_hooks].length} hooks defined.") + @logger.info("#{environment[:action_hooks].length} hooks defined.") end # Run the action chain in a busy block, marking the environment as diff --git a/lib/vagrant/plugin/v2/manager.rb b/lib/vagrant/plugin/v2/manager.rb index f8ae7c940..def073a45 100644 --- a/lib/vagrant/plugin/v2/manager.rb +++ b/lib/vagrant/plugin/v2/manager.rb @@ -18,11 +18,13 @@ module Vagrant # # @return [Array] def action_hooks - [].tap do |result| - @registered.each do |plugin| - result += plugin.components.action_hooks - end + result = [] + + @registered.each do |plugin| + result += plugin.components.action_hooks end + + result end # This returns all the registered commands.