Merge pull request #5233 from coderanger/notifstring

Add a warning for guard blocks that return a non-empty string

2 files changed, 55 insertions, 1 deletions

diff --git a/lib/chef/resource/conditional.rb b/lib/chef/resource/conditional.rb
index cdb9f13c45..452718cae8 100644
--- a/lib/chef/resource/conditional.rb
+++ b/lib/chef/resource/conditional.rb
@@ -103,7 +103,15 @@ class Chef
       end
 
       def evaluate_block
-        @block.call
+        @block.call.tap do |rv|
+          if rv.is_a?(String) && !rv.empty?
+            # This is probably a mistake:
+            #   not_if { "command" }
+            sanitized_rv = @parent_resource.sensitive ? "a string" : rv.inspect
+            Chef::Log.warn("#{@positivity} block for #{@parent_resource} returned #{sanitized_rv}, did you mean to run a command?" +
+              (@parent_resource.sensitive ? "" : " If so use '#{@positivity} #{sanitized_rv}' in your code."))
+          end
+        end
       end
 
       def short_description
diff --git a/spec/unit/resource/conditional_spec.rb b/spec/unit/resource/conditional_spec.rb
index b34b4200e6..e84b0980c4 100644
--- a/spec/unit/resource/conditional_spec.rb
+++ b/spec/unit/resource/conditional_spec.rb
@@ -124,6 +124,29 @@ describe Chef::Resource::Conditional do
         expect(@conditional.continue?).to be_falsey
       end
     end
+
+    describe "after running a block that returns a string value" do
+      before do
+        @conditional = Chef::Resource::Conditional.only_if(@parent_resource) { "some command" }
+      end
+
+      it "logs a warning" do
+        expect(Chef::Log).to receive(:warn).with("only_if block for [] returned \"some command\", did you mean to run a command? If so use 'only_if \"some command\"' in your code.")
+        @conditional.evaluate
+      end
+    end
+
+    describe "after running a block that returns a string value on a sensitive resource" do
+      before do
+        @parent_resource.sensitive(true)
+        @conditional = Chef::Resource::Conditional.only_if(@parent_resource) { "some command" }
+      end
+
+      it "logs a warning" do
+        expect(Chef::Log).to receive(:warn).with("only_if block for [] returned a string, did you mean to run a command?")
+        @conditional.evaluate
+      end
+    end
   end
 
   describe "when created as a `not_if`" do
@@ -204,5 +227,28 @@ describe Chef::Resource::Conditional do
         expect(@conditional.continue?).to be_truthy
       end
     end
+
+    describe "after running a block that returns a string value" do
+      before do
+        @conditional = Chef::Resource::Conditional.not_if(@parent_resource) { "some command" }
+      end
+
+      it "logs a warning" do
+        expect(Chef::Log).to receive(:warn).with("not_if block for [] returned \"some command\", did you mean to run a command? If so use 'not_if \"some command\"' in your code.")
+        @conditional.evaluate
+      end
+    end
+
+    describe "after running a block that returns a string value on a sensitive resource" do
+      before do
+        @parent_resource.sensitive(true)
+        @conditional = Chef::Resource::Conditional.not_if(@parent_resource) { "some command" }
+      end
+
+      it "logs a warning" do
+        expect(Chef::Log).to receive(:warn).with("not_if block for [] returned a string, did you mean to run a command?")
+        @conditional.evaluate
+      end
+    end
   end
 end