fix edge condition in old timeout APIslcg/deprecate-shell-out-methods

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

3 files changed, 14 insertions, 9 deletions

diff --git a/lib/chef/mixin/shell_out.rb b/lib/chef/mixin/shell_out.rb
index 16f289b443..f38c6e262d 100644
--- a/lib/chef/mixin/shell_out.rb
+++ b/lib/chef/mixin/shell_out.rb
@@ -131,15 +131,20 @@ class Chef
       # @api private
       def self.maybe_add_timeout(obj, options)
         options = options.dup
-        force = options.delete(:argument_that_will_go_away_in_chef_15_so_do_not_use_it)
-        return options if options.key?(:timeout)
+        force = options.delete(:argument_that_will_go_away_in_chef_15_so_do_not_use_it) # remove in Chef-15
+        # historically resources have not properly declared defaults on their timeouts, so a default default of 900s was enforced here
+        default_val = 900
+        if !force
+          return options if options.key?(:timeout) # leave this line in Chef-15, delete the rest of the conditional
+        else
+          default_val = options[:timeout] if options.key?(:timeout) # delete in Chef-15
+        end
         # note that we can't define an empty Chef::Resource::LWRPBase because that breaks descendants tracker, so we'd have to instead require the file here, which would pull in half
         # of chef, so instead convert to using strings.  once descendants tracker is gone, we can just declare the empty classes instead and use `is_a?` against the symbols.
         # (be nice if ruby supported strings in `is_a?` for looser coupling).
+        # FIXME: just use `if obj.respond_to?(:new_resource) && obj.new_resource.respond_to?(:timeout) && !options.key?(:timeout)` in Chef 15
         if obj.respond_to?(:new_resource) && ( force || ( obj.class.ancestors.map(&:name).include?("Chef::Provider") && !obj.class.ancestors.map(&:name).include?("Chef::Resource::LWRPBase") && !obj.class.ancestors.map(&:name).include?("Chef::Resource::ActionClass") && obj.new_resource.respond_to?(:timeout) && !options.key?(:timeout) ) )
-          # FIXME: just use `obj.respond_to?(:new_resource) && obj.new_resource.respond_to?(:timeout) && !options.key?(:timeout)` in Chef 15
-          # historically resources have not properly declared defaults on their timeouts, so a default default of 900s was enforced here
-          options[:timeout] = obj.new_resource.timeout ? obj.new_resource.timeout.to_f : 900
+          options[:timeout] = obj.new_resource.timeout ? obj.new_resource.timeout.to_f : default_val
         end
         options
       end
diff --git a/spec/unit/mixin/shell_out_spec.rb b/spec/unit/mixin/shell_out_spec.rb
index 4c9d53428f..2fef051b29 100644
--- a/spec/unit/mixin/shell_out_spec.rb
+++ b/spec/unit/mixin/shell_out_spec.rb
@@ -408,7 +408,7 @@ describe Chef::Mixin::ShellOut do
       end
     end
 
-    describe "deprecated timeouts" do
+    describe "deprecated timeouts" do # Chef 15: delete me
       let(:new_resource) { Chef::Resource::Package.new("foo") }
       let(:provider) { new_resource.provider_for_action(:install) }
 
@@ -428,7 +428,7 @@ describe Chef::Mixin::ShellOut do
         end
         it "#{method} overrides the new_resource.timeout with the timeout option" do
           new_resource.timeout(99)
-          expect(provider).to receive(stubbed_method).with("foo", timeout: 1)
+          expect(provider).to receive(stubbed_method).with("foo", timeout: 99)
           provider.send(method, "foo", timeout: 1)
         end
         it "#{method} defaults to 900 seconds and preserves options" do
@@ -441,7 +441,7 @@ describe Chef::Mixin::ShellOut do
         end
         it "#{method} overrides the new_resource.timeout with the timeout option and preseves options" do
           new_resource.timeout(99)
-          expect(provider).to receive(stubbed_method).with("foo", timeout: 1, env: nil)
+          expect(provider).to receive(stubbed_method).with("foo", timeout: 99, env: nil)
           provider.send(method, "foo", timeout: 1, env: nil)
         end
       end
diff --git a/spec/unit/provider/package_spec.rb b/spec/unit/provider/package_spec.rb
index 2eb7cf63e1..904e339e47 100644
--- a/spec/unit/provider/package_spec.rb
+++ b/spec/unit/provider/package_spec.rb
@@ -942,7 +942,7 @@ describe "Chef::Provider::Package - Multi" do
     end
 
     [ :shell_out_with_timeout, :shell_out_with_timeout! ].each do |method|
-      stubbed_method = method == :shell_out_with_timeout! ? :shell_out! : :shell_out
+      stubbed_method = method == :shell_out_with_timeout! ? :shell_out_compacted! : :shell_out_compacted
       [ %w{command arg1 arg2}, "command arg1 arg2" ].each do |command|
         it "#{method} defaults to 900 seconds" do
           expect(provider).to receive(stubbed_method).with(*command, timeout: 900)