CR feedback: move inheritance to class level, add tests for convert_boolean_true

6 files changed, 119 insertions, 23 deletions

diff --git a/lib/chef/guard_interpreter/resource_guard_interpreter.rb b/lib/chef/guard_interpreter/resource_guard_interpreter.rb
index 946ee2ba49..0a88d75d5c 100644
--- a/lib/chef/guard_interpreter/resource_guard_interpreter.rb
+++ b/lib/chef/guard_interpreter/resource_guard_interpreter.rb
@@ -92,11 +92,11 @@ class Chef
       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.class.respond_to?(:guard_inherited_attributes)
+          inherited_attributes = @parent_resource.class.send(:guard_inherited_attributes)
         end
-        
-        if inherited_attributes
+
+        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)
diff --git a/lib/chef/resource/powershell_script.rb b/lib/chef/resource/powershell_script.rb
index 9d4a58358c..08568b6964 100644
--- a/lib/chef/resource/powershell_script.rb
+++ b/lib/chef/resource/powershell_script.rb
@@ -21,9 +21,10 @@ class Chef
   class Resource
     class PowershellScript < Chef::Resource::WindowsScript
 
+      add_guard_inherited_attributes(:architecture)
+
       def initialize(name, run_context=nil)
         super(name, run_context, :powershell_script, "powershell.exe")
-        set_guard_inherited_attributes([:architecture])
         @convert_boolean_return = false
       end
 
diff --git a/lib/chef/resource/script.rb b/lib/chef/resource/script.rb
index 5619d4e9aa..6ef74ce77a 100644
--- a/lib/chef/resource/script.rb
+++ b/lib/chef/resource/script.rb
@@ -32,17 +32,6 @@ class Chef
         @code = nil
         @interpreter = nil
         @flags = nil
-        @guard_inherited_attributes = []
-
-        set_guard_inherited_attributes(
-          [
-           :cwd,
-           :environment,
-           :group,
-           :path,
-           :user,
-           :umask
-          ])
       end
 
       def code(arg=nil)
@@ -69,16 +58,32 @@ class Chef
         )
       end
 
-      protected
-
-      def set_guard_inherited_attributes(inherited_attributes)
-        @guard_inherited_attributes.concat(inherited_attributes).uniq
+      def self.add_guard_inherited_attributes(*inherited_attributes)
+        @class_inherited_attributes ||= []
+        @class_inherited_attributes = inherited_attributes if inherited_attributes
       end
 
-      def guard_inherited_attributes
-        @guard_inherited_attributes
+      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
 
+      add_guard_inherited_attributes(
+       :cwd,
+       :environment,
+       :group,
+       :path,
+       :user,
+       :umask
+       )
+
     end
   end
 end
diff --git a/spec/functional/resource/powershell_spec.rb b/spec/functional/resource/powershell_spec.rb
index d6df8b811e..9b5c498ea7 100644
--- a/spec/functional/resource/powershell_spec.rb
+++ b/spec/functional/resource/powershell_spec.rb
@@ -321,6 +321,20 @@ describe Chef::Resource::WindowsScript::PowershellScript, :windows_only do
       resource.should_skip?(:run).should be_false
     end
 
+    it "inherits cwd from the parent resource for only_if" do
+      custom_cwd = "#{ENV['SystemRoot']}\\system32\\drivers\\etc"
+      resource.cwd custom_cwd
+      resource.only_if  "exit ! [int32]($pwd.path -eq '#{custom_cwd}')"
+      resource.should_skip?(:run).should be_false
+    end
+
+    it "inherits cwd from the parent resource for not_if" do
+      custom_cwd = "#{ENV['SystemRoot']}\\system32\\drivers\\etc"
+      resource.cwd custom_cwd
+      resource.not_if  "exit ! [int32]($pwd.path -eq '#{custom_cwd}')"
+      resource.should_skip?(:run).should be_true
+    end
+
     it "evaluates a 64-bit resource with a 64-bit guard and interprets boolean false as zero status code", :windows64_only do
       resource.architecture :x86_64
       resource.only_if  "exit [int32]($env:PROCESSOR_ARCHITECTURE -ne 'AMD64')"
@@ -330,7 +344,7 @@ describe Chef::Resource::WindowsScript::PowershellScript, :windows_only do
     it "evaluates a 64-bit resource with a 64-bit guard and interprets boolean true as nonzero status code", :windows64_only do
       resource.architecture :x86_64
       resource.only_if  "exit [int32]($env:PROCESSOR_ARCHITECTURE -eq 'AMD64')"
-    resource.should_skip?(:run).should be_true
+      resource.should_skip?(:run).should be_true
     end
 
     it "evaluates a 32-bit resource with a 32-bit guard and interprets boolean false as zero status code" do
