Merge pull request #7788 from chef/lcg/chef-15-shell-out-removal

Do the shell_out deprecations for Chef-15.

5 files changed, 3 insertions, 302 deletions

diff --git a/lib/chef/mixin/shell_out.rb b/lib/chef/mixin/shell_out.rb
index 25a8ee989c..1ca272b805 100644
--- a/lib/chef/mixin/shell_out.rb
+++ b/lib/chef/mixin/shell_out.rb
@@ -46,65 +46,6 @@ class Chef
       # a thousand unit tests.
       #
 
-      def shell_out_compact(*args, **options)
-        Chef.deprecated(:shell_out, "shell_out_compact should be replaced by shell_out")
-        if options.empty?
-          shell_out(*args)
-        else
-          shell_out(*args, **options)
-        end
-      end
-
-      def shell_out_compact!(*args, **options)
-        Chef.deprecated(:shell_out, "shell_out_compact! should be replaced by shell_out!")
-        if options.empty?
-          shell_out!(*args)
-        else
-          shell_out!(*args, **options)
-        end
-      end
-
-      def shell_out_compact_timeout(*args, **options)
-        Chef.deprecated(:shell_out, "shell_out_compact_timeout should be replaced by shell_out")
-        if options.empty?
-          shell_out(*args, argument_that_will_go_away_in_chef_15_so_do_not_use_it: true)
-        else
-          shell_out(*args, argument_that_will_go_away_in_chef_15_so_do_not_use_it: true, **options)
-        end
-      end
-
-      def shell_out_compact_timeout!(*args, **options)
-        Chef.deprecated(:shell_out, "shell_out_compact_timeout! should be replaced by shell_out!")
-        if options.empty?
-          shell_out!(*args, argument_that_will_go_away_in_chef_15_so_do_not_use_it: true)
-        else
-          shell_out!(*args, argument_that_will_go_away_in_chef_15_so_do_not_use_it: true, **options)
-        end
-      end
-
-      def shell_out_with_systems_locale(*args, **options)
-        Chef.deprecated(:shell_out, "shell_out_with_systems_locale should be replaced by shell_out with the default_env option set to false")
-        if options.empty?
-          shell_out(*args, default_env: false)
-        else
-          shell_out(*args, default_env: false, **options)
-        end
-      end
-
-      def shell_out_with_systems_locale!(*args, **options)
-        Chef.deprecated(:shell_out, "shell_out_with_systems_locale! should be replaced by shell_out! with the default_env option set to false")
-        if options.empty?
-          shell_out!(*args, default_env: false)
-        else
-          shell_out!(*args, default_env: false, **options)
-        end
-      end
-
-      def a_to_s(*args)
-        Chef.deprecated(:shell_out, "a_to_s is deprecated use shell_out with splat-args")
-        args.flatten.reject { |i| i.nil? || i == "" }.map(&:to_s).join(" ")
-      end
-
       def shell_out(*args, **options)
         options = options.dup
         options = Chef::Mixin::ShellOut.maybe_add_timeout(self, options)
@@ -131,19 +72,10 @@ 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) # 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) ) )
+        return options if options.key?(:timeout)
+        if obj.respond_to?(:new_resource) && obj.new_resource.respond_to?(:timeout) && !options.key?(:timeout)
           options[:timeout] = obj.new_resource.timeout ? obj.new_resource.timeout.to_f : default_val
         end
         options
@@ -168,11 +100,6 @@ class Chef
         options
       end
 
-      def clean_array(*args)
-        Chef.deprecated(:shell_out, "do not call clean_array directly, just use shell_out with splat args or an array")
-        Chef::Mixin::ShellOut.clean_array(*args)
-      end
-
       private
 
       # this SHOULD be used for setting up expectations in rspec, see banner comment at top.
diff --git a/lib/chef/provider/package.rb b/lib/chef/provider/package.rb
index f6172f1edf..44d7fed6f4 100644
--- a/lib/chef/provider/package.rb
+++ b/lib/chef/provider/package.rb
@@ -674,14 +674,6 @@ class Chef
           true
         end
       end
-
-      def shell_out_with_timeout(*command_args)
-        shell_out_compact_timeout(*command_args)
-      end
-
-      def shell_out_with_timeout!(*command_args)
-        shell_out_compact_timeout!(*command_args)
-      end
     end
   end
 end
