Commit f596b16a authored by Adam Edwards's avatar Adam Edwards
Browse files

CR feedback: move command evaluation to guard interpreter

parent 718de5b0
......@@ -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
......
......@@ -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)
......
......@@ -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
......
......@@ -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'
......
......@@ -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
......
......@@ -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
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment