remove popen4 from cron resource + fix specs

Signed-off-by: Lamont Granquist <lamont@scriptkiddie.org>

3 files changed, 21 insertions, 60 deletions

diff --git a/lib/chef/provider/cron.rb b/lib/chef/provider/cron.rb
index 790849673d..bc1b1d97f3 100644
--- a/lib/chef/provider/cron.rb
+++ b/lib/chef/provider/cron.rb
@@ -201,29 +201,14 @@ class Chef
 
       def read_crontab
         crontab = nil
-        status = popen4("crontab -l -u #{new_resource.user}") do |pid, stdin, stdout, stderr|
-          crontab = stdout.read
-        end
-        if status.exitstatus > 1
-          raise Chef::Exceptions::Cron, "Error determining state of #{new_resource.name}, exit: #{status.exitstatus}"
-        end
-        crontab
+        so = shell_out!("crontab -l -u #{new_resource.user}", returns: [0, 1])
+        return nil if so.exitstatus == 1
+        so.stdout
       end
 
       def write_crontab(crontab)
         write_exception = false
-        status = popen4("crontab -u #{new_resource.user} -", :waitlast => true) do |pid, stdin, stdout, stderr|
-          begin
-            stdin.write crontab
-          rescue Errno::EPIPE => e
-            # popen4 could yield while child has already died.
-            write_exception = true
-            Chef::Log.debug("#{e.message}")
-          end
-        end
-        if status.exitstatus > 0 || write_exception
-          raise Chef::Exceptions::Cron, "Error updating state of #{new_resource.name}, exit: #{status.exitstatus}"
-        end
+        shell_out!("crontab -u #{new_resource.user} -", input: crontab)
       end
 
       def get_crontab_entry
diff --git a/spec/functional/resource/cron_spec.rb b/spec/functional/resource/cron_spec.rb
index f5948191c5..156babcffe 100644
--- a/spec/functional/resource/cron_spec.rb
+++ b/spec/functional/resource/cron_spec.rb
@@ -1,7 +1,7 @@
 # encoding: UTF-8
 #
 # Author:: Kaustubh Deorukhkar (<kaustubh@clogeny.com>)
-# Copyright:: Copyright 2013-2016, Chef Software Inc.
+# Copyright:: Copyright 2013-2017, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -161,7 +161,7 @@ describe Chef::Resource::Cron, :requires_root, :unix_only do
     end
 
     def cron_create_should_raise_exception
-      expect { new_resource.run_action(:create) }.to raise_error(Chef::Exceptions::Cron, /Error updating state of #{new_resource.name}, exit: 1/)
+      expect { new_resource.run_action(:create) }.to raise_error(Mixlib::ShellOut::ShellCommandFailed)
       cron_should_not_exists(new_resource.name)
     end
 
diff --git a/spec/unit/provider/cron_spec.rb b/spec/unit/provider/cron_spec.rb
index 64916ef454..19dc19b26c 100644
--- a/spec/unit/provider/cron_spec.rb
+++ b/spec/unit/provider/cron_spec.rb
@@ -880,8 +880,7 @@ MAILTO=foo@example.com
 
   describe "read_crontab" do
     before :each do
-      @status = double("Status", :exitstatus => 0)
-      @stdout = StringIO.new(<<-CRONTAB)
+      @stdout = <<-CRONTAB
 0 2 * * * /some/other/command
 
 # Chef Name: something else
@@ -889,11 +888,12 @@ MAILTO=foo@example.com
 
 # Another comment
       CRONTAB
-      allow(@provider).to receive(:popen4).and_yield(1234, StringIO.new, @stdout, StringIO.new).and_return(@status)
+      @status = double("Status", exitstatus: 0, stdout: @stdout)
+      allow(@provider).to receive(:shell_out!).and_return(@status)
     end
 
     it "should call crontab -l with the user" do
-      expect(@provider).to receive(:popen4).with("crontab -l -u #{@new_resource.user}").and_return(@status)
+      expect(@provider).to receive(:shell_out!).with("crontab -l -u #{@new_resource.user}", returns: [0, 1]).and_return(@status)
       @provider.send(:read_crontab)
     end
 
@@ -910,60 +910,36 @@ MAILTO=foo@example.com
     end
 
     it "should return nil if the user has no crontab" do
-      status = double("Status", :exitstatus => 1)
-      allow(@provider).to receive(:popen4).and_return(status)
+      @status = double("Status", exitstatus: 1, stdout: "")
+      allow(@provider).to receive(:shell_out!).and_return(@status)
       expect(@provider.send(:read_crontab)).to eq(nil)
     end
 
     it "should raise an exception if another error occurs" do
-      status = double("Status", :exitstatus => 2)
-      allow(@provider).to receive(:popen4).and_return(status)
-      expect do
-        @provider.send(:read_crontab)
-      end.to raise_error(Chef::Exceptions::Cron, "Error determining state of #{@new_resource.name}, exit: 2")
+      @status = double("Status", exitstatus: 2)
+      allow(@provider).to receive(:shell_out!).and_raise(Mixlib::ShellOut::ShellCommandFailed)
+      expect { @provider.send(:read_crontab) }.to raise_error(Mixlib::ShellOut::ShellCommandFailed)
     end
   end
 
   describe "write_crontab" do
     before :each do
-      @status = double("Status", :exitstatus => 0)
-      @stdin = StringIO.new
-      allow(@provider).to receive(:popen4).and_yield(1234, @stdin, StringIO.new, StringIO.new).and_return(@status)
+      @status = double("Status", exitstatus: 0)
+      allow(@provider).to receive(:shell_out!).and_return(@status)
     end
 
     it "should call crontab for the user" do
-      expect(@provider).to receive(:popen4).with("crontab -u #{@new_resource.user} -", :waitlast => true).and_return(@status)
+      expect(@provider).to receive(:shell_out!).with("crontab -u #{@new_resource.user} -", input: "Foo").and_return(@status)
       @provider.send(:write_crontab, "Foo")
     end
 
-    it "should write the given string to the crontab command" do
-      @provider.send(:write_crontab, "Foo\n# wibble\n wah!!")
-      expect(@stdin.string).to eq("Foo\n# wibble\n wah!!")
-    end
-
     it "should raise an exception if the command returns non-zero" do
-      allow(@status).to receive(:exitstatus).and_return(1)
-      expect do
-        @provider.send(:write_crontab, "Foo")
-      end.to raise_error(Chef::Exceptions::Cron, "Error updating state of #{@new_resource.name}, exit: 1")
-    end
-
-    it "should raise an exception if the command die's and parent tries to write" do
-      class WriteErrPipe
-        def write(str)
-          raise Errno::EPIPE, "Test"
-        end
-      end
-      allow(@status).to receive(:exitstatus).and_return(1)
-      allow(@provider).to receive(:popen4).and_yield(1234, WriteErrPipe.new, StringIO.new, StringIO.new).and_return(@status)
-
-      expect(Chef::Log).to receive(:debug).with("Broken pipe - Test")
-
+      @status = double("Status", exitstatus: 1)
+      allow(@provider).to receive(:shell_out!).and_raise(Mixlib::ShellOut::ShellCommandFailed)
       expect do
         @provider.send(:write_crontab, "Foo")
-      end.to raise_error(Chef::Exceptions::Cron, "Error updating state of #{@new_resource.name}, exit: 1")
+      end.to raise_error(Mixlib::ShellOut::ShellCommandFailed)
     end
-
   end
 
   describe "weekday_in_crontab" do