(#6978) Add before and after decorators to actions from options.

Options can now add before_action and after_action blocks; these are invoked
before or after any action is invoked on the face.  This allows these options
to declare common behaviour and have it automatically applied to the actions
invoked.

Option hooks have no defined order of invocation: they will run in a
completely random order.  Where there are dependencies they should be on the
value of the options hash passed to the invocation, not on side-effects of the
other invocations.

You are not able to influence the arguments, options, or calling of the action
body in a before or after decorator.  This is by design.

The invocation passes to the hook:

 1. The action object representing this action.
 2. The arguments to the action, as an array.
 3. The options for the action, as a hash.

Paired-With: Max Martin <max@puppetlabs.com>

6 files changed, 221 insertions, 18 deletions

diff --git a/spec/lib/puppet/face/basetest.rb b/spec/lib/puppet/face/basetest.rb
index 00616f74f..e935161ae 100755
--- a/spec/lib/puppet/face/basetest.rb
+++ b/spec/lib/puppet/face/basetest.rb
@@ -1 +1,2 @@
+require 'puppet/face'
 Puppet::Face.define(:basetest, '0.0.1')
diff --git a/spec/unit/interface/action_builder_spec.rb b/spec/unit/interface/action_builder_spec.rb
index 5b04df900..cafdde4a1 100755
--- a/spec/unit/interface/action_builder_spec.rb
+++ b/spec/unit/interface/action_builder_spec.rb
@@ -12,10 +12,11 @@ describe Puppet::Interface::ActionBuilder do
     end
 
     it "should define a method on the face which invokes the action" do
-      face = Puppet::Interface.new(:action_builder_test_interface, '0.0.1')
-      action = Puppet::Interface::ActionBuilder.build(face, :foo) do
-        when_invoked do
-          "invoked the method"
+      face = Puppet::Interface.new(:action_builder_test_interface, '0.0.1') do
+        action :foo do
+          when_invoked do
+            "invoked the method"
+          end
         end
       end
 
diff --git a/spec/unit/interface/action_manager_spec.rb b/spec/unit/interface/action_manager_spec.rb
index c4b21eaac..0534a49f1 100755
--- a/spec/unit/interface/action_manager_spec.rb
+++ b/spec/unit/interface/action_manager_spec.rb
@@ -103,6 +103,7 @@ describe Puppet::Interface::ActionManager do
       @klass = Class.new do
         include Puppet::Interface::ActionManager
         extend Puppet::Interface::ActionManager
+        def __invoke_decorations(*args) true end
       end
       @instance = @klass.new
     end
diff --git a/spec/unit/interface/action_spec.rb b/spec/unit/interface/action_spec.rb
index 8c6782976..51aa837dc 100755
--- a/spec/unit/interface/action_spec.rb
+++ b/spec/unit/interface/action_spec.rb
@@ -79,7 +79,7 @@ describe Puppet::Interface::Action do
       end
 
       it "should work when options are supplied" do
-        options = face.bar :bar => "beer"
+        options = face.bar(:bar => "beer")
         options.should == { :bar => "beer" }
       end
     end
@@ -169,4 +169,175 @@ describe Puppet::Interface::Action do
       }.should raise_error ArgumentError, /Option foo conflicts with existing option foo/i
     end
   end
