CR feedback: move command evaluation to guard interpreter

6 files changed, 57 insertions, 58 deletions

diff --git a/lib/chef/guard_interpreter/default_guard_interpreter.rb b/lib/chef/guard_interpreter/default_guard_interpreter.rb
index f7d039c1cf..df91c2b1ad 100644
--- a/lib/chef/guard_interpreter/default_guard_interpreter.rb
+++ b/lib/chef/guard_interpreter/default_guard_interpreter.rb
@@ -19,16 +19,22 @@
 class Chef
   class GuardInterpreter
     class DefaultGuardInterpreter
+      include Chef::Mixin::ShellOut
 
       protected
 
-      def initialize
+      def initialize(command, opts)
+        @command = command
+        @command_opts = opts
       end
 
       public
 
-      def translate_command_block(command, opts, &block)
-        [command, block]
+      def evaluate
+        shell_out(@command, @command_opts).status.success?
+      rescue Chef::Exceptions::CommandTimeout
+        Chef::Log.warn "Command '#{@command}' timed out"
+        false
       end
     end
   end
diff --git a/lib/chef/guard_interpreter/resource_guard_interpreter.rb b/lib/chef/guard_interpreter/resource_guard_interpreter.rb
index c4b6d33a16..c2a2b1d6b4 100644
--- a/lib/chef/guard_interpreter/resource_guard_interpreter.rb
+++ b/lib/chef/guard_interpreter/resource_guard_interpreter.rb
@@ -22,34 +22,13 @@ class Chef
   class GuardInterpreter
     class ResourceGuardInterpreter < DefaultGuardInterpreter
 
-      def translate_command_block(command, opts, &block)
-        merge_inherited_attributes
-
-        if command && ! block_given?
-          block_attributes = opts.merge({:code => command})
-
-          # Handles cases like powershell_script where default
-          # attributes are different when used in a guard vs. not. For
-          # powershell_script in particular, this will go away when
-          # the one attribue that causes this changes its default to be
-          # the same after some period to prepare for deprecation
-          if @resource.class.respond_to?(:get_default_attributes)
-            block_attributes = @resource.class.send(:get_default_attributes, opts).merge(block_attributes)
-          end
-
-          translated_block = to_block(block_attributes)
-          [nil, translated_block]
-        else
-          super
-        end
-      end
-
-      def initialize(resource_symbol, parent_resource)
+      def initialize(parent_resource, command, opts, &block)
+        super(command, opts)
         @parent_resource = parent_resource
 
-        resource_class = get_resource_class(parent_resource, resource_symbol)
+        resource_class = get_resource_class(parent_resource)
 
-        raise ArgumentError, "Specified guard_interpreter resource #{resource_symbol.to_s} unknown for this platform" if resource_class.nil?
+        raise ArgumentError, "Specified guard_interpreter resource #{parent_resource.guard_interpreter.to_s} unknown for this platform" if resource_class.nil?
 
         empty_events = Chef::EventDispatch::Dispatcher.new
         anonymous_run_context = Chef::RunContext.new(parent_resource.node, {}, empty_events)
@@ -61,6 +40,29 @@ class Chef
         end
       end
 
+      def evaluate
+        # Add attributes inherited from the parent class
+        # to the resource
+        merge_inherited_attributes
+
+        # Script resources have a code attribute, which is
+        # what is used to execute the command, so include
+        # that with attributes specified by caller in opts
+        block_attributes = @command_opts.merge({:code => @command})
+
+        # Handles cases like powershell_script where default
+        # attributes are different when used in a guard vs. not. For
+        # powershell_script in particular, this will go away when
+        # the one attribue that causes this changes its default to be
+        # the same after some period to prepare for deprecation
+        if @resource.class.respond_to?(:get_default_attributes)
+          block_attributes = @resource.class.send(:get_default_attributes, @command_opts).merge(block_attributes)
+        end
+
+        resource_block = block_from_attributes(block_attributes)
+        evaluate_action(nil, &resource_block)
+      end
+
       protected
 
       def evaluate_action(action=nil, &block)
@@ -78,18 +80,11 @@ class Chef
         resource_updated
       end
 
-      def to_block(attributes, action=nil)
-        resource_block = block_from_attributes(attributes)
-        Proc.new do
-          evaluate_action(action, &resource_block)
-        end
-      end
-
-      def get_resource_class(parent_resource, resource_symbol)
+      def get_resource_class(parent_resource)
         if parent_resource.nil? || parent_resource.node.nil?
           raise ArgumentError, "Node for guard resource parent must not be nil"
         end
