Refactor resource guard interpreter.refactor-resource-guard-interpreter

Signed-off-by: Pete Higgins <pete@peterhiggins.org>

6 files changed, 47 insertions, 128 deletions

diff --git a/lib/chef/guard_interpreter/resource_guard_interpreter.rb b/lib/chef/guard_interpreter/resource_guard_interpreter.rb
index 8528e4721d..a1e4205c03 100644
--- a/lib/chef/guard_interpreter/resource_guard_interpreter.rb
+++ b/lib/chef/guard_interpreter/resource_guard_interpreter.rb
@@ -20,19 +20,30 @@ require_relative "../guard_interpreter"
 
 class Chef
   class GuardInterpreter
-    class ResourceGuardInterpreter < DefaultGuardInterpreter
-
+    class ResourceGuardInterpreter
       def initialize(parent_resource, command, opts)
-        super(command, opts)
+        @command = command
+        @opts = opts
+
         @parent_resource = parent_resource
         @resource = get_interpreter_resource(parent_resource)
       end
 
+      # This class used to inherit from DefaultGuardInterpreter and it responds
+      # to #output, so leave this in for potential backwards compatibility.
+      def output
+        nil
+      end
+
       def evaluate
         # Add attributes inherited from the parent class
         # to the resource
         merge_inherited_attributes
 
+        @opts.each do |attribute, value|
+          @resource.send(attribute, value)
+        end
+
         # Only execute and script resources and use guard attributes.
         # The command to be executed on them are passed via different attributes.
         # Script resources use code attribute and execute resources use
@@ -42,9 +53,9 @@ class Chef
         # We need to make sure we check for Script first because any resource
         # that can get to here is an Execute resource.
         if @resource.is_a? Chef::Resource::Script
-          block_attributes = @command_opts.merge({ code: @command })
+          @resource.code @command
         else
-          block_attributes = @command_opts.merge({ command: @command })
+          @resource.command @command
         end
 
         # Handles cases like powershell_script where default
@@ -53,33 +64,24 @@ class Chef
         # the one attribute 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)
+          @resource.class.send(:get_default_attributes).each do |attribute, value|
+            @resource.send(attribute, value)
+          end
         end
 
-        resource_block = block_from_attributes(block_attributes)
-        evaluate_action(nil, &resource_block)
-      end
-
-      protected
-
-      def evaluate_action(action = nil, &block)
-        @resource.instance_eval(&block)
-
-        run_action = action || @resource.action
-
         begin
           # Coerce to an array to be safe. This could happen with a legacy
           # resource or something overriding the default_action code in a
           # subclass.
-          Array(run_action).each { |action_to_run| @resource.run_action(action_to_run) }
-          resource_updated = @resource.updated
+          Array(@resource.action).each { |action_to_run| @resource.run_action(action_to_run) }
+          @resource.updated
         rescue Mixlib::ShellOut::ShellCommandFailed
-          resource_updated = nil
+          nil
         end
-
-        resource_updated
       end
 
+      private
+
       def get_interpreter_resource(parent_resource)
         if parent_resource.nil? || parent_resource.node.nil?
           raise ArgumentError, "Node for guard resource parent must not be nil"
@@ -106,14 +108,6 @@ class Chef
         interpreter_resource
       end
 
-      def block_from_attributes(attributes)
-        Proc.new do
-          attributes.each_key do |attribute_name|
-            send(attribute_name, attributes[attribute_name]) if respond_to?(attribute_name)
-          end
-        end
-      end
-
       def merge_inherited_attributes
         inherited_attributes = []
 
@@ -121,15 +115,10 @@ class Chef
           inherited_attributes = @parent_resource.class.send(:guard_inherited_attributes)
         end
 
-        if inherited_attributes && !inherited_attributes.empty?
-          inherited_attributes.each do |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)
-              end
-            end
+        inherited_attributes.each do |attribute|
+          if @parent_resource.respond_to?(attribute) && @resource.respond_to?(attribute)
+            parent_value = @parent_resource.send(attribute)
+            @resource.send(attribute, parent_value)
           end
         end
       end
diff --git a/lib/chef/resource/powershell_script.rb b/lib/chef/resource/powershell_script.rb
index 784df68db1..eb72518009 100644
--- a/lib/chef/resource/powershell_script.rb
+++ b/lib/chef/resource/powershell_script.rb
@@ -76,7 +76,7 @@ class Chef
       # default for this resource, this method can be removed since
       # guard context and recipe resource context will have the
       # same behavior.
