respond to review comments

the logic is now:
if the resource is not sensitive, and if it's explicitly requested to be
streamed or if the log level is info or debug, then we'll consider
streaming it.
If we're configured to send the output to the events stream, we'll do
so.
Otherwise, if we're not daemonized and have a TTY, we'll go to STDOUT

6 files changed, 94 insertions, 71 deletions

diff --git a/chef-config/lib/chef-config/config.rb b/chef-config/lib/chef-config/config.rb
index 6a028e9564..4e9355192a 100644
--- a/chef-config/lib/chef-config/config.rb
+++ b/chef-config/lib/chef-config/config.rb
@@ -271,8 +271,8 @@ module ChefConfig
     # Using `force_logger` causes chef to default to logger output when STDOUT is a tty
     default :force_logger, false
 
-    # Using 'always_stream_output' will have Chef always stream the execute output
-    default :always_stream_output, false
+    # Using 'stream_execute_output' will have Chef always stream the execute output
+    default :stream_execute_output, false
 
     default :http_retry_count, 5
     default :http_retry_delay, 5
diff --git a/chef-config/spec/unit/config_spec.rb b/chef-config/spec/unit/config_spec.rb
index fc8528ad46..8e9a499a1a 100644
--- a/chef-config/spec/unit/config_spec.rb
+++ b/chef-config/spec/unit/config_spec.rb
@@ -264,8 +264,8 @@ RSpec.describe ChefConfig::Config do
           end
         end
 
-        it "ChefConfig::Config[:always_stream_output] defaults to false" do
-          expect(ChefConfig::Config[:always_stream_output]).to eq(false)
+        it "ChefConfig::Config[:stream_execute_output] defaults to false" do
+          expect(ChefConfig::Config[:stream_execute_output]).to eq(false)
         end
 
         it "ChefConfig::Config[:file_backup_path] defaults to /var/chef/backup" do
diff --git a/lib/chef/provider/execute.rb b/lib/chef/provider/execute.rb
index 494267828f..16de268258 100644
--- a/lib/chef/provider/execute.rb
+++ b/lib/chef/provider/execute.rb
@@ -78,12 +78,16 @@ class Chef
         !!new_resource.sensitive
       end
 
+      def live_stream?
+        !!new_resource.live_stream
+      end
+
       def stream_to_formatter?
-        Chef::Config[:always_stream_execute] || run_context.events.formatter?
+        Chef::Config[:stream_execute_output] && run_context.events.formatter?
       end
 
       def stream_to_stdout?
-        STDOUT.tty? && !Chef::Config[:daemon] && Chef::Log.info?
+        STDOUT.tty? && !Chef::Config[:daemon]
       end
 
       def opts
@@ -97,7 +101,7 @@ class Chef
         opts[:umask]       = umask if umask
         opts[:log_level]   = :info
         opts[:log_tag]     = new_resource.to_s
-        unless sensitive?
+        if (Chef::Log.info? || live_stream?) && !sensitive?
           if stream_to_formatter?
             opts[:live_stream] = Chef::EventDispatch::EventsOutputStream.new(run_context.events, :name => :execute)
           elsif stream_to_stdout?
diff --git a/lib/chef/resource/execute.rb b/lib/chef/resource/execute.rb
index 11c4ae045c..4f92a7ad35 100644
--- a/lib/chef/resource/execute.rb
+++ b/lib/chef/resource/execute.rb
@@ -49,6 +49,7 @@ class Chef
         @umask = nil
         @default_guard_interpreter = :execute
         @is_guard_interpreter = false
+        @live_stream = false
       end
 
       def umask(arg=nil)
@@ -101,6 +102,13 @@ class Chef
         )
       end
 
+      def live_stream(arg=nil)
+        set_or_return(
+          :live_stream,
+          arg,
+          :kind_of => [ TrueClass, FalseClass ])
+      end
+
       def path(arg=nil)
         Chef::Log.warn "The 'path' attribute of 'execute' is not used by any provider in Chef 11 or Chef 12. Use 'environment' attribute to configure 'PATH'. This attribute will be removed in Chef 13."
 