diff --git a/spec/functional/mixin/shell_out_spec.rb b/spec/functional/mixin/shell_out_spec.rb
index b3e9bf796d..df428afdcd 100644
--- a/spec/functional/mixin/shell_out_spec.rb
+++ b/spec/functional/mixin/shell_out_spec.rb
@@ -20,36 +20,6 @@ require "spec_helper"
 describe Chef::Mixin::ShellOut do
   include Chef::Mixin::ShellOut
 
-  describe "shell_out_with_systems_locale" do
-    before do
-      Chef::Config[:treat_deprecation_warnings_as_errors] = false
-    end
-
-    describe "when environment['LC_ALL'] is not set" do
-      it "should use the default shell_out setting" do
-        cmd = if windows?
-                shell_out_with_systems_locale("echo %LC_ALL%")
-              else
-                shell_out_with_systems_locale("echo $LC_ALL")
-              end
-
-        expect(cmd.stdout.chomp).to match_environment_variable("LC_ALL")
-      end
-    end
-
-    describe "when environment['LC_ALL'] is set" do
-      it "should use the option's setting" do
-        cmd = if windows?
-                shell_out_with_systems_locale("echo %LC_ALL%", environment: { "LC_ALL" => "POSIX" })
-              else
-                shell_out_with_systems_locale("echo $LC_ALL", environment: { "LC_ALL" => "POSIX" })
-              end
-
-        expect(cmd.stdout.chomp).to eq "POSIX"
-      end
-    end
-  end
-
   describe "shell_out default_env: false" do
     describe "when environment['LC_ALL'] is not set" do
       it "should use the default shell_out setting" do
diff --git a/spec/unit/mixin/shell_out_spec.rb b/spec/unit/mixin/shell_out_spec.rb
index 7f6021d911..a22060b25f 100644
--- a/spec/unit/mixin/shell_out_spec.rb
+++ b/spec/unit/mixin/shell_out_spec.rb
@@ -51,11 +51,8 @@ describe Chef::Mixin::ShellOut do
     let(:retobj) { instance_double(Mixlib::ShellOut, "error!" => false) }
     let(:cmd) { "echo '#{rand(1000)}'" }
 
-    [ :shell_out, :shell_out_compact, :shell_out_compact_timeout, :shell_out!, :shell_out_compact!, :shell_out_compact_timeout! ].each do |method|
+    [ :shell_out, :shell_out! ].each do |method|
       describe "##{method}" do
-        before do
-          Chef::Config[:treat_deprecation_warnings_as_errors] = false
-        end
 
         describe "when the last argument is a Hash" do
           describe "and environment is an option" do
@@ -177,69 +174,6 @@ describe Chef::Mixin::ShellOut do
       end
     end
 
