Enable progress output to be configuredtm/remote_file_download_progress

Add tests around progress output and tidy up

9 files changed, 66 insertions, 26 deletions

diff --git a/chef-config/lib/chef-config/config.rb b/chef-config/lib/chef-config/config.rb
index 8161cd9ea7..1455c56082 100644
--- a/chef-config/lib/chef-config/config.rb
+++ b/chef-config/lib/chef-config/config.rb
@@ -299,6 +299,12 @@ module ChefConfig
     # Using 'stream_execute_output' will have Chef always stream the execute output
     default :stream_execute_output, false
 
+    # Using `show_download_progress` will display the overall progress
+    # of a remote file download
+    default :show_download_progress, false
+    # How often to update the progress meter, in percent
+    default :download_progress_interval, 10
+
     default :http_retry_count, 5
     default :http_retry_delay, 5
     default :interval, nil
diff --git a/chef-config/spec/unit/config_spec.rb b/chef-config/spec/unit/config_spec.rb
index dbde3d3160..797fe3a233 100644
--- a/chef-config/spec/unit/config_spec.rb
+++ b/chef-config/spec/unit/config_spec.rb
@@ -274,6 +274,14 @@ RSpec.describe ChefConfig::Config do
           expect(ChefConfig::Config[:stream_execute_output]).to eq(false)
         end
 
+        it "ChefConfig::Config[:show_download_progress] defaults to false" do
+          expect(ChefConfig::Config[:show_download_progress]).to eq(false)
+        end
+
+        it "ChefConfig::Config[:download_progress_interval] defaults to every 10%" do
+          expect(ChefConfig::Config[:download_progress_interval]).to eq(10)
+        end
+
         it "ChefConfig::Config[:file_backup_path] defaults to /var/chef/backup" do
           allow(ChefConfig::Config).to receive(:cache_path).and_return(primary_cache_path)
           backup_path = is_windows ? "#{primary_cache_path}\\backup" : "#{primary_cache_path}/backup"
diff --git a/lib/chef/event_dispatch/base.rb b/lib/chef/event_dispatch/base.rb
index 65cb519dc7..e6bbbc929f 100644
--- a/lib/chef/event_dispatch/base.rb
+++ b/lib/chef/event_dispatch/base.rb
@@ -297,12 +297,6 @@ class Chef
       #    - resource_completed
       #
 
-      # Called when a progress notification should be sent to the user to
-      # indicate the overall progress of a long running operation, such as
-      # a large file download.
-      def resource_action_progress(resource, current, total, interval)
-      end
-
       # Called before action is executed on a resource.
       def resource_action_start(resource, action, notification_type = nil, notifier = nil)
       end
@@ -330,6 +324,12 @@ class Chef
       def resource_update_applied(resource, action, update)
       end
 
+      # Called when a progress notification should be sent to the user to
+      # indicate the overall progress of a long running operation, such as
+      # a large file download.
+      def resource_update_progress(resource, current, total, interval)
+      end
+
       # Called when a resource fails, but will retry.
       def resource_failed_retriable(resource, action, retry_count, exception)
       end
diff --git a/lib/chef/formatters/doc.rb b/lib/chef/formatters/doc.rb
index 31732066a8..bad22370ae 100644
--- a/lib/chef/formatters/doc.rb
+++ b/lib/chef/formatters/doc.rb
@@ -259,7 +259,7 @@ class Chef
         indent
       end
 
-      def resource_action_progress(resource, current, total, interval)
+      def resource_update_progress(resource, current, total, interval)
         @progress[resource] ||= 0
 
         percent_complete = (current.to_f / total.to_f * 100).to_i
@@ -269,7 +269,7 @@ class Chef
           @progress[resource] = percent_complete
 
           if percent_complete % interval == 0
-            start_line " - Progress: #{percent_complete}/100 %", :green
+            start_line " - Progress: #{percent_complete}%", :green
           end
         end
       end
diff --git a/lib/chef/http.rb b/lib/chef/http.rb
index c90f799cd9..4a3b0bc85e 100644
--- a/lib/chef/http.rb
+++ b/lib/chef/http.rb
@@ -74,8 +74,6 @@ class Chef
     attr_reader :redirect_limit
 
     attr_reader :options
-    attr_reader :show_progress
-    attr_reader :progress_interval
 
     attr_reader :middlewares
 
@@ -156,8 +154,7 @@ class Chef
       raise
     end
 
-
-    def streaming_request_with_progress(path, headers={}, &progress_block)
+    def streaming_request_with_progress(path, headers = {}, &progress_block)
       url = create_url(path)
       response, rest_request, return_value = nil, nil, nil
       tempfile = nil
@@ -421,7 +418,7 @@ class Chef
     end
 
     def stream_to_tempfile(url, response, &progress_block)
-      content_length = response['Content-Length']
+      content_length = response["Content-Length"]
       tf = Tempfile.open("chef-rest")
       if Chef::Platform.windows?
         tf.binmode # required for binary files on Windows platforms
@@ -458,4 +455,3 @@ class Chef
 
   end
 end