-      def self.get_default_attributes(opts)
+      def self.get_default_attributes
         { convert_boolean_return: true }
       end
     end
diff --git a/spec/support/shared/unit/script_resource.rb b/spec/support/shared/unit/script_resource.rb
index d1096f3ce9..b06551d170 100644
--- a/spec/support/shared/unit/script_resource.rb
+++ b/spec/support/shared/unit/script_resource.rb
@@ -56,7 +56,7 @@ 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
       expect_any_instance_of(Chef::Resource::Conditional).not_to receive(:evaluate_block)
-      expect_any_instance_of(Chef::GuardInterpreter::ResourceGuardInterpreter).not_to receive(:evaluate_action)
+      expect_any_instance_of(Chef::GuardInterpreter::ResourceGuardInterpreter).not_to receive(:evaluate)
       expect_any_instance_of(Chef::GuardInterpreter::DefaultGuardInterpreter).to receive(:evaluate).and_return(true)
       script_resource.only_if "echo hi"
       expect(script_resource.should_skip?(:run)).to eq(nil)
@@ -65,7 +65,7 @@ shared_examples_for "a script resource" do
     it "when a valid guard_interpreter resource is specified, a block should be used to evaluate the guard" do
       expect_any_instance_of(Chef::Resource::Conditional).not_to receive(:evaluate_block)
       expect_any_instance_of(Chef::GuardInterpreter::DefaultGuardInterpreter).not_to receive(:evaluate)
-      expect_any_instance_of(Chef::GuardInterpreter::ResourceGuardInterpreter).to receive(:evaluate_action).and_return(true)
+      expect_any_instance_of(Chef::GuardInterpreter::ResourceGuardInterpreter).to receive(:evaluate).and_return(true)
       script_resource.guard_interpreter :script
       script_resource.only_if "echo hi"
       expect(script_resource.should_skip?(:run)).to eq(nil)
diff --git a/spec/support/shared/unit/windows_script_resource.rb b/spec/support/shared/unit/windows_script_resource.rb
index 4a2f755952..2961b378e3 100644
--- a/spec/support/shared/unit/windows_script_resource.rb
+++ b/spec/support/shared/unit/windows_script_resource.rb
@@ -35,7 +35,7 @@ shared_examples_for "a Windows script resource" do
     end
 
     it "should use a resource to evaluate the guard when guard_interpreter is not specified" do
-      expect_any_instance_of(Chef::GuardInterpreter::ResourceGuardInterpreter).to receive(:evaluate_action).and_return(true)
+      expect_any_instance_of(Chef::GuardInterpreter::ResourceGuardInterpreter).to receive(:evaluate).and_return(true)
       expect_any_instance_of(Chef::GuardInterpreter::DefaultGuardInterpreter).not_to receive(:evaluate)
       windows_script_resource.only_if "echo hi"
       expect(windows_script_resource.should_skip?(:run)).to eq(nil)
diff --git a/spec/unit/guard_interpreter/resource_guard_interpreter_spec.rb b/spec/unit/guard_interpreter/resource_guard_interpreter_spec.rb
index 0d6289a999..0760bcee1d 100644
--- a/spec/unit/guard_interpreter/resource_guard_interpreter_spec.rb
+++ b/spec/unit/guard_interpreter/resource_guard_interpreter_spec.rb
@@ -93,8 +93,8 @@ describe Chef::GuardInterpreter::ResourceGuardInterpreter do
     end
 
     describe "script command opts switch" do
-      let(:command_opts) { {} }
-      let(:guard_interpreter) { Chef::GuardInterpreter::ResourceGuardInterpreter.new(parent_resource, "exit 0", command_opts) }
+      let(:guard_interpreter) { Chef::GuardInterpreter::ResourceGuardInterpreter.new(parent_resource, "exit 0", {}) }
+      let(:resource) { guard_interpreter.instance_variable_get("@resource") }
 
       context "resource is a Script" do
         context "and guard_interpreter is a :script" do
@@ -117,9 +117,9 @@ describe Chef::GuardInterpreter::ResourceGuardInterpreter do
             end
           end
 
