add internal flag to shell_out and execute resource

this will wind up replacing shell_out_with_systems_locale.

also fixes an issue in the sysctl provider.

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

9 files changed, 54 insertions, 46 deletions

diff --git a/lib/chef/mixin/shell_out.rb b/lib/chef/mixin/shell_out.rb
index 57d5be4adc..cb6e02bd7e 100644
--- a/lib/chef/mixin/shell_out.rb
+++ b/lib/chef/mixin/shell_out.rb
@@ -1,6 +1,6 @@
 #--
 # Author:: Daniel DeLeo (<dan@chef.io>)
-# Copyright:: Copyright 2010-2017, Chef Software Inc.
+# Copyright:: Copyright 2010-2018, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -104,13 +104,17 @@ class Chef
       # generally must support UTF-8 unicode.
       def shell_out(*args, **options)
         options = options.dup
-        env_key = options.has_key?(:env) ? :env : :environment
-        options[env_key] = {
-          "LC_ALL" => Chef::Config[:internal_locale],
-          "LANGUAGE" => Chef::Config[:internal_locale],
-          "LANG" => Chef::Config[:internal_locale],
-          env_path => sanitized_path,
-        }.update(options[env_key] || {})
+        internal = options.delete(:internal)
+        internal = true if internal.nil?
+        if internal
+          env_key = options.key?(:env) ? :env : :environment
+          options[env_key] = {
+            "LC_ALL" => Chef::Config[:internal_locale],
+            "LANGUAGE" => Chef::Config[:internal_locale],
+            "LANG" => Chef::Config[:internal_locale],
+            env_path => sanitized_path,
+          }.update(options[env_key] || {})
+        end
         shell_out_command(*args, **options)
       end
 
@@ -121,14 +125,12 @@ class Chef
         cmd
       end
 
-      def shell_out_with_systems_locale(*command_args)
-        shell_out_command(*command_args)
+      def shell_out_with_systems_locale(*args, **options) # FIXME: deprecate
+        shell_out(*args, internal: false, **options)
       end
 
-      def shell_out_with_systems_locale!(*command_args)
-        cmd = shell_out_with_systems_locale(*command_args)
-        cmd.error!
-        cmd
+      def shell_out_with_systems_locale!(*args, **options) # FIXME: deprecate
+        shell_out!(*args, internal: false, **options)
       end
 
       # Helper for subclasses to convert an array of string args into a string.  It
diff --git a/lib/chef/provider/execute.rb b/lib/chef/provider/execute.rb
index 6872e2d67d..265932db06 100644
--- a/lib/chef/provider/execute.rb
+++ b/lib/chef/provider/execute.rb
@@ -1,6 +1,6 @@
 #
 # Author:: Adam Jacob (<adam@chef.io>)
-# Copyright:: Copyright 2008-2017, Chef Software Inc.
+# Copyright:: Copyright 2008-2018, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -27,7 +27,7 @@ class Chef
 
       provides :execute
 
-      def_delegators :new_resource, :command, :returns, :environment, :user, :domain, :password, :group, :cwd, :umask, :creates, :elevated
+      def_delegators :new_resource, :command, :returns, :environment, :user, :domain, :password, :group, :cwd, :umask, :creates, :elevated, :internal
 
       def load_current_resource
         current_resource = Chef::Resource::Execute.new(new_resource.name)
@@ -55,7 +55,7 @@ class Chef
 
         converge_by("execute #{description}") do
           begin
-            shell_out_with_systems_locale!(command, opts)
+            shell_out!(command, opts)
           rescue Mixlib::ShellOut::ShellCommandFailed
             if sensitive?
               ex = Mixlib::ShellOut::ShellCommandFailed.new("Command execution failed. STDOUT/STDERR suppressed for sensitive resource")
@@ -97,6 +97,7 @@ class Chef
         opts[:group]       = group if group
         opts[:cwd]         = cwd if cwd
         opts[:umask]       = umask if umask
+        opts[:internal]    = internal
         opts[:log_level]   = :info
         opts[:log_tag]     = new_resource.to_s
         if (logger.info? || live_stream?) && !sensitive?
