CR feedback: refactor guard interpreter logic out of Resource into Conditional

8 files changed, 97 insertions, 50 deletions

diff --git a/lib/chef/resource.rb b/lib/chef/resource.rb
index e4fe404fdb..7c191b700e 100644
--- a/lib/chef/resource.rb
+++ b/lib/chef/resource.rb
@@ -23,9 +23,9 @@ require 'chef/dsl/data_query'
 require 'chef/dsl/registry_helper'
 require 'chef/dsl/reboot_pending'
 require 'chef/mixin/convert_to_class_name'
+require 'chef/resource/conditional/guard_interpreter'
 require 'chef/resource/conditional'
 require 'chef/resource/conditional_action_not_nothing'
-require 'chef/resource/conditional/guard_interpreter'
 require 'chef/resource_collection'
 require 'chef/resource_platform_map'
 require 'chef/node'
@@ -249,8 +249,8 @@ F
       @retry_delay = 2
       @not_if = []
       @only_if = []
-      @guard_interpreter = :default
       @source_line = nil
+      @guard_interpreter = :default
       @elapsed_time = 0
 
       @node = run_context ? deprecated_ivar(run_context.node, :node, :warn) : nil
@@ -561,9 +561,8 @@ F
     # * evaluates to true if the block is true, or if the command returns 0
     # * evaluates to false if the block is false, or if the command returns a non-zero exit code.
     def only_if(command=nil, opts={}, &block)
-      translated_command, translated_block = translate_command_block(command, opts, &block)
       if command || block_given?
-        @only_if << Conditional.only_if(translated_command, opts, &translated_block)
+        @only_if << Conditional.only_if(self, command, opts, &block)
       end
       @only_if
     end
@@ -583,9 +582,8 @@ F
     # * evaluates to true if the block is false, or if the command returns a non-zero exit status.
     # * evaluates to false if the block is true, or if the command returns a 0 exit status.
     def not_if(command=nil, opts={}, &block)
-      translated_command, translated_block = translate_command_block(command, opts, &block)
       if command || block_given?
-        @not_if << Conditional.not_if(translated_command, opts, &translated_block)
+        @not_if << Conditional.not_if(self, command, opts, &block)
       end
       @not_if
     end
@@ -831,19 +829,5 @@ F
         end
       end
     end
-
-    def translate_command_block(command, opts, &block)
-      guard_resource = guard_interpreter
-      guard_resource = nil if guard_interpreter == :default
-      if guard_resource && command && ! block_given?
-        evaluator = Conditional::GuardInterpreter.new(guard_resource, self)
-        block_attributes = opts.merge({:code => command})
-        translated_block = evaluator.to_block(block_attributes)
-        [nil, translated_block]
-      else
-        [command, block]
-      end
-    end
-
   end
 end
diff --git a/lib/chef/resource/conditional.rb b/lib/chef/resource/conditional.rb
index 60f65e14e2..94c4fe03f4 100644
--- a/lib/chef/resource/conditional.rb
+++ b/lib/chef/resource/conditional.rb
@@ -17,6 +17,7 @@
 #
 
 require 'chef/mixin/shell_out'
+require 'chef/resource/conditional/guard_interpreter'
 
 class Chef
   class Resource
@@ -29,12 +30,14 @@ class Chef
         private :new
       end
 
-      def self.not_if(command=nil, command_opts={}, &block)
-        new(:not_if, command, command_opts, &block)
+      def self.not_if(parent_resource, command=nil, command_opts={}, &block)
+        translated_command, translated_block = Chef::GuardInterpreter.translate_command_block(parent_resource, command, command_opts, &block)
+        new(:not_if, translated_command, command_opts, &translated_block)
       end
 
-      def self.only_if(command=nil, command_opts={}, &block)
-        new(:only_if, command, command_opts, &block)
+      def self.only_if(parent_resource, command=nil, command_opts={}, &block)
+        translated_command, translated_block = Chef::GuardInterpreter.translate_command_block(parent_resource, command, command_opts, &block)
+        new(:only_if, translated_command, command_opts, &translated_block)
       end
 
       attr_reader :positivity
