Merge pull request #2223 from opscode/sersut/execute-guard-interpreter

Guards of execute resource doesn't inherit command options from its parent resource

10 files changed, 183 insertions, 56 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 2795a4126c..2b6e280bea 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -201,6 +201,8 @@
 * Restore the deprecation logic of #valid_actions in LWRPs until Chef 13.
 * Now that we don't allow unforked chef-client interval runs, remove the reloading of previously defined LWRPs.
 * Use shell_out to determine Chef::Config[:internal_locale], fix CentOS locale detection bug.
+* `only_if` and `not_if` attributes of `execute` resource now inherits the parent resource's
+  attributes when set to a `String`.
 
 ## Last Release: 11.14.2
 
diff --git a/lib/chef/guard_interpreter/resource_guard_interpreter.rb b/lib/chef/guard_interpreter/resource_guard_interpreter.rb
index 229a8502c7..346b585d8c 100644
--- a/lib/chef/guard_interpreter/resource_guard_interpreter.rb
+++ b/lib/chef/guard_interpreter/resource_guard_interpreter.rb
@@ -33,10 +33,19 @@ class Chef
         # 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})
+        # 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
+        # command attribute. Moreover script resources are also execute
+        # resources. Here we make sure @command is assigned to the right
+        # attribute by checking the type of the resources.
+        # We need to make sure we check for Script first because any resource
+        # that can get to here is an Execute resource.
+        if @parent_resource.is_a? Chef::Resource::Script
+          block_attributes = @command_opts.merge({:code => @command})
+        else
+          block_attributes = @command_opts.merge({:command => @command})
+        end
 
         # Handles cases like powershell_script where default
         # attributes are different when used in a guard vs. not. For
@@ -79,8 +88,8 @@ class Chef
           raise ArgumentError, "Specified guard_interpreter resource #{parent_resource.guard_interpreter.to_s} unknown for this platform"
         end
 
-        if ! resource_class.ancestors.include?(Chef::Resource::Script)
-          raise ArgumentError, "Specified guard interpreter class #{resource_class} must be a kind of Chef::Resource::Script resource"
+        if ! resource_class.ancestors.include?(Chef::Resource::Execute)
+          raise ArgumentError, "Specified guard interpreter class #{resource_class} must be a kind of Chef::Resource::Execute resource"
         end
 
         empty_events = Chef::EventDispatch::Dispatcher.new
diff --git a/lib/chef/resource.rb b/lib/chef/resource.rb
index 70abfbcdb0..55de8f059c 100644
--- a/lib/chef/resource.rb
+++ b/lib/chef/resource.rb
@@ -229,6 +229,8 @@ F
 
     attr_reader :elapsed_time
 
+    attr_reader :default_guard_interpreter
+
     # Each notify entry is a resource/action pair, modeled as an
     # Struct with a #resource and #action member
 
@@ -250,7 +252,13 @@ F
       @not_if = []
       @only_if = []
       @source_line = nil
-      @guard_interpreter = :default
+      # We would like to raise an error when the user gives us a guard
+      # interpreter and a ruby_block to the guard. In order to achieve this
+      # we need to understand when the user overrides the default guard
+      # interpreter. Therefore we store the default separately in a different
+      # attribute.
+      @guard_interpreter = nil
+      @default_guard_interpreter = :default
       @elapsed_time = 0
       @sensitive = false
     end
@@ -410,11 +418,15 @@ F
     end
 
     def guard_interpreter(arg=nil)
-      set_or_return(
-        :guard_interpreter,
-        arg,
-        :kind_of => Symbol
-      )
+      if arg.nil?
+        @guard_interpreter || @default_guard_interpreter
+      else
+        set_or_return(
+          :guard_interpreter,
+          arg,
+          :kind_of => Symbol
+        )
+      end
     end
 
     # Sets up a notification from this resource to the resource specified by +resource_spec+.
diff --git a/lib/chef/resource/conditional.rb b/lib/chef/resource/conditional.rb
index 324c5a4676..8960a4d57f 100644
--- a/lib/chef/resource/conditional.rb
+++ b/lib/chef/resource/conditional.rb
@@ -59,8 +59,10 @@ class Chef
           @guard_interpreter = new_guard_interpreter(@parent_resource, @command, @command_opts, &@block)
           @block = nil
         when nil