diff --git a/lib/chef/resource/execute.rb b/lib/chef/resource/execute.rb
index f7313cae40..07daa1727f 100644
--- a/lib/chef/resource/execute.rb
+++ b/lib/chef/resource/execute.rb
@@ -1,7 +1,7 @@
 #
 # Author:: Adam Jacob (<adam@chef.io>)
 # Author:: Tyler Cloke (<tyler@chef.io>)
-# Copyright:: Copyright 2008-2017, Chef Software Inc.
+# Copyright:: Copyright 2008-2018, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -64,6 +64,8 @@ class Chef
 
       property :group, [ String, Integer ]
       property :live_stream, [ TrueClass, FalseClass ], default: false
+      # internal defaults to `false` so that the command execution more exactly matches what the user gets on the command line without magic
+      property :internal, [ TrueClass, FalseClass ], desired_state: false, default: false
       property :returns, [ Integer, Array ], default: 0
       property :timeout, [ Integer, Float ]
       property :user, [ String, Integer ]
diff --git a/lib/chef/resource/sysctl.rb b/lib/chef/resource/sysctl.rb
index aab1c055c1..6cc525e062 100644
--- a/lib/chef/resource/sysctl.rb
+++ b/lib/chef/resource/sysctl.rb
@@ -1,6 +1,6 @@
 #
 # Copyright:: 2018, Webb Agile Solutions Ltd.
-# Copyright:: 2018, Chef Software Inc.
+# Copyright:: 2018-2018, Chef Software Inc.
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -84,6 +84,7 @@ class Chef
 
           execute "Load sysctl values" do
             command "sysctl #{'-e ' if new_resource.ignore_error}-p"
+            internal true
             action :run
           end
         end
@@ -98,6 +99,7 @@ class Chef
             end
 
             execute "Load sysctl values" do
+              internal true
               command "sysctl -p"
               action :run
             end
diff --git a/spec/functional/resource/execute_spec.rb b/spec/functional/resource/execute_spec.rb
index c0956c5594..4dd3110629 100644
--- a/spec/functional/resource/execute_spec.rb
+++ b/spec/functional/resource/execute_spec.rb
@@ -1,6 +1,6 @@
 #
 # Author:: Serdar Sutay (<serdar@chef.io>)
-# Copyright:: Copyright 2014-2017, Chef Software Inc.
+# Copyright:: Copyright 2014-2018, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -51,7 +51,7 @@ describe Chef::Resource::Execute do
 
       # why_run mode doesn't disable the updated_by_last_action logic, so we really have to look at the provider action
       # to see if why_run correctly disabled the resource.  It should shell_out! for the guard but not the resource.
-      expect_any_instance_of(Chef::Provider::Execute).to receive(:shell_out_with_systems_locale!).once
+      expect_any_instance_of(Chef::Provider::Execute).to receive(:shell_out!).once
 
       resource.only_if guard
       resource.run_action(:run)
diff --git a/spec/unit/mixin/shell_out_spec.rb b/spec/unit/mixin/shell_out_spec.rb
index 9b12969026..4927a8ff54 100644
--- a/spec/unit/mixin/shell_out_spec.rb
+++ b/spec/unit/mixin/shell_out_spec.rb
@@ -226,7 +226,7 @@ describe Chef::Mixin::ShellOut do
 
       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(shell_out_obj).to receive(:shell_out_command).with(cmd).and_return(true)
+          expect(shell_out_obj).to receive(:shell_out_command).with(cmd, {}).and_return(true)
           shell_out_obj.shell_out_with_systems_locale(cmd)
         end
       end
diff --git a/spec/unit/provider/apt_update_spec.rb b/spec/unit/provider/apt_update_spec.rb
index e1ab0085ce..35a47c45e5 100644
--- a/spec/unit/provider/apt_update_spec.rb
+++ b/spec/unit/provider/apt_update_spec.rb
@@ -1,6 +1,6 @@
 #
 # Author:: Thom May (<thom@chef.io>)
-# Copyright:: Copyright (c) 2016-2017, Chef Software Inc.
+# Copyright:: Copyright (c) 2016-2018, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -45,7 +45,7 @@ describe Chef::Provider::AptUpdate do
     before do
       FileUtils.rmdir config_dir
       expect(File.exist?(config_dir)).to be false