diff --git a/lib/chef/resource/conditional/default_guard_interpreter.rb b/lib/chef/resource/conditional/default_guard_interpreter.rb
new file mode 100644
index 0000000000..198db6a7aa
--- /dev/null
+++ b/lib/chef/resource/conditional/default_guard_interpreter.rb
@@ -0,0 +1,34 @@
+#
+# Author:: Adam Edwards (<adamed@getchef.com>)
+# Copyright:: Copyright (c) 2014 Chef Software, Inc.
+# License:: Apache License, Version 2.0
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+class Chef
+  class DefaultGuardInterpreter
+
+    protected
+    def initialize
+    end
+
+    public
+    
+    def translate_command_block(command, opts, &block)
+      [command, block]
+    end
+    
+  end
+end
+      
diff --git a/lib/chef/resource/conditional/guard_interpreter.rb b/lib/chef/resource/conditional/guard_interpreter.rb
index f50fcf3f75..4f2fefb3cc 100644
--- a/lib/chef/resource/conditional/guard_interpreter.rb
+++ b/lib/chef/resource/conditional/guard_interpreter.rb
@@ -16,13 +16,35 @@
 # limitations under the License.
 #
 
-require 'chef/resource'
+require 'chef/resource/conditional/default_guard_interpreter'
 
 class Chef
-  class Resource::Conditional
-    class GuardInterpreter
+    class GuardInterpreter < DefaultGuardInterpreter
+
+      def self.translate_command_block(parent_resource, command, opts, &block)
+        evaluator = parent_resource.guard_interpreter == :default ?
+          DefaultGuardInterpreter.new :
+          new(parent_resource.guard_interpreter, parent_resource)
+
+        evaluator.translate_command_block(command, opts, &block)
+      end
+
+      def translate_command_block(command, opts, &block)
+        merge_inherited_attributes
+        if command && ! block_given?
+          block_attributes = opts.merge({:code => command})
+          translated_block = to_block(block_attributes)
+          [nil, translated_block]
+        else
+          super
+        end
+      end
+
+      protected
 
       def initialize(resource_symbol, parent_resource)
+        @parent_resource = parent_resource
+
         resource_class = get_resource_class(parent_resource, resource_symbol)
 
         raise ArgumentError, "Specified guard_interpreter resource #{resource_symbol.to_s} unknown for this platform" if resource_class.nil?
@@ -35,8 +57,6 @@ class Chef
         if ! @resource.kind_of?(Chef::Resource::Script)
           raise ArgumentError, "Specified guard interpreter class #{resource_class} must be a kind of Chef::Resource::Script resource"
         end
-
-        merge_inherited_attributes(parent_resource)
       end
 
       def evaluate_action(action=nil, &block)
@@ -78,17 +98,17 @@ class Chef
         end
       end
 
-      def merge_inherited_attributes(parent_resource)
+      def merge_inherited_attributes
         inherited_attributes = []
 
-        if parent_resource.respond_to?(:guard_inherited_attributes)
-          inherited_attributes = parent_resource.send(:guard_inherited_attributes)
+        if @parent_resource.respond_to?(:guard_inherited_attributes)
+          inherited_attributes = @parent_resource.send(:guard_inherited_attributes)
         end
         
         if inherited_attributes
           inherited_attributes.each do |attribute|
-            if parent_resource.respond_to?(attribute) && @resource.respond_to?(attribute)
-              parent_value = parent_resource.send(attribute)
+            if @parent_resource.respond_to?(attribute) && @resource.respond_to?(attribute)
+              parent_value = @parent_resource.send(attribute)
               child_value = @resource.send(attribute)
               if parent_value || child_value
                 @resource.send(attribute, parent_value)
@@ -98,5 +118,4 @@ class Chef
         end
       end
     end
-  end
 end