+
+  context "with action decorators" do
+    context "local only" do
+      let :face do
+        Puppet::Interface.new(:action_decorators, '0.0.1') do
+          action :bar do when_invoked do true end end
+          def report(arg) end
+        end
+      end
+
+      it "should call action before decorators" do
+        face.action(:baz) do
+          option "--baz" do
+            before_action do
+              report(:action_option)
+            end
+          end
+          when_invoked do true end
+        end
+
+        face.expects(:report).with(:action_option)
+        face.baz :baz => true
+      end
+
+      it "should call action after decorators" do
+        face.action(:baz) do
+          option "--baz" do
+            after_action do
+              report(:action_option)
+            end
+          end
+          when_invoked do true end
+        end
+
+        face.expects(:report).with(:action_option)
+        face.baz :baz => true
+      end
+
+      it "should call local before decorators" do
+        face.option "--foo FOO" do
+          before_action do
+            report(:before)
+          end
+        end
+        face.expects(:report).with(:before)
+        face.bar({:foo => 12})
+      end
+
+      it "should call local after decorators" do
+        face.option "--foo FOO" do after_action do report(:after) end end
+        face.expects(:report).with(:after)
+        face.bar({:foo => 12})
+      end
+
+      context "with inactive decorators" do
+        it "should not invoke a decorator if the options are empty" do
+          face.option "--foo FOO" do
+            before_action do report :before_action end
+          end
+          face.expects(:report).never # I am testing the negative.
+          face.bar
+        end
+
+        context "with some decorators only" do
+          before :each do
+            face.option "--foo" do before_action do report :foo end end
+            face.option "--bar" do before_action do report :bar end end
+          end
+
+          it "should work with the foo option" do
+            face.expects(:report).with(:foo)
+            face.bar(:foo => true)
+          end
+
+          it "should work with the bar option" do
+            face.expects(:report).with(:bar)
+            face.bar(:bar => true)
+          end
+
+          it "should work with both options" do
+            face.expects(:report).with(:foo)
+            face.expects(:report).with(:bar)
+            face.bar(:foo => true, :bar => true)
+          end
+        end
+      end
+    end
+
+    context "with inherited decorators" do
+      let :parent do
+        parent = Class.new(Puppet::Interface)
+        parent.script :on_parent do :on_parent end
+        parent.define_method :report do |arg| arg end
+        parent
+      end
+
+      let :child do
+        child = parent.new(:inherited_decorators, '0.0.1') do
+          script :on_child do :on_child end
+        end
+      end
+
+      context "with a child decorator" do
+        subject do
+          child.option "--foo FOO" do
+            before_action do
+              report(:child_before)
+            end
+          end
+          child.expects(:report).with(:child_before)
+          child
+        end
+
+        it "child actions should invoke the decorator" do
+          subject.on_child({:foo => true, :bar => true}).should == :on_child
+        end
+
+        it "parent actions should invoke the decorator" do
+          subject.on_parent({:foo => true, :bar => true}).should == :on_parent
+        end
+      end
+
+      context "with a parent decorator" do
+        subject do
+          parent.option "--foo FOO" do
+            before_action do
+              report(:parent_before)
+            end
+          end
+          child.expects(:report).with(:parent_before)
+          child
+        end
+
+        it "child actions should invoke the decorator" do
+          subject.on_child({:foo => true, :bar => true}).should == :on_child
+        end
+
+        it "parent actions should invoke the decorator" do
+          subject.on_parent({:foo => true, :bar => true}).should == :on_parent
+        end
+      end
+
+      context "with child and parent decorators" do
+        subject do
+          parent.option "--foo FOO" do
+            before_action { report(:parent_before) }
+            after_action  { report(:parent_after)  }
+          end
+          child.option "--bar BAR" do
+            before_action { report(:child_before) }
+            after_action  { report(:child_after)  }
+          end
+
+          child.expects(:report).with(:child_before)
+          child.expects(:report).with(:parent_before)
+          child.expects(:report).with(:parent_after)
+          child.expects(:report).with(:child_after)
+
+          child
+        end
+
+        it "child actions should invoke all the decorator" do
+          subject.on_child({:foo => true, :bar => true}).should == :on_child
+        end
+
+        it "parent actions should invoke all the decorator" do
+          subject.on_parent({:foo => true, :bar => true}).should == :on_parent
+        end
+      end
+    end
+  end
 end
diff --git a/spec/unit/interface/option_builder_spec.rb b/spec/unit/interface/option_builder_spec.rb
index fae48324e..0bcbed82c 100755
--- a/spec/unit/interface/option_builder_spec.rb
+++ b/spec/unit/interface/option_builder_spec.rb
@@ -8,22 +8,27 @@ describe Puppet::Interface::OptionBuilder do
       should be_an_instance_of Puppet::Interface::Option
   end
 
-  describe "when using the DSL block" do
-    it "should work with an empty block" do
-      option = Puppet::Interface::OptionBuilder.build(face, "--foo") do
-        # This block deliberately left blank.
-      end
+  it "should work with an empty block" do
+    option = Puppet::Interface::OptionBuilder.build(face, "--foo") do
+      # This block deliberately left blank.
+    end
+
+    option.should be_an_instance_of Puppet::Interface::Option
+  end
 
-      option.should be_an_instance_of Puppet::Interface::Option
+  it "should support documentation declarations" do
+    text = "this is the description"
+    option = Puppet::Interface::OptionBuilder.build(face, "--foo") do
+      desc text
     end
+    option.should be_an_instance_of Puppet::Interface::Option
+    option.desc.should == text
+  end
 
-    it "should support documentation declarations" do
-      text = "this is the description"
-      option = Puppet::Interface::OptionBuilder.build(face, "--foo") do
-        desc text
-      end
-      option.should be_an_instance_of Puppet::Interface::Option
-      option.desc.should == text
+  it "should support a before_action hook" do
+    option = Puppet::Interface::OptionBuilder.build(face, "--foo") do
+      before_action do :whatever end
     end
+    option.before_action.should be_an_instance_of UnboundMethod
   end
 end
diff --git a/spec/unit/interface/option_spec.rb b/spec/unit/interface/option_spec.rb
index 3bcd121e2..4c24dc940 100755
--- a/spec/unit/interface/option_spec.rb
+++ b/spec/unit/interface/option_spec.rb
@@ -72,4 +72,28 @@ describe Puppet::Interface::Option do
       option.to_s.should == "foo-bar"
     end
   end
+
+  %w{before after}.each do |side|
+    describe "#{side} hooks" do
+      subject { Puppet::Interface::Option.new(face, "--foo") }
+      let :proc do Proc.new do :from_proc end end
+
+      it { should respond_to "#{side}_action" }
+      it { should respond_to "#{side}_action=" }
+
+      it "should set the #{side}_action hook" do
+        subject.send("#{side}_action").should be_nil
+        subject.send("#{side}_action=", proc)
+        subject.send("#{side}_action").should be_an_instance_of UnboundMethod
+      end
+
+      data = [1, "foo", :foo, Object.new, method(:hash), method(:hash).unbind]
+      data.each do |input|
+        it "should fail if a #{input.class} is added to the #{side} hooks" do
+          expect { subject.send("#{side}_action=", input) }.
+            to raise_error ArgumentError, /not a proc/
+        end
+      end
+    end
+  end
 end