-    describe "#shell_out_with_systems_locale" do
-      before do
-        Chef::Config[:treat_deprecation_warnings_as_errors] = false
-      end
-
-      describe "when the last argument is a Hash" do
-        describe "and environment is an option" do
-          it "should not change environment['LC_ALL'] when set to nil" do
-            options = { environment: { "LC_ALL" => nil } }
-            expect(Chef::Mixin::ShellOut).to receive(:shell_out_command).with(cmd, options).and_return(true)
-            shell_out_obj.shell_out_with_systems_locale(cmd, options)
-          end
-
-          it "should not change environment['LC_ALL'] when set to non-nil" do
-            options = { environment: { "LC_ALL" => "en_US.UTF-8" } }
-            expect(Chef::Mixin::ShellOut).to receive(:shell_out_command).with(cmd, options).and_return(true)
-            shell_out_obj.shell_out_with_systems_locale(cmd, options)
-          end
-
-          it "should no longer set environment['LC_ALL'] to nil when 'LC_ALL' not present" do
-            options = { environment: { "HOME" => "/Users/morty" } }
-            expect(Chef::Mixin::ShellOut).to receive(:shell_out_command).with(cmd, options).and_return(true)
-            shell_out_obj.shell_out_with_systems_locale(cmd, options)
-          end
-        end
-
-        describe "and env is an option" do
-          it "should not change env when set to nil" do
-            options = { env: { "LC_ALL" => nil } }
-            expect(Chef::Mixin::ShellOut).to receive(:shell_out_command).with(cmd, options).and_return(true)
-            shell_out_obj.shell_out_with_systems_locale(cmd, options)
-          end
-
-          it "should not change env when set to non-nil" do
-            options = { env: { "LC_ALL" => "en_US.UTF-8" } }
-            expect(Chef::Mixin::ShellOut).to receive(:shell_out_command).with(cmd, options).and_return(true)
-            shell_out_obj.shell_out_with_systems_locale(cmd, options)
-          end
-
-          it "should no longer set env['LC_ALL'] to nil when 'LC_ALL' not present" do
-            options = { env: { "HOME" => "/Users/morty" } }
-            expect(Chef::Mixin::ShellOut).to receive(:shell_out_command).with(cmd, options).and_return(true)
-            shell_out_obj.shell_out_with_systems_locale(cmd, options)
-          end
-        end
-
-        describe "and no env/environment option is present" do
-          it "should no longer add environment option and set environment['LC_ALL'] to nil" do
-            options = { user: "morty" }
-            expect(Chef::Mixin::ShellOut).to receive(:shell_out_command).with(cmd, options).and_return(true)
-            shell_out_obj.shell_out_with_systems_locale(cmd, options)
-          end
-        end
-      end
-
-      describe "when the last argument is not a Hash" do
-        it "should no longer add environment options and set environment['LC_ALL'] to nil" do
-          expect(Chef::Mixin::ShellOut).to receive(:shell_out_command).with(cmd).and_return(true)
-          shell_out_obj.shell_out_with_systems_locale(cmd)
-        end
-      end
-    end
-
     describe "#shell_out default_env: false" do
 
       describe "when the last argument is a Hash" do
@@ -300,22 +234,6 @@ describe Chef::Mixin::ShellOut do
       end
     end
 
-    describe "deprecations" do
-      [ :shell_out_with_systems_locale, :shell_out_compact, :shell_out_compact_timeout, :shell_out_with_systems_locale!, :shell_out_compact!, :shell_out_compact_timeout! ].each do |method|
-        it "should not respond to #{method} in Chef-15", chef: ">= 15" do
-          expect(shell_out_obj.respond_to?(method)).to be false
-        end
-      end
-
-      it "removed shell_out_with_timeout from Chef::Provider::Package", chef: ">= 15" do
-        expect(Chef::Provider::Package.instance_methods + Chef::Provider::Package.private_instance_methods).not_to include(:shell_out_with_timeout)
-      end
-
-      it "removed shell_out_with_timeout! from Chef::Provider::Package", chef: ">= 15" do
-        expect(Chef::Provider::Package.instance_methods + Chef::Provider::Package.private_instance_methods).not_to include(:shell_out_with_timeout!)
-      end
-    end
-
     describe "Custom Resource timeouts" do
       class CustomResource < Chef::Resource
         provides :whatever
@@ -329,17 +247,6 @@ describe Chef::Mixin::ShellOut do
       let(:new_resource) { CustomResource.new("foo") }
       let(:provider) { new_resource.provider_for_action(:install) }
 
-      describe "on Chef-14", chef: "< 15" do
-        it "doesn't add timeout for shell_out" do
-          expect(provider).to receive(:shell_out_compacted).with("foo")
-          provider.shell_out("foo")
-        end
-        it "doesn't add timeout for shell_out!" do
-          expect(provider).to receive(:shell_out_compacted!).with("foo")
-          provider.shell_out!("foo")
-        end
-      end
-
       describe "on Chef-15", chef: ">= 15" do
         [ :shell_out, :shell_out! ].each do |method|
           stubbed_method = (method == :shell_out) ? :shell_out_compacted : :shell_out_compacted!
@@ -407,44 +314,5 @@ describe Chef::Mixin::ShellOut do
         end
       end
     end