-      allow_any_instance_of(Chef::Provider::Execute).to receive(:shell_out_with_systems_locale!).with("apt-get -q update", anything())
+      allow_any_instance_of(Chef::Provider::Execute).to receive(:shell_out!).with("apt-get -q update", anything())
     end
 
     it "should create the directory" do
@@ -64,7 +64,7 @@ describe Chef::Provider::AptUpdate do
   describe "#action_update" do
     it "should update the apt cache" do
       provider.load_current_resource
-      expect_any_instance_of(Chef::Provider::Execute).to receive(:shell_out_with_systems_locale!).with("apt-get -q update", anything())
+      expect_any_instance_of(Chef::Provider::Execute).to receive(:shell_out!).with("apt-get -q update", anything())
       provider.run_action(:update)
       expect(new_resource).to be_updated_by_last_action
     end
@@ -79,14 +79,14 @@ describe Chef::Provider::AptUpdate do
 
     it "should run if the time stamp is old" do
       expect(File).to receive(:mtime).with("#{stamp_dir}/update-success-stamp").and_return(Time.now - 86_500)
-      expect_any_instance_of(Chef::Provider::Execute).to receive(:shell_out_with_systems_locale!).with("apt-get -q update", anything())
+      expect_any_instance_of(Chef::Provider::Execute).to receive(:shell_out!).with("apt-get -q update", anything())
       provider.run_action(:periodic)
       expect(new_resource).to be_updated_by_last_action
     end
 
     it "should not run if the time stamp is new" do
       expect(File).to receive(:mtime).with("#{stamp_dir}/update-success-stamp").and_return(Time.now)
-      expect_any_instance_of(Chef::Provider::Execute).not_to receive(:shell_out_with_systems_locale!).with("apt-get -q update", anything())
+      expect_any_instance_of(Chef::Provider::Execute).not_to receive(:shell_out!).with("apt-get -q update", anything())
       provider.run_action(:periodic)
       expect(new_resource).to_not be_updated_by_last_action
     end
@@ -98,14 +98,14 @@ describe Chef::Provider::AptUpdate do
 
       it "should run if the time stamp is old" do
         expect(File).to receive(:mtime).with("#{stamp_dir}/update-success-stamp").and_return(Time.now - 500)
-        expect_any_instance_of(Chef::Provider::Execute).to receive(:shell_out_with_systems_locale!).with("apt-get -q update", anything())
+        expect_any_instance_of(Chef::Provider::Execute).to receive(:shell_out!).with("apt-get -q update", anything())
         provider.run_action(:periodic)
         expect(new_resource).to be_updated_by_last_action
       end
 
       it "should not run if the time stamp is new" do
         expect(File).to receive(:mtime).with("#{stamp_dir}/update-success-stamp").and_return(Time.now - 300)
-        expect_any_instance_of(Chef::Provider::Execute).not_to receive(:shell_out_with_systems_locale!).with("apt-get -q update", anything())
+        expect_any_instance_of(Chef::Provider::Execute).not_to receive(:shell_out!).with("apt-get -q update", anything())
         provider.run_action(:periodic)
         expect(new_resource).to_not be_updated_by_last_action
       end
diff --git a/spec/unit/provider/execute_spec.rb b/spec/unit/provider/execute_spec.rb
index 58cb9f33a8..97e4f3d112 100644
--- a/spec/unit/provider/execute_spec.rb
+++ b/spec/unit/provider/execute_spec.rb
@@ -1,6 +1,6 @@
 #
 # Author:: Prajakta Purohit (<prajakta@chef.io>)
-# Copyright:: Copyright 2008-2017, Chef Software Inc.
+# Copyright:: Copyright 2008-2018, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -33,6 +33,7 @@ describe Chef::Provider::Execute do
       timeout:      3600,
       returns:      0,
       log_level:    :info,
+      internal:     false,
       log_tag:      new_resource.to_s,
     }
   end
@@ -76,7 +77,7 @@ describe Chef::Provider::Execute do
 
   describe "#action_run" do
     it "runs shell_out with the default options" do