diff --git a/spec/support/shared/unit/execute_resource.rb b/spec/support/shared/unit/execute_resource.rb
index e969a2ebd5..3a88ff8890 100644
--- a/spec/support/shared/unit/execute_resource.rb
+++ b/spec/support/shared/unit/execute_resource.rb
@@ -111,6 +111,11 @@ shared_examples_for "an execute resource" do
     expect(@resource.creates).to eql("something")
   end
 
+  it "should accept a boolean for live streaming" do
+    @resource.live_stream true
+    expect(@resource.live_stream).to be true
+  end
+
   describe "when it has cwd, environment, group, path, return value, and a user" do
     before do
       @resource.command("grep")
diff --git a/spec/unit/provider/execute_spec.rb b/spec/unit/provider/execute_spec.rb
index 69fd7ae655..3af35a17ca 100644
--- a/spec/unit/provider/execute_spec.rb
+++ b/spec/unit/provider/execute_spec.rb
@@ -155,41 +155,8 @@ describe Chef::Provider::Execute do
       end.to raise_error(Mixlib::ShellOut::ShellCommandFailed, /suppressed for sensitive resource/)
     end
 
-    it "should set the live_stream if Chef::Config[:always_stream_execute] is set" do
-      Chef::Config[:always_stream_execute] = true
-      nopts = opts
-      nopts[:live_stream] = @live_stream
-      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)
-      expect(new_resource).to be_updated
-    end
-
-    it "should not set the live_stream if Chef::Config[:always_stream_execute] is set but sensitive is on" do
-      Chef::Config[:always_stream_execute] = true
-      new_resource.sensitive true
-      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)
-      expect(new_resource).to be_updated
-    end
-
-    describe "with an output formatter listening" do
-      let(:events) { d = Chef::EventDispatch::Dispatcher.new; d.register(Chef::Formatters::Doc.new(StringIO.new, StringIO.new)); d }
-
-      it "should set the live_stream" do
-        nopts = opts
-        nopts[:live_stream] = @live_stream
-        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)
-        expect(new_resource).to be_updated
-      end
-
-      it "should not set the live_stream if the resource is sensitive" 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(new_resource.name, opts)
         expect(provider).to receive(:converge_by).with("execute sensitive resource").and_call_original
@@ -197,40 +164,79 @@ describe Chef::Provider::Execute do
         provider.run_action(:run)
         expect(new_resource).to be_updated
       end
-    end
 
-    describe "with only logging enabled" do
-      it "should set the live_stream to STDOUT if we are a TTY, not daemonized, not sensitive, and info is enabled" do
-        nopts = opts
-        nopts[:live_stream] = STDOUT
-        allow(STDOUT).to receive(:tty?).and_return(true)
-        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)
-        expect(new_resource).to be_updated
+      describe "with an output formatter listening" do
+        let(:events) { d = Chef::EventDispatch::Dispatcher.new; d.register(Chef::Formatters::Doc.new(StringIO.new, StringIO.new)); d }
+
+        before do
+          Chef::Config[:stream_execute_output] = true
+        end
+
+        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(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)
+          expect(new_resource).to be_updated
+        end
+
+        it "should set the live_stream if the resource requests live streaming" do
+          Chef::Log.level = :warn
+          new_resource.live_stream true
+          nopts = opts
+          nopts[:live_stream] = @live_stream
+          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)
+          expect(new_resource).to be_updated
+        end
+
+        it "should not set the live_stream if the resource is sensitive" do
+          new_resource.sensitive true
+          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)
+          expect(new_resource).to be_updated
+        end
       end
 
-      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(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)
-        expect(new_resource).to be_updated
-      end
+      describe "with only logging enabled" do
+        it "should set the live_stream to STDOUT if we are a TTY, not daemonized, not sensitive, and info is enabled" do
+          nopts = opts
+          nopts[:live_stream] = STDOUT
+          allow(STDOUT).to receive(:tty?).and_return(true)
+          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)
+          expect(new_resource).to be_updated
+        end
+
+        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(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)
+          expect(new_resource).to be_updated
+        end
+
+        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(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)
+          expect(new_resource).to be_updated
+        end
 
-      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(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)
-        expect(new_resource).to be_updated
       end
-
     end
 
   end