Don't mask directory deletion errors (#4991)

closes #4988

2 files changed, 14 insertions, 2 deletions

diff --git a/lib/chef/provider/directory.rb b/lib/chef/provider/directory.rb
index 7cc05259b6..619ab5d8b6 100644
--- a/lib/chef/provider/directory.rb
+++ b/lib/chef/provider/directory.rb
@@ -145,7 +145,9 @@ class Chef
         if ::File.exists?(@new_resource.path)
           converge_by("delete existing directory #{@new_resource.path}") do
             if @new_resource.recursive == true
-              FileUtils.rm_rf(@new_resource.path)
+              # we don't use rm_rf here because it masks all errors, including
+              # IO errors or permission errors that would prvent the deletion
+              FileUtils.rm_r(@new_resource.path)
               Chef::Log.info("#{@new_resource} deleted #{@new_resource.path} recursively")
             else
               ::Dir.delete(@new_resource.path)
diff --git a/spec/unit/provider/directory_spec.rb b/spec/unit/provider/directory_spec.rb
index f8864af7f8..aebbaa6e81 100644
--- a/spec/unit/provider/directory_spec.rb
+++ b/spec/unit/provider/directory_spec.rb
@@ -227,7 +227,7 @@ describe Chef::Provider::Directory do
     end
   end
 
-  describe "#run_action(:create)" do
+  describe "#run_action(:delete)" do
     describe "when the directory exists" do
       it "deletes the directory" do
         directory.run_action(:delete)
@@ -238,6 +238,16 @@ describe Chef::Provider::Directory do
         directory.run_action(:delete)
         expect(new_resource).to be_updated
       end
+
+      it "does not use rm_rf which silently consumes errors" do
+        expect(FileUtils).not_to receive(:rm_rf)
+        expect(FileUtils).to receive(:rm_r)
+        # set recursive or FileUtils isn't used at all.
+        new_resource.recursive(true)
+        directory.run_action(:delete)
+        # reset back...
+        new_resource.recursive(false)
+      end
     end
 
     describe "when the directory does not exist" do