-        Chef::Resource.resource_for_node(resource_symbol, parent_resource.node)
+        Chef::Resource.resource_for_node(parent_resource.guard_interpreter, parent_resource.node)
       end
 
       def block_from_attributes(attributes)
diff --git a/lib/chef/resource/conditional.rb b/lib/chef/resource/conditional.rb
index 19881feb70..e6623be5dd 100644
--- a/lib/chef/resource/conditional.rb
+++ b/lib/chef/resource/conditional.rb
@@ -31,13 +31,11 @@ class Chef
       end
 
       def self.not_if(parent_resource, command=nil, command_opts={}, &block)
-        translated_command, translated_block = translate_command_block(parent_resource, command, command_opts, &block)
-        new(:not_if, translated_command, command_opts, &translated_block)
+        new(:not_if, parent_resource, command, command_opts, &block)
       end
 
       def self.only_if(parent_resource, command=nil, command_opts={}, &block)
-        translated_command, translated_block = translate_command_block(parent_resource, command, command_opts, &block)
-        new(:only_if, translated_command, command_opts, &translated_block)
+        new(:only_if, parent_resource, command, command_opts, &block)
       end
 
       attr_reader :positivity
@@ -45,14 +43,16 @@ class Chef
       attr_reader :command_opts
       attr_reader :block
 
-      def initialize(positivity, command=nil, command_opts={}, &block)
+      def initialize(positivity, parent_resource, command=nil, command_opts={}, &block)
         @positivity = positivity
         case command
         when String
+          @guard_interpreter = new_guard_interpreter(parent_resource, command, command_opts, &block)
           @command, @command_opts = command, command_opts
           @block = nil
         when nil
           raise ArgumentError, "only_if/not_if requires either a command or a block" unless block_given?
+          @guard_interpreter = nil
           @command, @command_opts = nil, nil
           @block = block
         else
@@ -72,11 +72,11 @@ class Chef
       end
 
       def evaluate
-        @command ? evaluate_command : evaluate_block
+        @guard_interpreter ? evaluate_command : evaluate_block
       end
 
       def evaluate_command
-        shell_out(@command, @command_opts).status.success?
+        @guard_interpreter.evaluate
       rescue Chef::Exceptions::CommandTimeout
         Chef::Log.warn "Command '#{@command}' timed out"
         false
@@ -103,16 +103,14 @@ class Chef
         end
       end
 
-      def self.translate_command_block(parent_resource, command, opts, &block)
-        guard_interpreter = nil
+      private
 
+      def new_guard_interpreter(parent_resource, command, opts)
         if parent_resource.guard_interpreter == :default
-          guard_interpreter = Chef::GuardInterpreter::DefaultGuardInterpreter.new
+          guard_interpreter = Chef::GuardInterpreter::DefaultGuardInterpreter.new(command, opts)
         else
-          guard_interpreter = Chef::GuardInterpreter::ResourceGuardInterpreter.new(parent_resource.guard_interpreter, parent_resource)
+          guard_interpreter = Chef::GuardInterpreter::ResourceGuardInterpreter.new(parent_resource, command, opts)
         end
-
-        guard_interpreter.translate_command_block(command, opts, &block)
       end
 
     end
diff --git a/spec/support/shared/unit/script_resource.rb b/spec/support/shared/unit/script_resource.rb
index 7cdd117e40..1137958420 100644
--- a/spec/support/shared/unit/script_resource.rb
+++ b/spec/support/shared/unit/script_resource.rb
@@ -79,7 +79,7 @@ shared_examples_for "a script resource" do
     end
 
     it "when a valid guard_interpreter resource is specified, a block should be used to evaluate the guard" do
-      Chef::Resource::Conditional.any_instance.should_not_receive(:evaluate_command)
+      Chef::GuardInterpreter::DefaultGuardInterpreter.any_instance.should_not_receive(:evaluate)
       Chef::GuardInterpreter::ResourceGuardInterpreter.any_instance.should_receive(:evaluate_action).and_return(true)
       resource.guard_interpreter :script
       resource.only_if 'echo hi'
diff --git a/spec/unit/resource/conditional_spec.rb b/spec/unit/resource/conditional_spec.rb
index 99d9aaa408..4df185bcd6 100644
--- a/spec/unit/resource/conditional_spec.rb
+++ b/spec/unit/resource/conditional_spec.rb
@@ -52,7 +52,7 @@ describe Chef::Resource::Conditional do
     describe 'after running a command which timed out' do
       before do
         @conditional = Chef::Resource::Conditional.only_if(@parent_resource, "false")