-          it "merges to :code" do
-            expect(command_opts).to receive(:merge).with({ code: "exit 0" }).and_call_original
-            expect(guard_interpreter.evaluate).to eq(true)
+          it "assigns the comand to the resource's code property" do
+            guard_interpreter.evaluate
+            expect(resource.code).to eq("exit 0")
           end
         end
 
@@ -130,9 +130,9 @@ describe Chef::GuardInterpreter::ResourceGuardInterpreter do
             parent_resource
           end
 
-          it "merges to :code" do
-            expect(command_opts).to receive(:merge).with({ command: "exit 0" }).and_call_original
-            expect(guard_interpreter.evaluate).to eq(true)
+          it "assigns the comand to the resource's command property" do
+            guard_interpreter.evaluate
+            expect(resource.command).to eq("exit 0")
           end
         end
       end
@@ -144,9 +144,9 @@ describe Chef::GuardInterpreter::ResourceGuardInterpreter do
           parent_resource
         end
 
-        it "merges to :command" do
-          expect(command_opts).to receive(:merge).with({ command: "exit 0" }).and_call_original
-          expect(guard_interpreter.evaluate).to eq(true)
+        it "assigns the comand to the resource's command property" do
+          guard_interpreter.evaluate
+          expect(resource.command).to eq("exit 0")
         end
       end
 
diff --git a/spec/unit/resource/powershell_script_spec.rb b/spec/unit/resource/powershell_script_spec.rb
index 75fdf6ffc6..1c4b081593 100644
--- a/spec/unit/resource/powershell_script_spec.rb
+++ b/spec/unit/resource/powershell_script_spec.rb
@@ -47,81 +47,11 @@ describe Chef::Resource::PowershellScript do
     expect(resource.convert_boolean_return).to eq(false)
   end
 
-  context "when using guards" do
-    before(:each) do
-      allow(resource).to receive(:run_action)
-      allow(resource).to receive(:updated).and_return(true)
-    end
+  it "inherits exactly the :cwd, :environment, :group, :path, :user, :umask, :architecture, :elevated, :interpreter properties from a parent resource class" do
+    inherited_difference = Chef::Resource::PowershellScript.guard_inherited_attributes -
+      %i{cwd environment group path user umask architecture elevated interpreter}
 
-    it "inherits exactly the :cwd, :environment, :group, :path, :user, :umask, :architecture, :elevated, :interpreter properties from a parent resource class" do
-      inherited_difference = Chef::Resource::PowershellScript.guard_inherited_attributes -
-        %i{cwd environment group path user umask architecture elevated interpreter}
-
-      expect(inherited_difference).to eq([])
-    end
-
-    it "allows guard interpreter to be set to Chef::Resource::Script" do
-      resource.guard_interpreter(:script)
-      allow_any_instance_of(Chef::GuardInterpreter::ResourceGuardInterpreter).to receive(:evaluate_action).and_return(false)
-      resource.only_if("echo hi")
-    end
-
-    it "allows guard interpreter to be set to Chef::Resource::Bash derived from Chef::Resource::Script" do
-      resource.guard_interpreter(:bash)
-      allow_any_instance_of(Chef::GuardInterpreter::ResourceGuardInterpreter).to receive(:evaluate_action).and_return(false)
-      resource.only_if("echo hi")
-    end
-
-    it "allows 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::GuardInterpreter::ResourceGuardInterpreter).to receive(:evaluate_action).and_return(false)
-      resource.only_if("echo hi")
-    end
-
-    it "enables 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(:block_from_attributes).with(
-        { convert_boolean_return: true, code: "$true" }
-      ).and_return(Proc.new {})
-      resource.only_if("$true")
-    end
-
-    it "enables convert_boolean_return by default for guards in non-Chef::Resource::Script derived resources when no guard params are specified" do
-      node = Chef::Node.new
-      run_context = Chef::RunContext.new(node, nil, nil)
-      file_resource = Chef::Resource::File.new("idontexist", run_context)
-      file_resource.guard_interpreter :powershell_script
-
-      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 "enables 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(: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 "passes 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(: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 "passes 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(:block_from_attributes).with(
-        parameters_with_boolean_disabled
-      ).and_return(Proc.new {})
-      resource.only_if("$true", parameters_with_boolean_disabled)
-    end
+    expect(inherited_difference).to eq([])
   end
 
   context "as a script running in Windows-based scripting language" do