From daf711fd80a9e94f4675a3a8dc5a9660f42cce35 Mon Sep 17 00:00:00 2001
From: Mitchell Hashimoto <mitchell.hashimoto@gmail.com>
Date: Fri, 9 Dec 2011 14:22:03 -0800
Subject: [PATCH] Separate Vagrant::Action into Runner and Registry

---
 lib/vagrant/action.rb                         | 128 +-----------------
 lib/vagrant/action/box/download.rb            |   6 +-
 lib/vagrant/action/builtin.rb                 |   3 +-
 lib/vagrant/action/environment.rb             |  25 +---
 lib/vagrant/action/registry.rb                |  32 +++++
 lib/vagrant/action/runner.rb                  |  49 +++++++
 lib/vagrant/box.rb                            |   8 --
 lib/vagrant/box_collection.rb                 |  13 +-
 lib/vagrant/command/box.rb                    |   2 +-
 lib/vagrant/environment.rb                    |  18 ++-
 test/unit/vagrant/action/environment_test.rb  |  17 +++
 test/unit/vagrant/action/registry_test.rb     |  26 ++++
 test/unit/vagrant/action/runner_test.rb       |  43 ++++++
 test/unit/vagrant/box_collection_test.rb      |  17 ++-
 .../vagrant/action/environment_test.rb        |  27 ----
 15 files changed, 215 insertions(+), 199 deletions(-)
 create mode 100644 lib/vagrant/action/registry.rb
 create mode 100644 lib/vagrant/action/runner.rb
 create mode 100644 test/unit/vagrant/action/environment_test.rb
 create mode 100644 test/unit/vagrant/action/registry_test.rb
 create mode 100644 test/unit/vagrant/action/runner_test.rb
 delete mode 100644 test/unit_legacy/vagrant/action/environment_test.rb

diff --git a/lib/vagrant/action.rb b/lib/vagrant/action.rb
index a2878620f..5e7088b95 100644
--- a/lib/vagrant/action.rb
+++ b/lib/vagrant/action.rb
@@ -10,134 +10,10 @@ require 'vagrant/action/general'
 require 'vagrant/action/vm'
 
 module Vagrant
-  # Manages action running and registration. Every Vagrant environment
-  # has an instance of {Action} to allow for running in the context of
-  # the environment, which is accessible at {Environment#actions}. Actions
-  # are the foundation of most functionality in Vagrant, and are implemented
-  # architecturally as "middleware."
-  #
-  # # Registering an Action
-  #
-  # The main benefits of registering an action is the ability to retrieve and
-  # modify that registered action, as well as easily run the action. An example
-  # of registering an action is shown below, with a simple middleware which just
-  # outputs to `STDOUT`:
-  #
-  #     class StdoutMiddleware
-  #       def initialize(app, env)
-  #         @app = app
-  #       end
-  #
-  #       def call(env)
-  #         puts "HI!"
-  #         @app.call(env)
-  #       end
-  #     end
-  #
-  #     Vagrant::Action.register(:stdout, StdoutMiddleware)
-  #
-  # Then to run a registered action, assuming `env` is a loaded {Environment}:
-  #
-  #     env.actions.run(:stdout)
-  #
-  # Or to retrieve the action class for any reason:
-  #
-  #     Vagrant::Action[:stdout]
-  #
-  # # Running an Action
-  #
-  # There are various built-in registered actions such as `start`, `stop`, `up`,
-  # etc. Actions are built to be run in the context of an environment, so use
-  # {Environment#actions} to run all actions. Then simply call {#run}:
-  #
-  #     env.actions.run(:name)
-  #
-  # Where `:name` is the name of the registered action.
-  #
   class Action
     autoload :Environment, 'vagrant/action/environment'
-    autoload :MultiStep,   'vagrant/action/multistep'
-    autoload :Step,        'vagrant/action/step'
+    autoload :Registry,    'vagrant/action/registry'
+    autoload :Runner,      'vagrant/action/runner'
     autoload :Warden,      'vagrant/action/warden'