@@ -344,6 +358,57 @@ describe Chef::Resource::WindowsScript::PowershellScript, :windows_only do
       resource.only_if  "exit [int32]($env:PROCESSOR_ARCHITECTURE -eq 'X86')"
       resource.should_skip?(:run).should be_true
     end
+
+    it "evaluates a simple boolean false as nonzero status code when convert_boolean_return is true for only_if" do
+      resource.only_if  "$false"
+      resource.should_skip?(:run).should be_true
+    end
+
+    it "evaluates a simple boolean true as nonzero status code when convert_boolean_return is true for not_if" do
+      resource.convert_boolean_return true
+      resource.not_if  "$false"
+      resource.should_skip?(:run).should be_false
+    end
+
+    it "evaluates a simple boolean true as 0 status code when convert_boolean_return is true for only_if" do
+      resource.convert_boolean_return true
+      resource.only_if  "$true"
+      resource.should_skip?(:run).should be_false
+    end
+
+    it "evaluates a simple boolean true as 0 status code when convert_boolean_return is true for not_if" do
+      resource.convert_boolean_return true
+      resource.not_if  "$true"
+      resource.should_skip?(:run).should be_true
+    end
+
+    it "evaluates a 32-bit resource with a 32-bit guard and interprets boolean false as zero status code using convert_boolean_return for only_if" do
+      resource.convert_boolean_return true
+      resource.architecture :i386
+      resource.only_if  "$env:PROCESSOR_ARCHITECTURE -eq 'X86'"
+      resource.should_skip?(:run).should be_false
+    end
+
+    it "evaluates a 32-bit resource with a 32-bit guard and interprets boolean false as zero status code using convert_boolean_return for not_if" do
+      resource.convert_boolean_return true
+      resource.architecture :i386
+      resource.not_if  "$env:PROCESSOR_ARCHITECTURE -ne 'X86'"
+      resource.should_skip?(:run).should be_false
+    end
+
+    it "evaluates a 32-bit resource with a 32-bit guard and interprets boolean true as nonzero status code using convert_boolean_return for only_if" do
+      resource.convert_boolean_return true
+      resource.architecture :i386
+      resource.only_if  "$env:PROCESSOR_ARCHITECTURE -ne 'X86'"
+      resource.should_skip?(:run).should be_true
+    end
+
+    it "evaluates a 32-bit resource with a 32-bit guard and interprets boolean true as nonzero status code using convert_boolean_return for not_if" do
+      resource.convert_boolean_return true
+      resource.architecture :i386
+      resource.not_if  "$env:PROCESSOR_ARCHITECTURE -eq 'X86'"
+      resource.should_skip?(:run).should be_true
+    end
   end
 
   def get_script_output
diff --git a/spec/support/shared/unit/script_resource.rb b/spec/support/shared/unit/script_resource.rb
index add99d45bd..7cdd117e40 100644
--- a/spec/support/shared/unit/script_resource.rb
+++ b/spec/support/shared/unit/script_resource.rb
@@ -63,6 +63,13 @@ shared_examples_for "a script resource" do
       resource.code 'echo hi'
     end
 
+    it "inherits exactly the :cwd, :environment, :group, :path, :user, and :umask attributes from a parent resource class" do
+      inherited_difference = Chef::Resource::Script.guard_inherited_attributes -
+        [:cwd, :environment, :group, :path, :user, :umask ]
+
+      inherited_difference.should == []
+    end
+
     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)
diff --git a/spec/unit/resource/powershell_spec.rb b/spec/unit/resource/powershell_spec.rb
index 52012a0f41..8929d6a9d0 100644
--- a/spec/unit/resource/powershell_spec.rb
+++ b/spec/unit/resource/powershell_spec.rb
@@ -36,6 +36,17 @@ describe Chef::Resource::PowershellScript do
     @resource.should be_a_kind_of(Chef::Resource::PowershellScript)
   end
 
+  it "should set convert_boolean_return to false by default" do
+    @resource.convert_boolean_return.should == false
+  end
+
+  it "should return the value for convert_boolean_return that was set" do
+    @resource.convert_boolean_return true
+    @resource.convert_boolean_return.should == true
+    @resource.convert_boolean_return false
+    @resource.convert_boolean_return.should == false
+  end
+
   context "when using guards" do
     let(:resource) { @resource }
     before(:each) do
@@ -43,6 +54,13 @@ describe Chef::Resource::PowershellScript do
       resource.stub(:updated).and_return(true)
     end
 
+    it "inherits exactly the :cwd, :environment, :group, :path, :user, :umask, and :architecture attributes from a parent resource class" do
+      inherited_difference = Chef::Resource::PowershellScript.guard_inherited_attributes -
+        [:cwd, :environment, :group, :path, :user, :umask, :architecture ]
+
+      inherited_difference.should == []
+    end
+
     it "should allow 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)