-          # we should have a block if we get here
-          if @parent_resource.guard_interpreter != :default
+          # We should have a block if we get here
+          # Check to see if the user set the guard_interpreter on the parent resource. Note that
+          # this error will not be raised when using the default_guard_interpreter
+          if @parent_resource.guard_interpreter != @parent_resource.default_guard_interpreter
             msg = "#{@parent_resource.name} was given a guard_interpreter of #{@parent_resource.guard_interpreter}, "
             msg << "but not given a command as a string. guard_interpreter does not support blocks (because they just contain ruby)."
             raise ArgumentError, msg
diff --git a/lib/chef/resource/execute.rb b/lib/chef/resource/execute.rb
index 7c4fa48c0a..ae118b1c9e 100644
--- a/lib/chef/resource/execute.rb
+++ b/lib/chef/resource/execute.rb
@@ -35,12 +35,12 @@ class Chef
         @cwd = nil
         @environment = nil
         @group = nil
-        @path = nil
         @returns = 0
         @timeout = nil
         @user = nil
         @allowed_actions.push(:run)
         @umask = nil
+        @default_guard_interpreter = :execute
       end
 
       def umask(arg=nil)
@@ -93,14 +93,6 @@ class Chef
         )
       end
 
-      def path(arg=nil)
-        set_or_return(
-          :path,
-          arg,
-          :kind_of => [ Array ]
-        )
-      end
-
       def returns(arg=nil)
         set_or_return(
           :returns,
@@ -125,6 +117,30 @@ class Chef
         )
       end
 
+      def self.set_guard_inherited_attributes(*inherited_attributes)
+        @class_inherited_attributes = inherited_attributes
+      end
+
+      def self.guard_inherited_attributes(*inherited_attributes)
+        # Similar to patterns elsewhere, return attributes from this
+        # class and superclasses as a form of inheritance
+        ancestor_attributes = []
+
+        if superclass.respond_to?(:guard_inherited_attributes)
+          ancestor_attributes = superclass.guard_inherited_attributes
+        end
+
+        ancestor_attributes.concat(@class_inherited_attributes ? @class_inherited_attributes : []).uniq
+      end
+
+      set_guard_inherited_attributes(
+       :cwd,
+       :environment,
+       :group,
+       :user,
+       :umask
+       )
+
     end
   end
 end
diff --git a/lib/chef/resource/script.rb b/lib/chef/resource/script.rb
index 6f66fb9094..8cc9c6f0c5 100644
--- a/lib/chef/resource/script.rb
+++ b/lib/chef/resource/script.rb
@@ -58,31 +58,6 @@ class Chef
         )
       end
 
-      def self.set_guard_inherited_attributes(*inherited_attributes)
-        @class_inherited_attributes = inherited_attributes
-      end
-
-      def self.guard_inherited_attributes(*inherited_attributes)
-        # Similar to patterns elsewhere, return attributes from this
-        # class and superclasses as a form of inheritance
-        ancestor_attributes = []
-
-        if superclass.respond_to?(:guard_inherited_attributes)
-          ancestor_attributes = superclass.guard_inherited_attributes
-        end
-
-        ancestor_attributes.concat(@class_inherited_attributes ? @class_inherited_attributes : []).uniq
-      end
-
-      set_guard_inherited_attributes(
-       :cwd,
-       :environment,
-       :group,
-       :path,
-       :user,
-       :umask
-       )
-
     end
   end
 end