-
-    include Util
-    @@reported_interrupt = false
-
-    class << self
-      # Returns the list of registered actions.
-      #
-      # @return [Array]
-      def actions
-        @actions ||= {}
-      end
-
-      # Registers an action and associates it with a symbol. This
-      # symbol can then be referenced in other action builds and
-      # callbacks can be registered on that symbol.
-      #
-      # @param [Symbol] key
-      def register(key, callable)
-        actions[key.to_sym] = callable
-      end
-
-      # Retrieves a registered action by key.
-      #
-      # @param [Symbol] key
-      def [](key)
-        actions[key.to_sym]
-      end
-    end
-
-    # The environment to run the actions in.
-    attr_reader :env
-
-    # Initializes the action with the given environment which the actions
-    # will be run in.
-    #
-    # @param [Environment] env
-    def initialize(env)
-      @env = env
-      @logger = Log4r::Logger.new("vagrant::action")
-    end
-
-    # Runs the given callable object in the context of the environment.
-    # If a symbol is given as the `callable` parameter, then it is looked
-    # up in the registered actions list which are registered with {register}.
-    #
-    # Any options given are injected into the environment hash.
-    #
-    # @param [Object] callable An object which responds to `call`.
-    def run(callable_id, options=nil)
-      callable = callable_id
-      callable = Builder.new.use(callable_id) if callable_id.kind_of?(Class)
-      callable = self.class.actions[callable_id] if callable_id.kind_of?(Symbol)
-      raise ArgumentError, "Argument to run must be a callable object or registered action." if !callable || !callable.respond_to?(:call)
-
-      action_environment = Action::Environment.new(env)
-      action_environment.merge!(options || {})
-
-      # Run the before action run callback, if we're not doing that already
-      run(:before_action_run, action_environment) if callable_id != :before_action_run
-
-      # 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.
-      int_callback = lambda do
-        if action_environment.interrupted?
-          env.ui.error I18n.t("vagrant.actions.runner.exit_immediately")
-          abort
-        end
-
-        env.ui.warn I18n.t("vagrant.actions.runner.waiting_cleanup") if !@@reported_interrupt
-        action_environment.interrupt!
-        @@reported_interrupt = true
-      end
-
-      # We place a process lock around every action that is called
-      @logger.info("Running action: #{callable_id}")
-      env.lock do
-        Busy.busy(int_callback) { callable.call(action_environment) }
-      end
-    end
   end
 end
diff --git a/lib/vagrant/action/box/download.rb b/lib/vagrant/action/box/download.rb
index 31f5b686f..381490f55 100644
--- a/lib/vagrant/action/box/download.rb
+++ b/lib/vagrant/action/box/download.rb
@@ -36,7 +36,7 @@ module Vagrant
 
             # Use the class if it matches the given URI or if this
             # is the last class...
-            if classes.length == (i + 1) || klass.match?(@env["box"].uri)
+            if classes.length == (i + 1) || klass.match?(@env["box_url"])
               @env.ui.info I18n.t("vagrant.actions.box.download.with", :class => klass.to_s)
               @downloader = klass.new(@env)
               break
@@ -47,7 +47,7 @@ module Vagrant
           # just in case for now.
           raise Errors::BoxDownloadUnknownType if !@downloader
 
-          @downloader.prepare(@env["box"].uri)
+          @downloader.prepare(@env["box_url"])
           true
         end
 
@@ -76,7 +76,7 @@ module Vagrant
         end
 
         def download_to(f)
-          @downloader.download!(@env["box"].uri, f)
+          @downloader.download!(@env["box_url"], f)
         end
       end
     end
diff --git a/lib/vagrant/action/builtin.rb b/lib/vagrant/action/builtin.rb
index 4e907144d..ba5d1e419 100644
--- a/lib/vagrant/action/builtin.rb
+++ b/lib/vagrant/action/builtin.rb
@@ -5,6 +5,7 @@ module Vagrant
     # all the necessary Vagrant libraries are loaded. Hopefully
     # in the future this will no longer be necessary with autoloading.
     def self.builtin!