-
-    describe "deprecated timeouts" do # Chef 15: delete me
-      let(:new_resource) { Chef::Resource::Package.new("foo") }
-      let(:provider) { new_resource.provider_for_action(:install) }
-
-      before(:each) do
-        Chef::Config[:treat_deprecation_warnings_as_errors] = false
-      end
-
-      [ :shell_out_compact_timeout, :shell_out_compact_timeout! ].each do |method|
-        stubbed_method = (method == :shell_out_compact_timeout) ? :shell_out_compacted : :shell_out_compacted!
-        it "#{method} defaults to 900 seconds" do
-          expect(provider).to receive(stubbed_method).with("foo", timeout: 900)
-          provider.send(method, "foo")
-        end
-        it "#{method} overrides the default timeout with its options" do
-          expect(provider).to receive(stubbed_method).with("foo", timeout: 1)
-          provider.send(method, "foo", timeout: 1)
-        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: 99)
-          provider.send(method, "foo", timeout: 1)
-        end
-        it "#{method} defaults to 900 seconds and preserves options" do
-          expect(provider).to receive(stubbed_method).with("foo", env: nil, timeout: 900)
-          provider.send(method, "foo", env: nil)
-        end
-        it "#{method} overrides the default timeout with its options and preserves options" do
-          expect(provider).to receive(stubbed_method).with("foo", timeout: 1, env: nil)
-          provider.send(method, "foo", timeout: 1, env: nil)
-        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: 99, env: nil)
-          provider.send(method, "foo", timeout: 1, env: nil)
-        end
-      end
-    end
   end
 end
diff --git a/spec/unit/provider/package_spec.rb b/spec/unit/provider/package_spec.rb
index 181ac40a1b..f085e09ce4 100644
--- a/spec/unit/provider/package_spec.rb
+++ b/spec/unit/provider/package_spec.rb
@@ -543,24 +543,6 @@ describe "Subclass with use_multipackage_api" do
     expect(provider.use_multipackage_api?).to be true
   end
 
-  context "#a_to_s utility for subclasses" do
-    before(:each) do
-      Chef::Config[:treat_deprecation_warnings_as_errors] = false
-    end
-
-    it "converts varargs of strings to a single string" do
-      expect(provider.send(:a_to_s, "a", nil, "b", "", "c", " ", "d e", "f-g")).to eq("a b c   d e f-g")
-    end
-
-    it "converts an array of strings to a single string" do
-      expect(provider.send(:a_to_s, ["a", nil, "b", "", "c", " ", "d e", "f-g"])).to eq("a b c   d e f-g")
-    end
-
-    it "converts a mishmash of array args to a single string" do
-      expect(provider.send(:a_to_s, "a", [ nil, "b", "", [ "c" ] ], " ", [ "d e", "f-g" ])).to eq("a b c   d e f-g")
-    end
-  end
-
   it "when user passes string to package_name, passes arrays to install_package" do
     new_resource.package_name "vim"
     new_resource.version nil
@@ -951,44 +933,6 @@ describe "Chef::Provider::Package - Multi" do
     end
   end
 
-  describe "shell_out helpers" do
-    before(:each) do
-      Chef::Config[:treat_deprecation_warnings_as_errors] = false
-    end
-
-    [ :shell_out_with_timeout, :shell_out_with_timeout! ].each do |method|
-      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)
-          provider.send(method, *command)
-        end
-        it "#{method} overrides the default timeout with its options" do
-          expect(provider).to receive(stubbed_method).with(*command, timeout: 1)
-          provider.send(method, *command, timeout: 1)
-        end
-        it "#{method} overrides both timeouts with the new_resource.timeout" do
-          new_resource.timeout(99)
-          expect(provider).to receive(stubbed_method).with(*command, timeout: 99)
-          provider.send(method, *command, timeout: 1)
-        end
-        it "#{method} defaults to 900 seconds and preserves options" do
-          expect(provider).to receive(stubbed_method).with(*command, env: nil, timeout: 900)
-          provider.send(method, *command, env: nil)
-        end
-        it "#{method} overrides the default timeout with its options and preserves options" do
-          expect(provider).to receive(stubbed_method).with(*command, timeout: 1, env: nil)
-          provider.send(method, *command, timeout: 1, env: nil)
-        end
-        it "#{method} overrides both timeouts with the new_resource.timeout and preseves options" do
-          new_resource.timeout(99)
-          expect(provider).to receive(stubbed_method).with(*command, timeout: 99, env: nil)
-          provider.send(method, *command, timeout: 1, env: nil)
-        end
-      end
-    end
-  end
-
   describe "version_compare" do
     it "tests equality" do
       expect(provider.version_compare("1.3", "1.3")).to eql(0)