diff --git a/spec/functional/resource/execute_spec.rb b/spec/functional/resource/execute_spec.rb
new file mode 100644
index 0000000000..ff358fe045
--- /dev/null
+++ b/spec/functional/resource/execute_spec.rb
@@ -0,0 +1,113 @@
+#
+# Author:: Serdar Sutay (<serdar@opscode.com>)
+# Copyright:: Copyright (c) 2014 Opscode, 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.
+#
+
+require 'spec_helper'
+require 'functional/resource/base'
+
+describe Chef::Resource::Execute do
+  let(:execute_resource) {
+    exec_resource = Chef::Resource::Execute.new("foo_resource", run_context)
+
+    exec_resource.environment(resource_environment) if resource_environment
+    exec_resource.cwd(resource_cwd) if resource_cwd
+    exec_resource.command("echo hello")
+    if guard
+      if guard_options
+        exec_resource.only_if(guard, guard_options)
+      else
+        exec_resource.only_if(guard)
+      end
+    end
+    exec_resource
+  }
+
+  let(:resource_environment) { nil }
+  let(:resource_cwd) { nil }
+  let(:guard) { nil }
+  let(:guard_options) { nil }
+
+  describe "when guard is ruby block" do
+    it "guard can still run" do
+      execute_resource.only_if do
+        true
+      end
+      execute_resource.run_action(:run)
+      execute_resource.should be_updated_by_last_action
+    end
+  end
+
+  describe "when parent resource sets :cwd" do
+    let(:resource_cwd) { CHEF_SPEC_DATA }
+
+    let(:guard) { %{ruby -e 'exit 1 unless File.exists?("./big_json_plus_one.json")'} }
+
+    it "guard inherits :cwd from resource" do
+      execute_resource.run_action(:run)
+      execute_resource.should be_updated_by_last_action
+    end
+  end
+
+  describe "when parent resource sets :environment" do
+    let(:resource_environment) do
+      {
+        "SAWS_SECRET"  => "supersecret",
+        "SAWS_KEY"     => "qwerty"
+      }
+    end
+
+    # We use ruby command so that we don't need to deal with platform specific
+    # commands while testing execute resource. We set it so that the resource
+    # will be updated if the ENV variable is set to what we are intending
+    let(:guard) { %{ruby -e 'exit 1 if ENV["SAWS_SECRET"] != "supersecret"'} }
+
+    it "guard inherits :environment value from resource" do
+      execute_resource.run_action(:run)
+      execute_resource.should be_updated_by_last_action
+    end
+
+    describe "when guard sets additional values in the :environment" do
+      let(:guard) { %{ruby -e 'exit 1 if ENV["SGCE_SECRET"] != "regularsecret"'} }
+
+      let(:guard_options) do
+        {
+          :environment => { 'SGCE_SECRET' => "regularsecret" }
+        }
+      end
+
+      it "guard sees merged value for in its ENV" do
+        execute_resource.run_action(:run)
+        execute_resource.should be_updated_by_last_action
+      end
+    end
+
+    describe "when guard sets same value in the :environment" do
+      let(:guard) { %{ruby -e 'exit 1 if ENV["SAWS_SECRET"] != "regularsecret"'} }
+
+      let(:guard_options) do
+        {
+          :environment => { 'SAWS_SECRET' => "regularsecret" }
+        }
+      end
+
+      it "guard sees value from guard options in its ENV" do
+        execute_resource.run_action(:run)
+        execute_resource.should be_updated_by_last_action
+      end
+    end
+  end
+end
diff --git a/spec/support/shared/unit/execute_resource.rb b/spec/support/shared/unit/execute_resource.rb
index 609e77ad63..298e0c5baf 100644
--- a/spec/support/shared/unit/execute_resource.rb
+++ b/spec/support/shared/unit/execute_resource.rb
@@ -76,11 +76,6 @@ shared_examples_for "an execute resource" do
     @resource.group.should eql(1)
   end
 
-  it "should accept an array for the execution path" do
-    @resource.path ["woot"]
-    @resource.path.should eql(["woot"])
-  end
-
   it "should accept an integer for the return code" do
     @resource.returns 1
     @resource.returns.should eql(1)
@@ -112,7 +107,6 @@ shared_examples_for "an execute resource" do
       @resource.cwd("/tmp/")
       @resource.environment({ :one => :two })
       @resource.group("legos")
-      @resource.path(["/var/local/"])
       @resource.returns(1)
       @resource.user("root")
     end
@@ -122,4 +116,3 @@ shared_examples_for "an execute resource" do
     end
   end
 end
-
diff --git a/spec/unit/resource/execute_spec.rb b/spec/unit/resource/execute_spec.rb
index 8c8dcfb6ca..e1728b7892 100644
--- a/spec/unit/resource/execute_spec.rb
+++ b/spec/unit/resource/execute_spec.rb
@@ -23,4 +23,9 @@ describe Chef::Resource::Execute do
   let(:resource_instance_name) { "some command" }
   let(:execute_resource) { Chef::Resource::Execute.new(resource_instance_name) }
   it_behaves_like "an execute resource"
+
+  it "default guard interpreter should be :execute interpreter" do
+    execute_resource.guard_interpreter.should be(:execute)
+  end
+
 end
diff --git a/spec/unit/resource_spec.rb b/spec/unit/resource_spec.rb
index 692345c943..5dfc17f333 100644
--- a/spec/unit/resource_spec.rb
+++ b/spec/unit/resource_spec.rb
@@ -349,7 +349,7 @@ describe Chef::Resource do
         :updated_by_last_action, :before, :supports,
         :noop, :ignore_failure, :name, :source_line,
         :action, :retries, :retry_delay, :elapsed_time,
-        :guard_interpreter, :sensitive ]
+        :default_guard_interpreter, :guard_interpreter, :sensitive ]
       (hash.keys - expected_keys).should == []
       (expected_keys - hash.keys).should == []
       hash[:name].should eql("funk")