diff options
author | Adam Edwards <adamed@opscode.com> | 2014-03-28 11:14:56 -0700 |
---|---|---|
committer | Adam Edwards <adamed@opscode.com> | 2014-03-29 00:21:11 -0700 |
commit | e0b02d69aaf9f408cfd9dbe9ea47561f694815bf (patch) | |
tree | 23d29bb34f82f8da8e622668c5e89d27ada6ba81 | |
parent | b57795c468a366ff8d755e22f1364c10a5d27af9 (diff) | |
download | chef-e0b02d69aaf9f408cfd9dbe9ea47561f694815bf.tar.gz |
CR feedback: refactor guard interpreter logic out of Resource into Conditional
-rw-r--r-- | lib/chef/resource.rb | 24 | ||||
-rw-r--r-- | lib/chef/resource/conditional.rb | 11 | ||||
-rw-r--r-- | lib/chef/resource/conditional/default_guard_interpreter.rb | 34 | ||||
-rw-r--r-- | lib/chef/resource/conditional/guard_interpreter.rb | 41 | ||||
-rw-r--r-- | spec/support/shared/unit/script_resource.rb | 4 | ||||
-rw-r--r-- | spec/unit/resource/conditional_spec.rb | 21 | ||||
-rw-r--r-- | spec/unit/resource/powershell_spec.rb | 6 | ||||
-rw-r--r-- | spec/unit/resource_spec.rb | 6 |
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 |