diff --git a/spec/support/shared/unit/script_resource.rb b/spec/support/shared/unit/script_resource.rb
index 51f6fc7215..8a1f66b706 100644
--- a/spec/support/shared/unit/script_resource.rb
+++ b/spec/support/shared/unit/script_resource.rb
@@ -66,14 +66,14 @@ shared_examples_for "a script resource" do
     it "when guard_interpreter is set to the default value, the guard command string should be evaluated by command execution and not through a resource" do
       Chef::Resource::Conditional.any_instance.should_not_receive(:evaluate_block)
       Chef::Resource::Conditional.any_instance.should_receive(:evaluate_command).and_return(true)
-      Chef::Resource::Conditional::GuardInterpreter.any_instance.should_not_receive(:evaluate_action)
+      Chef::GuardInterpreter.any_instance.should_not_receive(:evaluate_action)
       resource.only_if 'echo hi'
       resource.should_skip?(:run).should == nil
     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::Resource::Conditional::GuardInterpreter.any_instance.should_receive(:evaluate_action).and_return(true)
+      Chef::GuardInterpreter.any_instance.should_receive(:evaluate_action).and_return(true)
       resource.guard_interpreter :script
       resource.only_if 'echo hi'
       resource.should_skip?(:run).should == nil
diff --git a/spec/unit/resource/conditional_spec.rb b/spec/unit/resource/conditional_spec.rb
index 1be7bcea71..99d9aaa408 100644
--- a/spec/unit/resource/conditional_spec.rb
+++ b/spec/unit/resource/conditional_spec.rb
@@ -24,12 +24,13 @@ describe Chef::Resource::Conditional do
     Mixlib::ShellOut.any_instance.stub(:run_command).and_return(nil)
     @status = OpenStruct.new(:success? => true)
     Mixlib::ShellOut.any_instance.stub(:status).and_return(@status)
+    @parent_resource = Chef::Resource.new(nil, Chef::Node.new)
   end
 
   describe "when created as an `only_if`" do
     describe "after running a successful command" do
       before do
-        @conditional = Chef::Resource::Conditional.only_if("true")
+        @conditional = Chef::Resource::Conditional.only_if(@parent_resource, "true")
       end
 
       it "indicates that resource convergence should continue" do
@@ -40,7 +41,7 @@ describe Chef::Resource::Conditional do
     describe "after running a negative/false command" do
       before do
         @status.send("success?=", false)
-        @conditional = Chef::Resource::Conditional.only_if("false")
+        @conditional = Chef::Resource::Conditional.only_if(@parent_resource, "false")
       end
 
       it "indicates that resource convergence should not continue" do
@@ -50,7 +51,7 @@ describe Chef::Resource::Conditional do
 
     describe 'after running a command which timed out' do
       before do
-        @conditional = Chef::Resource::Conditional.only_if("false")
+        @conditional = Chef::Resource::Conditional.only_if(@parent_resource, "false")
         @conditional.stub(:shell_out).and_raise(Chef::Exceptions::CommandTimeout)
       end
 
@@ -66,7 +67,7 @@ describe Chef::Resource::Conditional do
 
     describe "after running a block that returns a truthy value" do
       before do
-        @conditional = Chef::Resource::Conditional.only_if { Object.new }
+        @conditional = Chef::Resource::Conditional.only_if(@parent_resource) { Object.new }
       end
 
       it "indicates that resource convergence should continue" do
@@ -76,7 +77,7 @@ describe Chef::Resource::Conditional do
 
     describe "after running a block that returns a falsey value" do
       before do
-        @conditional = Chef::Resource::Conditional.only_if { nil }
+        @conditional = Chef::Resource::Conditional.only_if(@parent_resource) { nil }
       end
 
       it "indicates that resource convergence should not continue" do
@@ -88,7 +89,7 @@ describe Chef::Resource::Conditional do
   describe "when created as a `not_if`" do
     describe "after running a successful/true command" do
       before do
-        @conditional = Chef::Resource::Conditional.not_if("true")
+        @conditional = Chef::Resource::Conditional.not_if(@parent_resource, "true")
       end
 
       it "indicates that resource convergence should not continue" do