+      return
       # provision - Provisions a running VM
       register(:provision, Builder.new do
         use VM::CheckAccessible
@@ -109,7 +110,7 @@ module Vagrant
       # Other callbacks. There will be more of these in the future. For
       # now, these are limited to what are needed internally.
       register(:before_action_run, Builder.new do
-        use General::Validate
+#        use General::Validate
       end)
     end
   end
diff --git a/lib/vagrant/action/environment.rb b/lib/vagrant/action/environment.rb
index 7da09f942..08d5d73da 100644
--- a/lib/vagrant/action/environment.rb
+++ b/lib/vagrant/action/environment.rb
@@ -4,32 +4,11 @@ module Vagrant
     # to the `call` method of each action. This environment contains
     # some helper methods for accessing the environment as well
     # as being a hash, to store any additional options.
-    class Environment < Util::HashWithIndifferentAccess
-      # The {Vagrant::Environment} object represented by this
-      # action environment.
-      attr_reader :env
-
-      def initialize(env)
-        super() do |h,k|
-          # By default, try to find the key as a method on the
-          # environment. Gross eval use here.
-          begin
-            value = eval("h.env.#{k}")
-            h[k] = value
-          rescue Exception
-            nil
-          end
-        end
-
-        @env = env
+    class Environment < Hash
+      def initialize
         @interrupted = false
       end
 
-      # Returns a UI object from the environment
-      def ui
-        env.ui
-      end
-
       # Marks an environment as interrupted (by an outside signal or
       # anything). This will trigger any middleware sequences using this
       # environment to halt. This is automatically set by {Action} when
diff --git a/lib/vagrant/action/registry.rb b/lib/vagrant/action/registry.rb
new file mode 100644
index 000000000..0763ca896
--- /dev/null
+++ b/lib/vagrant/action/registry.rb
@@ -0,0 +1,32 @@
+module Vagrant
+  class Action
+    # This is the action registry, which stores action steps indexed
+    # by a unique name. These registry names can be used to call actions
+    # via the `Runner` class.
+    class Registry
+      def initialize
+        @actions = {}
+      end
+
+      # Register a callable by key.
+      #
+      # The callable should be given in a block which will be lazily evaluated
+      # when the action is needed.
+      #
+      # If an action by the given name already exists then it will be
+      # overwritten.
+      def register(key, &block)
+        @actions[key] = block
+      end
+
+      # Get an action by the given key.
+      #
+      # This will evaluate the block given to `register` and return the resulting
+      # action stack.
+      def get(key)
+        return nil if !@actions.has_key?(key)
+        @actions[key].call
+      end
+    end
+  end
+end
diff --git a/lib/vagrant/action/runner.rb b/lib/vagrant/action/runner.rb
new file mode 100644
index 000000000..cf7db63eb
--- /dev/null
+++ b/lib/vagrant/action/runner.rb
@@ -0,0 +1,49 @@
+require 'log4r'
+
+require 'vagrant/util/busy'
+
+# TODO:
+# * env.ui
+# * env.lock
+
+module Vagrant
+  class Action
+    class Runner
+      @@reported_interrupt = false
+
+      def initialize(registry)
+        @registry = registry
+        @logger   = Log4r::Logger.new("vagrant::action::runner")
+      end
+
+      def run(callable_id, options=nil)
+        callable = callable_id
+        callable = Builder.new.use(callable_id) if callable_id.kind_of?(Class)
+        callable = @registry.get(callable_id) if callable_id.kind_of?(Symbol)
+        raise ArgumentError, "Argument to run must be a callable object or registered action." if !callable || !callable.respond_to?(:call)
+
+        # Create the initial environment with the options given
+        environment = Environment.new
+        environment.merge!(options || {})
+
+        # 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.
+        int_callback = lambda do
+          if environment.interrupted?
+            env.ui.error I18n.t("vagrant.actions.runner.exit_immediately")
+            abort
+          end
+
+          env.ui.warn I18n.t("vagrant.actions.runner.waiting_cleanup") if !@@reported_interrupt
+          environment.interrupt!
+          @@reported_interrupt = true
+        end
+
+        # We place a process lock around every action that is called
+        @logger.info("Running action: #{callable_id}")
+        Util::Busy.busy(int_callback) { callable.call(environment) }
+      end
+    end
+  end
+end
diff --git a/lib/vagrant/box.rb b/lib/vagrant/box.rb
index 3c1924b54..f6b105b14 100644
--- a/lib/vagrant/box.rb
+++ b/lib/vagrant/box.rb
@@ -37,14 +37,6 @@ module Vagrant
       directory.join(env.config.vm.box_ovf)
     end
 
-    # Begins the process of adding a box to the vagrant installation. This
-    # method requires that `name` and `uri` be set. The logic of this method
-    # is kicked out to the `box_add` registered middleware.
-    def add
-      raise Errors::BoxAlreadyExists, :name => name if File.directory?(directory)
-      env.actions.run(:box_add, { "box" => self, "validate" => false })
-    end
-
     # Begins the process of destroying this box. This cannot be undone!
     def destroy
       env.actions.run(:box_remove, { "box" => self, "validate" => false })
diff --git a/lib/vagrant/box_collection.rb b/lib/vagrant/box_collection.rb
index 9bd871ec3..b3a43f239 100644
--- a/lib/vagrant/box_collection.rb
+++ b/lib/vagrant/box_collection.rb
@@ -12,9 +12,10 @@ module Vagrant
     attr_reader :directory
 
     # Initializes the class to search for boxes in the given directory.
-    def initialize(directory)
-      @directory = directory
-      @boxes     = []
+    def initialize(directory, action_runner)
+      @directory     = directory
+      @boxes         = []
+      @action_runner = action_runner
 
       reload!
     end
@@ -29,6 +30,12 @@ module Vagrant
       nil
     end
 
+    # Adds a box to this collection with the given name and located
+    # at the given URL.
+    def add(name, url)
+      @action_runner.run(:box_add, :box_name => name, :box_url => url)
+    end
+
     # Loads the list of all boxes from the source. This modifies the
     # current array.
     def reload!
diff --git a/lib/vagrant/command/box.rb b/lib/vagrant/command/box.rb
index a2908ff9f..2f731ad85 100644
--- a/lib/vagrant/command/box.rb
+++ b/lib/vagrant/command/box.rb
@@ -5,7 +5,7 @@ module Vagrant
 
       desc "add NAME URI", "Add a box to the system"
       def add(name, uri)
-        Box.add(env, name, uri)
+        env.boxes.add(name, uri)
       end
 
       desc "remove NAME", "Remove a box from the system"
diff --git a/lib/vagrant/environment.rb b/lib/vagrant/environment.rb
index fcde3a6ea..a29000314 100644
--- a/lib/vagrant/environment.rb
+++ b/lib/vagrant/environment.rb
@@ -146,7 +146,7 @@ module Vagrant
     #
     # @return [BoxCollection]
     def boxes
-      @_boxes ||= BoxCollection.new(boxes_path)
+      @_boxes ||= BoxCollection.new(boxes_path, action_runner)
     end
 
     # Returns the VMs associated with this environment.
@@ -205,12 +205,18 @@ module Vagrant
       @host ||= Hosts::Base.load(self, config.vagrant.host)
     end
 
-    # Returns the {Action} class for this environment which allows actions
-    # to be executed (middleware chains) in the context of this environment.
+    # Action runner for executing actions in the context of this environment.
     #
-    # @return [Action]
-    def actions
-      @actions ||= Action.new(self)
+    # @return [Action::Runner]
+    def action_runner
+      @action_runner ||= Action::Runner.new(action_registry)
+    end
+
+    # Action registry for registering new actions with this environment.
+    #
+    # @return [Action::Registry]
+    def action_registry
+      @action_registry ||= Action::Registry.new
     end
 
     # Loads on initial access and reads data from the global data store.
diff --git a/test/unit/vagrant/action/environment_test.rb b/test/unit/vagrant/action/environment_test.rb
new file mode 100644
index 000000000..c16850d06
--- /dev/null
+++ b/test/unit/vagrant/action/environment_test.rb
@@ -0,0 +1,17 @@
+require File.expand_path("../../../base", __FILE__)
+
+describe Vagrant::Action::Environment do
+  let(:instance) { described_class.new }
+
+  it "should be a hash" do
+    instance.should be_empty
+    instance["foo"] = "bar"
+    instance["foo"].should == "bar"
+  end
+
+  it "should keep track of interrupted state" do
+    instance.should_not be_interrupted
+    instance.interrupt!
+    instance.should be_interrupted
+  end
+end
diff --git a/test/unit/vagrant/action/registry_test.rb b/test/unit/vagrant/action/registry_test.rb
new file mode 100644
index 000000000..eda383214
--- /dev/null
+++ b/test/unit/vagrant/action/registry_test.rb
@@ -0,0 +1,26 @@
+require File.expand_path("../../../base", __FILE__)
+
+describe Vagrant::Action::Registry do
+  let(:instance) { described_class.new }
+
+  it "should return nil for nonexistent actions" do
+    instance.get("foo").should be_nil
+  end
+
+  it "should register an action without calling the block yet" do
+    expect do
+      instance.register("foo") do
+        raise Exception, "BOOM!"
+      end
+    end.to_not raise_error
+  end
+
+  it "should call and return the result of a block when asking for the action" do
+    object = Object.new
+    instance.register("foo") do
+      object
+    end
+
+    instance.get("foo").should eql(object)
+  end
+end
diff --git a/test/unit/vagrant/action/runner_test.rb b/test/unit/vagrant/action/runner_test.rb
new file mode 100644
index 000000000..15f10013e
--- /dev/null
+++ b/test/unit/vagrant/action/runner_test.rb
@@ -0,0 +1,43 @@
+require File.expand_path("../../../base", __FILE__)
+
+describe Vagrant::Action::Runner do
+  let(:registry) do
+    d = double("registry")
+    d.stub(:get)
+    d
+  end
+
+  let(:instance) { described_class.new(registry) }
+
+  it "should raise an error if an invalid callable is given" do
+    expect { instance.run(7) }.to raise_error(ArgumentError, /must be a callable/)
+  end
+
+  it "should be able to use a Proc as a callable" do
+    callable = Proc.new { raise Exception, "BOOM" }
+    expect { instance.run(callable) }.to raise_error(Exception, "BOOM")
+  end
+
+  it "should be able to use a Class as a callable" do
+    callable = Class.new do
+      def initialize(app, env)
+      end
+
+      def call(env)
+        raise Exception, "BOOM"
+      end
+    end
+
+    expect { instance.run(callable) }.to raise_error(Exception, "BOOM")
+  end
+
+  it "should pass options into hash given to callable" do
+    result = nil
+    callable = lambda do |env|
+      result = env["data"]
+    end
+
+    instance.run(callable, "data" => "foo")
+    result.should == "foo"
+  end
+end
diff --git a/test/unit/vagrant/box_collection_test.rb b/test/unit/vagrant/box_collection_test.rb
index 1d21ef59d..050ab0315 100644
--- a/test/unit/vagrant/box_collection_test.rb
+++ b/test/unit/vagrant/box_collection_test.rb
@@ -4,7 +4,8 @@ describe Vagrant::BoxCollection do
   include_context "unit"
 
   let(:environment) { isolated_environment }
-  let(:instance)    { described_class.new(environment.boxes_dir) }
+  let(:action_runner) { double("action runner") }
+  let(:instance)    { described_class.new(environment.boxes_dir, action_runner) }
 
   it "should list all available boxes" do
     # No boxes yet.
@@ -29,4 +30,18 @@ describe Vagrant::BoxCollection do
       result.name.should == "foo"
     end
   end
+
+  it "should add the box" do
+    name = "foo"
+    url  = "bar"
+
+    # Test the invocation of the action runner with the proper name
+    # and parameters. We leave the testing of the actual stack to
+    # acceptance tests, and individual pieces to unit tests of each
+    # step.
+    options = { :box_name => name, :box_url => url }
+    action_runner.should_receive(:run).with(:box_add, options)
+
+    instance.add(name, url)
+  end
 end
diff --git a/test/unit_legacy/vagrant/action/environment_test.rb b/test/unit_legacy/vagrant/action/environment_test.rb
deleted file mode 100644
index 633ab4ecb..000000000
--- a/test/unit_legacy/vagrant/action/environment_test.rb
+++ /dev/null
@@ -1,27 +0,0 @@
-require "test_helper"
-
-class ActionEnvironmentTest < Test::Unit::TestCase
-  setup do
-    @klass = Vagrant::Action::Environment
-    @instance = @klass.new(vagrant_env)
-  end
-
-  should "be a hash with indifferent access" do
-    assert @instance.is_a?(Vagrant::Util::HashWithIndifferentAccess)
-  end
-
-  should "default values to those on the env" do
-    @instance.env.stubs(:key).returns("value")
-    assert_equal "value", @instance["key"]
-  end
-
-  should "setup the UI" do
-    assert_equal @instance.env.ui, @instance.ui
-  end
-
-  should "report interrupted if interrupt error" do
-    assert !@instance.interrupted?
-    @instance.interrupt!
-    assert @instance.interrupted?
-  end
-end