-      expect(provider).to receive(:shell_out_with_systems_locale!).with(new_resource.name, opts)
+      expect(provider).to receive(:shell_out!).with(new_resource.name, opts)
       expect(provider).to receive(:converge_by).with("execute foo_resource").and_call_original
       expect(Chef::Log).not_to receive(:warn)
       provider.run_action(:run)
@@ -103,7 +104,7 @@ describe Chef::Provider::Execute do
 
     it "if you pass a command attribute, it runs the command" do
       new_resource.command "/usr/argelbargle/bin/oogachacka 12345"
-      expect(provider).to receive(:shell_out_with_systems_locale!).with(new_resource.command, opts)
+      expect(provider).to receive(:shell_out!).with(new_resource.command, opts)
       expect(provider).to receive(:converge_by).with("execute #{new_resource.command}").and_call_original
       expect(Chef::Log).not_to receive(:warn)
       provider.run_action(:run)
@@ -114,7 +115,7 @@ describe Chef::Provider::Execute do
       new_resource.sensitive true
       # Since the resource is sensitive, it should not have :live_stream set
       opts.delete(:live_stream)
-      expect(provider).to receive(:shell_out_with_systems_locale!).with(new_resource.name, opts)
+      expect(provider).to receive(:shell_out!).with(new_resource.name, opts)
       expect(provider).to receive(:converge_by).with("execute sensitive resource").and_call_original
       expect(Chef::Log).not_to receive(:warn)
       provider.run_action(:run)
@@ -124,7 +125,7 @@ describe Chef::Provider::Execute do
     it "should do nothing if the sentinel file exists" do
       new_resource.creates "/foo_resource"
       expect(FileTest).to receive(:exist?).with(new_resource.creates).and_return(true)
-      expect(provider).not_to receive(:shell_out_with_systems_locale!)
+      expect(provider).not_to receive(:shell_out!)
       expect(Chef::Log).not_to receive(:warn)
       provider.run_action(:run)
       expect(new_resource).not_to be_updated
@@ -137,7 +138,7 @@ describe Chef::Provider::Execute do
       end
 
       it "should raise if user specified relative path without cwd for Chef-13" do
-        expect(provider).not_to receive(:shell_out_with_systems_locale!)
+        expect(provider).not_to receive(:shell_out!)
         expect { provider.run_action(:run) }.to raise_error(Chef::Exceptions::Execute)
       end
     end
@@ -148,7 +149,7 @@ describe Chef::Provider::Execute do
       expect(FileTest).not_to receive(:exist?).with(new_resource.creates)
       expect(FileTest).to receive(:exist?).with(File.join("/tmp", new_resource.creates)).and_return(true)
       expect(Chef::Log).not_to receive(:warn)
-      expect(provider).not_to receive(:shell_out_with_systems_locale!)
+      expect(provider).not_to receive(:shell_out!)
 
       provider.run_action(:run)
       expect(new_resource).not_to be_updated
@@ -157,7 +158,7 @@ describe Chef::Provider::Execute do
     it "should not include stdout/stderr in failure exception for sensitive resource" do
       opts.delete(:live_stream)
       new_resource.sensitive true
-      expect(provider).to receive(:shell_out_with_systems_locale!).and_raise(Mixlib::ShellOut::ShellCommandFailed)
+      expect(provider).to receive(:shell_out!).and_raise(Mixlib::ShellOut::ShellCommandFailed)
       expect do
         provider.run_action(:run)
       end.to raise_error(Mixlib::ShellOut::ShellCommandFailed, /suppressed for sensitive resource/)
@@ -166,7 +167,7 @@ describe Chef::Provider::Execute do
     describe "streaming output" do
       it "should not set the live_stream if sensitive is on" do
         new_resource.sensitive true
-        expect(provider).to receive(:shell_out_with_systems_locale!).with(new_resource.name, opts)
+        expect(provider).to receive(:shell_out!).with(new_resource.name, opts)
         expect(provider).to receive(:converge_by).with("execute sensitive resource").and_call_original
         expect(Chef::Log).not_to receive(:warn)
         provider.run_action(:run)
@@ -183,7 +184,7 @@ describe Chef::Provider::Execute do
         it "should set the live_stream if the log level is info or above" do
           nopts = opts
           nopts[:live_stream] = @live_stream