@@ -99,7 +100,7 @@ describe Chef::Resource::Conditional do
     describe "after running a failed/false command" do
       before do
         @status.send("success?=", false)
-        @conditional = Chef::Resource::Conditional.not_if("false")
+        @conditional = Chef::Resource::Conditional.not_if(@parent_resource, "false")
       end
 
       it "indicates that resource convergence should continue" do
@@ -109,7 +110,7 @@ describe Chef::Resource::Conditional do
 
     describe 'after running a command which timed out' do
       before do
-        @conditional = Chef::Resource::Conditional.not_if("false")
+        @conditional = Chef::Resource::Conditional.not_if(@parent_resource,  "false")
         @conditional.stub(:shell_out).and_raise(Chef::Exceptions::CommandTimeout)
       end
 
@@ -125,7 +126,7 @@ describe Chef::Resource::Conditional do
 
     describe "after running a block that returns a truthy value" do
       before do
-        @conditional = Chef::Resource::Conditional.not_if { Object.new }
+        @conditional = Chef::Resource::Conditional.not_if(@parent_resource) { Object.new }
       end
 
       it "indicates that resource convergence should not continue" do
@@ -135,7 +136,7 @@ describe Chef::Resource::Conditional do
 
     describe "after running a block that returns a falsey value" do
       before do
-        @conditional = Chef::Resource::Conditional.not_if { nil }
+        @conditional = Chef::Resource::Conditional.not_if(@parent_resource) { nil }
       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 a567a434cf..0d678e7a5a 100644
--- a/spec/unit/resource/powershell_spec.rb
+++ b/spec/unit/resource/powershell_spec.rb
@@ -45,19 +45,19 @@ describe Chef::Resource::PowershellScript do
 
     it "should allow guard interpreter to be set to Chef::Resource::Script" do
       resource.guard_interpreter(:script)
-      allow_any_instance_of(Chef::Resource::Conditional::GuardInterpreter).to receive(:evaluate_action).and_return(false)
+      allow_any_instance_of(Chef::GuardInterpreter).to receive(:evaluate_action).and_return(false)
       resource.only_if("echo hi")
     end
 
     it "should allow guard interpreter to be set to Chef::Resource::Bash derived from Chef::Resource::Script" do
       resource.guard_interpreter(:bash)
-      allow_any_instance_of(Chef::Resource::Conditional::GuardInterpreter).to receive(:evaluate_action).and_return(false)
+      allow_any_instance_of(Chef::GuardInterpreter).to receive(:evaluate_action).and_return(false)
       resource.only_if("echo hi")
     end
 
     it "should allow guard interpreter to be set to Chef::Resource::PowershellScript derived indirectly from Chef::Resource::Script" do
       resource.guard_interpreter(:powershell_script)
-      allow_any_instance_of(Chef::Resource::Conditional::GuardInterpreter).to receive(:evaluate_action).and_return(false)
+      allow_any_instance_of(Chef::GuardInterpreter).to receive(:evaluate_action).and_return(false)
       resource.only_if("echo hi")
     end
   end
diff --git a/spec/unit/resource_spec.rb b/spec/unit/resource_spec.rb
index 734d92db42..8b46e56fcb 100644
--- a/spec/unit/resource_spec.rb
+++ b/spec/unit/resource_spec.rb
@@ -533,11 +533,17 @@ describe Chef::Resource do
         resource.guard_interpreter.should == :default
       end
 
+      it "if set to :default should return :default when read" do
+        resource.guard_interpreter(:default)
+        resource.guard_interpreter.should == :default
+      end
+
       it "should raise Chef::Exceptions::ValidationFailed on an attempt to set the guard_interpreter attribute to something other than a Symbol" do
         expect { resource.guard_interpreter('command_dot_com') }.to raise_error(Chef::Exceptions::ValidationFailed)
       end
 
       it "should not raise an exception when setting the guard interpreter attribute to a Symbol" do
+        Chef::GuardInterpreter.stub(:new).and_return(nil)
         expect { resource.guard_interpreter(:command_dot_com) }.not_to raise_error
       end
     end