From 77b6020a1c70b4643853f203739bc0fa1e04a8a7 Mon Sep 17 00:00:00 2001 From: Pete Higgins Date: Thu, 8 Oct 2020 17:11:41 -0700 Subject: Refactor resource guard interpreter. Signed-off-by: Pete Higgins --- .../resource_guard_interpreter.rb | 67 ++++++++----------- lib/chef/resource/powershell_script.rb | 2 +- spec/support/shared/unit/script_resource.rb | 4 +- .../support/shared/unit/windows_script_resource.rb | 2 +- .../resource_guard_interpreter_spec.rb | 22 +++--- spec/unit/resource/powershell_script_spec.rb | 78 ++-------------------- 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 -- cgit v1.2.1