-
diff --git a/lib/chef/provider/remote_file/http.rb b/lib/chef/provider/remote_file/http.rb
index 3b97d116aa..d4f2446e27 100644
--- a/lib/chef/provider/remote_file/http.rb
+++ b/lib/chef/provider/remote_file/http.rb
@@ -63,7 +63,7 @@ class Chef
           http = Chef::HTTP::Simple.new(uri, http_client_opts)
           if want_progress?
             tempfile = http.streaming_request_with_progress(uri, headers) do |size, total|
-              events.resource_action_progress(new_resource, size, total, new_resource.progress_interval)
+              events.resource_update_progress(new_resource, size, total, progress_interval)
             end
           else
             tempfile = http.streaming_request(uri, headers)
@@ -89,7 +89,11 @@ class Chef
         end
 
         def want_progress?
-          new_resource.show_progress
+          events.formatter? && (Chef::Config[:show_download_progress] || !!new_resource.show_progress)
+        end
+
+        def progress_interval
+          Chef::Config[:download_progress_interval]
         end
 
         def want_mtime_cache_control?
diff --git a/lib/chef/resource/remote_file.rb b/lib/chef/resource/remote_file.rb
index b615297c24..321c3fcf4d 100644
--- a/lib/chef/resource/remote_file.rb
+++ b/lib/chef/resource/remote_file.rb
@@ -122,7 +122,7 @@ class Chef
         )
       end
 
-      def show_progress(args=nil)
+      def show_progress(args = nil)
         set_or_return(
           :show_progress,
           args,
@@ -131,15 +131,6 @@ class Chef
         )
       end
 
-      def progress_interval(args=nil)
-        set_or_return(
-          :progress_interval,
-          args,
-          :default => 10,
-          :equal_to => [1, 5, 10, 20, 25]
-        )
-      end
-
       private
 
       include Chef::Mixin::Uris
diff --git a/spec/unit/formatters/doc_spec.rb b/spec/unit/formatters/doc_spec.rb
index e9bff0759d..b8eccc1bc9 100644
--- a/spec/unit/formatters/doc_spec.rb
+++ b/spec/unit/formatters/doc_spec.rb
@@ -75,4 +75,18 @@ describe Chef::Formatters::Base do
     expect(formatter.elapsed_time).to eql(36610.0)
     expect(formatter.pretty_elapsed_time).to include("10 hours 10 minutes 10 seconds")
   end
+
+  it "shows the percentage completion of an action" do
+    res = Chef::Resource::RemoteFile.new("canteloupe")
+    formatter.resource_update_progress(res, 35, 50, 10)
+    expect(out.string).to include(" - Progress: 70%")
+  end
+
+  it "updates the percentage completion of an action" do
+    res = Chef::Resource::RemoteFile.new("canteloupe")
+    formatter.resource_update_progress(res, 70, 100, 10)
+    expect(out.string).to include(" - Progress: 70%")
+    formatter.resource_update_progress(res, 80, 100, 10)
+    expect(out.string).to include(" - Progress: 80%")
+  end
 end
diff --git a/spec/unit/provider/remote_file/http_spec.rb b/spec/unit/provider/remote_file/http_spec.rb
index 60ecd45dbb..f58a3d3c14 100644
--- a/spec/unit/provider/remote_file/http_spec.rb
+++ b/spec/unit/provider/remote_file/http_spec.rb
@@ -163,6 +163,12 @@ describe Chef::Provider::RemoteFile::HTTP do
 
     let(:last_response) { {} }
 
+    let(:event_dispatcher) do
+      event_dispatcher = double(Chef::EventDispatch::Dispatcher)
+      allow(event_dispatcher).to receive(:formatter?).and_return(false)
+      event_dispatcher
+    end
+
     let(:rest) do
       rest = double(Chef::HTTP::Simple)
       allow(rest).to receive(:streaming_request).and_return(tempfile)
@@ -173,6 +179,7 @@ describe Chef::Provider::RemoteFile::HTTP do
     before do
       new_resource.headers({})
       new_resource.use_last_modified(false)
+      allow(new_resource).to receive(:events).and_return(event_dispatcher)
       expect(Chef::Provider::RemoteFile::CacheControlData).to receive(:load_and_validate).with(uri, current_resource_checksum).and_return(cache_control_data)
 
       expect(Chef::HTTP::Simple).to receive(:new).with(*expected_http_args).and_return(rest)
@@ -205,6 +212,20 @@ describe Chef::Provider::RemoteFile::HTTP do
         expect(cache_control_data.checksum).to eq(fetched_content_checksum)
       end
 
+      context "with progress reports" do
+        before do
+          Chef::Config[:show_download_progress] = true
+        end
+
+        it "should yield its progress" do
+          allow(rest).to receive(:streaming_request_with_progress).and_yield(50, 100).and_yield(70, 100).and_return(tempfile)
+          expect(event_dispatcher).to receive(:formatter?).and_return(true)
+          expect(event_dispatcher).to receive(:resource_update_progress).with(new_resource, 50, 100, 10).ordered
+          expect(event_dispatcher).to receive(:resource_update_progress).with(new_resource, 70, 100, 10).ordered
+          fetcher.fetch
+        end
+      end
+
       context "and the response does not contain an etag" do
         let(:last_response) { { "etag" => nil } }
         it "does not include an etag in the result" do