-          expect(provider).to receive(:shell_out_with_systems_locale!).with(new_resource.name, nopts)
+          expect(provider).to receive(:shell_out!).with(new_resource.name, nopts)
           expect(provider).to receive(:converge_by).with("execute foo_resource").and_call_original
           expect(Chef::Log).not_to receive(:warn)
           provider.run_action(:run)
@@ -195,7 +196,7 @@ describe Chef::Provider::Execute do
           new_resource.live_stream true
           nopts = opts
           nopts[:live_stream] = @live_stream
-          expect(provider).to receive(:shell_out_with_systems_locale!).with(new_resource.name, nopts)
+          expect(provider).to receive(:shell_out!).with(new_resource.name, nopts)
           expect(provider).to receive(:converge_by).with("execute foo_resource").and_call_original
           expect(Chef::Log).not_to receive(:warn)
           provider.run_action(:run)
@@ -204,7 +205,7 @@ describe Chef::Provider::Execute do
 
         it "should not set the live_stream if the resource is sensitive" do
           new_resource.sensitive true
-          expect(provider).to receive(:shell_out_with_systems_locale!).with(new_resource.name, opts)
+          expect(provider).to receive(:shell_out!).with(new_resource.name, opts)
           expect(provider).to receive(:converge_by).with("execute sensitive resource").and_call_original
           expect(Chef::Log).not_to receive(:warn)
           provider.run_action(:run)
@@ -217,7 +218,7 @@ describe Chef::Provider::Execute do
           nopts = opts
           nopts[:live_stream] = STDOUT
           allow(STDOUT).to receive(:tty?).and_return(true)
-          expect(provider).to receive(:shell_out_with_systems_locale!).with(new_resource.name, nopts)
+          expect(provider).to receive(:shell_out!).with(new_resource.name, nopts)
           expect(provider).to receive(:converge_by).with("execute foo_resource").and_call_original
           expect(Chef::Log).not_to receive(:warn)
           provider.run_action(:run)
@@ -227,7 +228,7 @@ describe Chef::Provider::Execute do
         it "should not set the live_stream to STDOUT if we are a TTY, not daemonized, but sensitive" do
           new_resource.sensitive true
           allow(STDOUT).to receive(:tty?).and_return(true)
-          expect(provider).to receive(:shell_out_with_systems_locale!).with(new_resource.name, opts)
+          expect(provider).to receive(:shell_out!).with(new_resource.name, opts)
           expect(provider).to receive(:converge_by).with("execute sensitive resource").and_call_original
           expect(Chef::Log).not_to receive(:warn)
           provider.run_action(:run)
@@ -237,7 +238,7 @@ describe Chef::Provider::Execute do
         it "should not set the live_stream to STDOUT if we are a TTY, but daemonized" do
           Chef::Config[:daemon] = true
           allow(STDOUT).to receive(:tty?).and_return(true)
-          expect(provider).to receive(:shell_out_with_systems_locale!).with(new_resource.name, opts)
+          expect(provider).to receive(:shell_out!).with(new_resource.name, opts)
           expect(provider).to receive(:converge_by).with("execute foo_resource").and_call_original
           expect(Chef::Log).not_to receive(:warn)
           provider.run_action(:run)
diff --git a/spec/unit/provider/script_spec.rb b/spec/unit/provider/script_spec.rb
index 951d6546c3..42a1186cc0 100644
--- a/spec/unit/provider/script_spec.rb
+++ b/spec/unit/provider/script_spec.rb
@@ -131,7 +131,7 @@ describe Chef::Provider::Script, "action_run" do
 
     describe "when running the script" do
       let (:default_opts) do
-        { timeout: 3600, returns: 0, log_level: :info, log_tag: "script[run some perl code]" }
+        { timeout: 3600, returns: 0, internal: false, log_level: :info, log_tag: "script[run some perl code]" }
       end
 
       before do
@@ -143,7 +143,7 @@ describe Chef::Provider::Script, "action_run" do
       end
 
       it "should call shell_out! with the command" do
-        expect(provider).to receive(:shell_out_with_systems_locale!).with(provider.command, default_opts).and_return(true)
+        expect(provider).to receive(:shell_out!).with(provider.command, default_opts).and_return(true)
         provider.action_run
       end