[Pkg-puppet-devel] [SCM] Puppet packaging for Debian branch, experimental, updated. debian/2.6.8-1-844-g7ec39d5

Daniel Pittman daniel at puppetlabs.com
Tue May 10 08:18:51 UTC 2011


The following commit has been merged in the experimental branch:
commit b23cc8abec1a1ec41b554b4e72f9a3c21feaf9da
Author: Daniel Pittman <daniel at puppetlabs.com>
Date:   Fri Apr 29 17:24:18 2011 -0700

    (#7282) action without `when_invoked` should fail...
    
    We used to let actions be declared without the `when_invoked` block, which was
    usually a sign of either someone writing their method code direct in action
    declaration, or someone forgetting to add their code at all.
    
    This was just let silently by: the error only showed up when you finally tried
    to invoke the action, and a NoMethod error was raised by the face.
    
    ...except for our own testing.  We took advantage of this a whole pile of
    times in there; fixing the original UI issue means fixing all those too.
    
    Reviewed-By: Nick Lewis <nick at puppetlabs.com>

diff --git a/lib/puppet/interface/action.rb b/lib/puppet/interface/action.rb
index 0dbdd57..203d808 100644
--- a/lib/puppet/interface/action.rb
+++ b/lib/puppet/interface/action.rb
@@ -171,7 +171,8 @@ class Puppet::Interface::Action
   # this stuff work, because it would have been cleaner.  Which gives you an
   # idea how motivated we were to make this cleaner.  Sorry.
   # --daniel 2011-03-31
-  attr_reader :positional_arg_count
+  attr_reader   :positional_arg_count
+  attr_accessor :when_invoked
   def when_invoked=(block)
 
     internal_name = "#{@name} implementation, required on Ruby 1.8".to_sym
@@ -219,9 +220,11 @@ WRAPPER
     if @face.is_a?(Class)
       @face.class_eval do eval wrapper, nil, file, line end
       @face.define_method(internal_name, &block)
+      @when_invoked = @face.instance_method(name)
     else
       @face.instance_eval do eval wrapper, nil, file, line end
       @face.meta_def(internal_name, &block)
+      @when_invoked = @face.method(name).unbind
     end
   end
 
diff --git a/lib/puppet/interface/action_builder.rb b/lib/puppet/interface/action_builder.rb
index afc49e9..0bf4f14 100644
--- a/lib/puppet/interface/action_builder.rb
+++ b/lib/puppet/interface/action_builder.rb
@@ -69,5 +69,6 @@ class Puppet::Interface::ActionBuilder
     @face   = face
     @action = Puppet::Interface::Action.new(face, name)
     instance_eval(&block)
+    @action.when_invoked or raise ArgumentError, "actions need to know what to do when_invoked; please add the block"
   end
 end
diff --git a/spec/lib/puppet/face/huzzah.rb b/spec/lib/puppet/face/huzzah.rb
index 2c2b7aa..6593d35 100755
--- a/spec/lib/puppet/face/huzzah.rb
+++ b/spec/lib/puppet/face/huzzah.rb
@@ -3,5 +3,5 @@ Puppet::Face.define(:huzzah, '2.0.1') do
   copyright "Puppet Labs", 2011
   license   "Apache 2 license; see COPYING"
   summary "life is a thing for celebration"
-  action :bar do "is where beer comes from" end
+  script :bar do "is where beer comes from" end
 end
diff --git a/spec/unit/interface/action_builder_spec.rb b/spec/unit/interface/action_builder_spec.rb
index bf7afa7..e0d0ebe 100755
--- a/spec/unit/interface/action_builder_spec.rb
+++ b/spec/unit/interface/action_builder_spec.rb
@@ -7,7 +7,8 @@ describe Puppet::Interface::ActionBuilder do
   let :face do Puppet::Interface.new(:puppet_interface_actionbuilder, '0.0.1') end
 
   it "should build an action" do
-    action = Puppet::Interface::ActionBuilder.build(nil, :foo) do
+    action = Puppet::Interface::ActionBuilder.build(face, :foo) do
+      when_invoked do true end
     end
     action.should be_a(Puppet::Interface::Action)
     action.name.should == :foo
@@ -26,17 +27,25 @@ describe Puppet::Interface::ActionBuilder do
       should raise_error("Action :foo must specify a block")
   end
 
+  it "should require an invocation block" do
+    expect {
+      Puppet::Interface::ActionBuilder.build(face, :foo) {}
+    }.to raise_error(/actions need to know what to do when_invoked; please add the block/)
+  end
+
   describe "when handling options" do
     it "should have a #option DSL function" do
       method = nil
       Puppet::Interface::ActionBuilder.build(face, :foo) do
+        when_invoked do true end
         method = self.method(:option)
       end
-      method.should be
+      method.should be_an_instance_of Method
     end
 
     it "should define an option without a block" do
       action = Puppet::Interface::ActionBuilder.build(face, :foo) do
+        when_invoked do true end
         option "--bar"
       end
       action.should be_option :bar
@@ -44,6 +53,7 @@ describe Puppet::Interface::ActionBuilder do
 
     it "should accept an empty block" do
       action = Puppet::Interface::ActionBuilder.build(face, :foo) do
+        when_invoked do true end
         option "--bar" do
           # This space left deliberately blank.
         end
@@ -55,6 +65,7 @@ describe Puppet::Interface::ActionBuilder do
   context "inline documentation" do
     it "should set the summary" do
       action = Puppet::Interface::ActionBuilder.build(face, :foo) do
+        when_invoked do true end
         summary "this is some text"
       end
       action.summary.should == "this is some text"
@@ -64,13 +75,16 @@ describe Puppet::Interface::ActionBuilder do
   context "action defaulting" do
     it "should set the default to true" do
       action = Puppet::Interface::ActionBuilder.build(face, :foo) do
+        when_invoked do true end
         default
       end
       action.default.should be_true
     end
 
     it "should not be default by, er, default. *cough*" do
-      action = Puppet::Interface::ActionBuilder.build(face, :foo) do end
+      action = Puppet::Interface::ActionBuilder.build(face, :foo) do
+        when_invoked do true end
+      end
       action.default.should be_false
     end
   end
@@ -79,6 +93,7 @@ describe Puppet::Interface::ActionBuilder do
     it "should fail if no rendering format is given" do
       expect {
         Puppet::Interface::ActionBuilder.build(face, :foo) do
+          when_invoked do true end
           when_rendering do true end
         end
       }.to raise_error ArgumentError, /must give a rendering format to when_rendering/
@@ -87,6 +102,7 @@ describe Puppet::Interface::ActionBuilder do
     it "should fail if no block is given" do
       expect {
         Puppet::Interface::ActionBuilder.build(face, :foo) do
+          when_invoked do true end
           when_rendering :json
         end
       }.to raise_error ArgumentError, /must give a block to when_rendering/
@@ -95,6 +111,7 @@ describe Puppet::Interface::ActionBuilder do
     it "should fail if the block takes no arguments" do
       expect {
         Puppet::Interface::ActionBuilder.build(face, :foo) do
+          when_invoked do true end
           when_rendering :json do true end
         end
       }.to raise_error ArgumentError, /when_rendering methods take one argument, the result, not/
@@ -103,6 +120,7 @@ describe Puppet::Interface::ActionBuilder do
     it "should fail if the block takes more than one argument" do
       expect {
         Puppet::Interface::ActionBuilder.build(face, :foo) do
+          when_invoked do true end
           when_rendering :json do |a, b, c| true end
         end
       }.to raise_error ArgumentError, /when_rendering methods take one argument, the result, not/
@@ -111,6 +129,7 @@ describe Puppet::Interface::ActionBuilder do
     it "should fail if the block takes a variable number of arguments" do
       expect {
         Puppet::Interface::ActionBuilder.build(face, :foo) do
+          when_invoked do true end
           when_rendering :json do |*args| true end
         end
       }.to raise_error(ArgumentError,
@@ -119,6 +138,7 @@ describe Puppet::Interface::ActionBuilder do
 
     it "should stash a rendering block" do
       action = Puppet::Interface::ActionBuilder.build(face, :foo) do
+        when_invoked do true end
         when_rendering :json do |a| true end
       end
       action.when_rendering(:json).should be_an_instance_of Method
@@ -127,6 +147,7 @@ describe Puppet::Interface::ActionBuilder do
     it "should fail if you try to set the same rendering twice" do
       expect {
         Puppet::Interface::ActionBuilder.build(face, :foo) do
+          when_invoked do true end
           when_rendering :json do |a| true end
           when_rendering :json do |a| true end
         end
@@ -135,6 +156,7 @@ describe Puppet::Interface::ActionBuilder do
 
     it "should work if you set two different renderings" do
       action = Puppet::Interface::ActionBuilder.build(face, :foo) do
+        when_invoked do true end
         when_rendering :json do |a| true end
         when_rendering :yaml do |a| true end
       end
@@ -144,6 +166,7 @@ describe Puppet::Interface::ActionBuilder do
 
     it "should be bound to the face when called" do
       action = Puppet::Interface::ActionBuilder.build(face, :foo) do
+        when_invoked do true end
         when_rendering :json do |a| self end
       end
       action.when_rendering(:json).call(true).should == face
@@ -152,13 +175,16 @@ describe Puppet::Interface::ActionBuilder do
 
   context "#render_as" do
     it "should default to nil (eg: based on context)" do
-      action = Puppet::Interface::ActionBuilder.build(face, :foo) do end
+      action = Puppet::Interface::ActionBuilder.build(face, :foo) do
+        when_invoked do true end
+      end
       action.render_as.should be_nil
     end
 
     it "should fail if not rendering format is given" do
       expect {
         Puppet::Interface::ActionBuilder.build(face, :foo) do
+          when_invoked do true end
           render_as
         end
       }.to raise_error ArgumentError, /must give a rendering format to render_as/
@@ -167,6 +193,7 @@ describe Puppet::Interface::ActionBuilder do
     Puppet::Network::FormatHandler.formats.each do |name|
       it "should accept #{name.inspect} format" do
         action = Puppet::Interface::ActionBuilder.build(face, :foo) do
+          when_invoked do true end
           render_as name
         end
         action.render_as.should == name
@@ -175,6 +202,7 @@ describe Puppet::Interface::ActionBuilder do
 
     it "should accept :for_humans format" do
       action = Puppet::Interface::ActionBuilder.build(face, :foo) do
+        when_invoked do true end
         render_as :for_humans
       end
       action.render_as.should == :for_humans
@@ -184,6 +212,7 @@ describe Puppet::Interface::ActionBuilder do
       it "should fail if given #{input.inspect}" do
         expect {
           Puppet::Interface::ActionBuilder.build(face, :foo) do
+            when_invoked do true end
             render_as input
           end
         }.to raise_error ArgumentError, /#{input.inspect} is not a valid rendering format/
diff --git a/spec/unit/interface/action_manager_spec.rb b/spec/unit/interface/action_manager_spec.rb
index 07d517c..5a479ad 100755
--- a/spec/unit/interface/action_manager_spec.rb
+++ b/spec/unit/interface/action_manager_spec.rb
@@ -94,7 +94,7 @@ describe Puppet::Interface::ActionManager do
     end
 
     it "should be able to indicate when an action is defined" do
-      subject.action(:foo) { "something" }
+      subject.action(:foo) { when_invoked do true end }
       subject.should be_action(:foo)
     end
   end
@@ -218,25 +218,29 @@ describe Puppet::Interface::ActionManager do
 
   describe "#action" do
     it 'should add an action' do
-      subject.action(:foo) {  }
+      subject.action(:foo) { when_invoked do true end }
       subject.get_action(:foo).should be_a Puppet::Interface::Action
     end
 
     it 'should support default actions' do
-      subject.action(:foo) { default }
+      subject.action(:foo) { when_invoked do true end; default }
       subject.get_default_action.should == subject.get_action(:foo)
     end
 
     it 'should not support more than one default action' do
-      subject.action(:foo) { default }
-      expect { subject.action(:bar) { default } }.should raise_error
+      subject.action(:foo) { when_invoked do true end; default }
+      expect { subject.action(:bar) {
+          when_invoked do true end
+          default
+        }
+      }.should raise_error /cannot both be default/
     end
   end
 
   describe "#get_action" do
     let :parent_class do
       parent_class = Class.new(Puppet::Interface)
-      parent_class.action(:foo) {}
+      parent_class.action(:foo) { when_invoked do true end }
       parent_class
     end
 
diff --git a/spec/unit/interface/action_spec.rb b/spec/unit/interface/action_spec.rb
index f8102f4..e799609 100755
--- a/spec/unit/interface/action_spec.rb
+++ b/spec/unit/interface/action_spec.rb
@@ -150,6 +150,7 @@ describe Puppet::Interface::Action do
     it "should support options with an empty block" do
       face = Puppet::Interface.new(:action_level_options, '0.0.1') do
         action :foo do
+          when_invoked do true end
           option "--bar" do
             # this line left deliberately blank
           end
@@ -162,7 +163,10 @@ describe Puppet::Interface::Action do
 
     it "should return only action level options when there are no face options" do
       face = Puppet::Interface.new(:action_level_options, '0.0.1') do
-        action :foo do option "--bar" end
+        action :foo do
+          when_invoked do true end
+          option "--bar"
+        end
       end
 
       face.get_action(:foo).options.should =~ [:bar]
@@ -171,8 +175,8 @@ describe Puppet::Interface::Action do
     describe "with both face and action options" do
       let :face do
         Puppet::Interface.new(:action_level_options, '0.0.1') do
-          action :foo do option "--bar" end
-          action :baz do option "--bim" end
+          action :foo do when_invoked do true end ; option "--bar" end
+          action :baz do when_invoked do true end ; option "--bim" end
           option "--quux"
         end
       end
@@ -186,7 +190,10 @@ describe Puppet::Interface::Action do
         parent.option "--foo"
         child = parent.new(:inherited_options, '0.0.1') do
           option "--bar"
-          action :action do option "--baz" end
+          action :action do
+            when_invoked do true end
+            option "--baz"
+          end
         end
 
         action = child.get_action(:action)
@@ -215,7 +222,10 @@ describe Puppet::Interface::Action do
     it_should_behave_like "things that declare options" do
       def add_options_to(&block)
         face = Puppet::Interface.new(:with_options, '0.0.1') do
-          action(:foo, &block)
+          action(:foo) do
+            when_invoked do true end
+            self.instance_eval &block
+          end
         end
         face.get_action(:foo)
       end
@@ -498,7 +508,9 @@ describe Puppet::Interface::Action do
   it_should_behave_like "documentation on faces" do
     subject do
       face = Puppet::Interface.new(:action_documentation, '0.0.1') do
-        action :documentation do end
+        action :documentation do
+          when_invoked do true end
+        end
       end
       face.get_action(:documentation)
     end
diff --git a/spec/unit/interface_spec.rb b/spec/unit/interface_spec.rb
index 27da397..e28e55a 100755
--- a/spec/unit/interface_spec.rb
+++ b/spec/unit/interface_spec.rb
@@ -146,6 +146,7 @@ describe Puppet::Interface do
         option "--foo"
         option "--bar"
         action :baz do
+          when_invoked { true }
           option "--quux"
         end
       end
@@ -155,7 +156,10 @@ describe Puppet::Interface do
     it "should fail when a face option duplicates an action option" do
       expect {
         subject.new(:action_level_options, '0.0.1') do
-          action :bar do option "--foo" end
+          action :bar do
+            when_invoked { true }
+            option "--foo"
+          end
           option "--foo"
         end
       }.should raise_error ArgumentError, /Option foo conflicts with existing option foo on/i
@@ -163,8 +167,8 @@ describe Puppet::Interface do
 
     it "should work when two actions have the same option" do
       face = subject.new(:with_options, '0.0.1') do
-        action :foo do option "--quux" end
-        action :bar do option "--quux" end
+        action :foo do when_invoked { true } ; option "--quux" end
+        action :bar do when_invoked { true } ; option "--quux" end
       end
 
       face.get_action(:foo).options.should =~ [:quux]
@@ -176,14 +180,14 @@ describe Puppet::Interface do
     let :parent do
       parent = Class.new(subject)
       parent.option("--inherited")
-      parent.action(:parent_action) do end
+      parent.action(:parent_action) do when_invoked { true } end
       parent
     end
 
     let :face do
       face = parent.new(:example, '0.2.1')
       face.option("--local")
-      face.action(:face_action) do end
+      face.action(:face_action) do when_invoked { true } end
       face
     end
 

-- 
Puppet packaging for Debian



More information about the Pkg-puppet-devel mailing list