-        @conditional.stub(:shell_out).and_raise(Chef::Exceptions::CommandTimeout)
+        Chef::GuardInterpreter::DefaultGuardInterpreter.any_instance.stub(:shell_out).and_raise(Chef::Exceptions::CommandTimeout)
       end
 
       it 'indicates that resource convergence should not continue' do
@@ -111,7 +111,7 @@ describe Chef::Resource::Conditional do
     describe 'after running a command which timed out' do
       before do
         @conditional = Chef::Resource::Conditional.not_if(@parent_resource,  "false")
-        @conditional.stub(:shell_out).and_raise(Chef::Exceptions::CommandTimeout)
+        Chef::GuardInterpreter::DefaultGuardInterpreter.any_instance.stub(:shell_out).and_raise(Chef::Exceptions::CommandTimeout)
       end
 
       it 'indicates that resource convergence should continue' do
diff --git a/spec/unit/resource/powershell_spec.rb b/spec/unit/resource/powershell_spec.rb
index 1ef50f5eeb..da20c4f0bf 100644
--- a/spec/unit/resource/powershell_spec.rb
+++ b/spec/unit/resource/powershell_spec.rb
@@ -81,7 +81,7 @@ describe Chef::Resource::PowershellScript do
 
     it "should enable convert_boolean_return by default for guards in the context of powershell_script when no guard params are specified" do
       allow_any_instance_of(Chef::GuardInterpreter::ResourceGuardInterpreter).to receive(:evaluate_action).and_return(true)
-      allow_any_instance_of(Chef::GuardInterpreter::ResourceGuardInterpreter).to receive(:to_block).with(
+      allow_any_instance_of(Chef::GuardInterpreter::ResourceGuardInterpreter).to receive(:block_from_attributes).with(
         {:convert_boolean_return => true, :code => "$true"}).and_return(Proc.new {})
       resource.only_if("$true")
     end
@@ -92,21 +92,21 @@ describe Chef::Resource::PowershellScript do
       file_resource = Chef::Resource::File.new('idontexist', run_context)
       file_resource.guard_interpreter :powershell_script
 
-      allow_any_instance_of(Chef::GuardInterpreter::ResourceGuardInterpreter).to receive(:to_block).with(
+      allow_any_instance_of(Chef::GuardInterpreter::ResourceGuardInterpreter).to receive(:block_from_attributes).with(
         {:convert_boolean_return => true, :code => "$true"}).and_return(Proc.new {})
       resource.only_if("$true")
     end
 
     it "should enable convert_boolean_return by default for guards in the context of powershell_script when guard params are specified" do
       guard_parameters = {:cwd => '/etc/chef', :architecture => :x86_64}
-      allow_any_instance_of(Chef::GuardInterpreter::ResourceGuardInterpreter).to receive(:to_block).with(
+      allow_any_instance_of(Chef::GuardInterpreter::ResourceGuardInterpreter).to receive(:block_from_attributes).with(
         {:convert_boolean_return => true, :code => "$true"}.merge(guard_parameters)).and_return(Proc.new {})
       resource.only_if("$true", guard_parameters)
     end
 
     it "should pass convert_boolean_return as true if it was specified as true in a guard parameter" do
       guard_parameters = {:cwd => '/etc/chef', :convert_boolean_return => true, :architecture => :x86_64}
-      allow_any_instance_of(Chef::GuardInterpreter::ResourceGuardInterpreter).to receive(:to_block).with(
+      allow_any_instance_of(Chef::GuardInterpreter::ResourceGuardInterpreter).to receive(:block_from_attributes).with(
         {:convert_boolean_return => true, :code => "$true"}.merge(guard_parameters)).and_return(Proc.new {})
       resource.only_if("$true", guard_parameters)
     end
@@ -114,7 +114,7 @@ describe Chef::Resource::PowershellScript do
     it "should pass convert_boolean_return as false if it was specified as true in a guard parameter" do
       other_guard_parameters = {:cwd => '/etc/chef', :architecture => :x86_64}
       parameters_with_boolean_disabled = other_guard_parameters.merge({:convert_boolean_return => false, :code => "$true"})
-      allow_any_instance_of(Chef::GuardInterpreter::ResourceGuardInterpreter).to receive(:to_block).with(
+      allow_any_instance_of(Chef::GuardInterpreter::ResourceGuardInterpreter).to receive(:block_from_attributes).with(
         parameters_with_boolean_disabled).and_return(Proc.new {})
       resource.only_if("$true", parameters_with_boolean_disabled)
     end