Merge pull request #9484 from chef/lcg/log-resource-no-notify

Turn off notifications from log resource by default

11 files changed, 78 insertions, 77 deletions

diff --git a/chef-config/lib/chef-config/config.rb b/chef-config/lib/chef-config/config.rb
index 8d6d6e8923..1ee27949bc 100644
--- a/chef-config/lib/chef-config/config.rb
+++ b/chef-config/lib/chef-config/config.rb
@@ -834,7 +834,7 @@ module ChefConfig
 
     # Whether the resource count should be updated for log resource
     # on running chef-client
-    default :count_log_resource_updates, true
+    default :count_log_resource_updates, false
 
     # The selected profile when using credentials.
     default :profile, nil
diff --git a/lib/chef/formatters/doc.rb b/lib/chef/formatters/doc.rb
index 0a4edd2f8b..88f332626c 100644
--- a/lib/chef/formatters/doc.rb
+++ b/lib/chef/formatters/doc.rb
@@ -273,7 +273,7 @@ class Chef
       # Called when a resource has no converge actions, e.g., it was already correct.
       def resource_up_to_date(resource, action)
         @up_to_date_resources += 1
-        puts " (up to date)", stream: resource
+        puts " (up to date)", stream: resource unless resource.suppress_up_to_date_messages?
         unindent
       end
 
diff --git a/lib/chef/provider/log.rb b/lib/chef/provider/log.rb
deleted file mode 100644
index 54e324cc11..0000000000
--- a/lib/chef/provider/log.rb
+++ /dev/null
@@ -1,43 +0,0 @@
-#
-# Author:: Cary Penniman (<cary@rightscale.com>)
-# Copyright:: Copyright 2008-2017, Chef Software Inc.
-# License:: Apache License, Version 2.0
-#
-# Licensed under the Apache License, Version 2.0 (the "License");
-# you may not use this file except in compliance with the License.
-# You may obtain a copy of the License at
-#
-#     http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing, software
-# distributed under the License is distributed on an "AS IS" BASIS,
-# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-# See the License for the specific language governing permissions and
-# limitations under the License.
-#
-
-class Chef
-  class Provider
-    class Log
-      # Chef log provider, allows logging to chef's logs
-      class ChefLog < Chef::Provider
-        provides :log, target_mode: true
-
-        # No concept of a 'current' resource for logs, this is a no-op
-        #
-        # @return [true] Always returns true
-        def load_current_resource
-          true
-        end
-
-        # Write the log to Chef's log
-        #
-        # @return [true] Always returns true
-        action :write do
-          logger.send(new_resource.level, new_resource.message)
-          new_resource.updated_by_last_action(true) if Chef::Config[:count_log_resource_updates]
-        end
-      end
-    end
-  end
-end
diff --git a/lib/chef/providers.rb b/lib/chef/providers.rb
index aa482e9705..9209ff4ca1 100644
--- a/lib/chef/providers.rb
+++ b/lib/chef/providers.rb
@@ -32,7 +32,6 @@ require_relative "provider/http_request"
 require_relative "provider/ifconfig"
 require_relative "provider/launchd"
 require_relative "provider/link"
-require_relative "provider/log"
 require_relative "provider/ohai"
 require_relative "provider/mount"
 require_relative "provider/noop"
diff --git a/lib/chef/resource.rb b/lib/chef/resource.rb
index ad2862c70b..4967a2a84d 100644
--- a/lib/chef/resource.rb
+++ b/lib/chef/resource.rb
@@ -1566,6 +1566,16 @@ class Chef
       # can't/won't support.
       self.class.resource_for_node(name, node).new("name", run_context).provider_for_action(action).class
     end
+
+    # This is used to suppress the "(up to date)" message in the doc formatter
+    # for the log resource (where it is nonsensical).
+    #
+    # This is not exactly a private API, but its doubtful there exist many other sane
+    # use cases for this.
+    #
+    def suppress_up_to_date_messages?
+      false
+    end
   end
 end
 
diff --git a/lib/chef/resource/log.rb b/lib/chef/resource/log.rb
index 32c9b8a931..b32ad7fdef 100644
--- a/lib/chef/resource/log.rb
+++ b/lib/chef/resource/log.rb
@@ -49,6 +49,18 @@ class Chef
 
       allowed_actions :write
       default_action :write
+
+      def suppress_up_to_date_messages?
+        true
+      end
+
+      # Write the log to Chef's log
+      #
+      # @return [true] Always returns true
+      action :write do
+        logger.send(new_resource.level, new_resource.message)
+        new_resource.updated_by_last_action(true) if Chef::Config[:count_log_resource_updates]
+      end
     end
   end
 end
diff --git a/lib/chef/resource/notify_group.rb b/lib/chef/resource/notify_group.rb
index 3930ece967..cbbe917540 100644
--- a/lib/chef/resource/notify_group.rb
+++ b/lib/chef/resource/notify_group.rb
@@ -62,6 +62,13 @@ class Chef
         new_resource.updated_by_last_action(true)
       end
 
+      # This is deliberate.  Users should be sending a single notification to a notify_group resource which then
+      # distributes multiple notifications.  Having a notify_group run by default is unnecessary indirection and
+      # should be replaced by just running the resources in order.  If resources need to be batch run multiple times
+      # per invocation then the resources themselves are not properly declarative idempotent resources, and the user
+      # is attempting to turn Chef into an imperative language using this construct and/or the batch of resources
+      # need to be turned into a custom resource.
+      #
       default_action :nothing
     end
   end
diff --git a/spec/integration/recipes/notifies_spec.rb b/spec/integration/recipes/notifies_spec.rb
index ae534dcad4..b5d1db1a1a 100644
--- a/spec/integration/recipes/notifies_spec.rb
+++ b/spec/integration/recipes/notifies_spec.rb
@@ -15,11 +15,13 @@ describe "notifications" do
           apt_update do
             action :nothing
           end
-          log "foo" do
+          notify_group "foo" do
             notifies :nothing, 'apt_update', :delayed
+            action :run
           end
-          log "bar" do
+          notify_group "bar" do
             notifies :nothing, 'apt_update[]', :delayed
+            action :run
           end
         EOM
       end
@@ -34,7 +36,7 @@ describe "notifications" do
 
       result = shell_out("#{chef_client} -c \"#{path_to("config/client.rb")}\" --no-color -F doc -o 'x::default'", cwd: chef_dir)
       # our delayed notification should run at the end of the parent run_context after the baz resource
-      expect(result.stdout).to match(/\* apt_update\[\] action nothing \(skipped due to action :nothing\)\s+\* log\[foo\] action write\s+\* log\[bar\] action write\s+\* apt_update\[\] action nothing \(skipped due to action :nothing\)/)
+      expect(result.stdout).to match(/\* apt_update\[\] action nothing \(skipped due to action :nothing\)\s+\* notify_group\[foo\] action run\s+\* notify_group\[bar\] action run\s+\* apt_update\[\] action nothing \(skipped due to action :nothing\)/)
       result.error!
     end
   end
@@ -49,8 +51,9 @@ describe "notifications" do
           resource_name :notifying_test
 
           action :run do
-            log "bar" do
+            notify_group "bar" do
               notifies :write, 'log[foo]', :delayed
+              action :run
             end
           end
         EOM
@@ -75,7 +78,7 @@ describe "notifications" do
 
       result = shell_out("#{chef_client} -c \"#{path_to("config/client.rb")}\" --no-color -F doc -o 'x::default'", cwd: chef_dir)
       # our delayed notification should run at the end of the parent run_context after the baz resource
-      expect(result.stdout).to match(/\* log\[bar\] action write\s+\* log\[baz\] action write\s+\* log\[foo\] action write/)
+      expect(result.stdout).to match(/\* notify_group\[bar\] action run\s+\* log\[baz\] action write\s+\* log\[foo\] action write/)
       result.error!
     end
   end
@@ -90,8 +93,9 @@ describe "notifications" do
           resource_name :notifying_test
 
           action :run do
-            log "bar" do
+            notify_group "bar" do
               notifies :write, 'log[foo]', :delayed
+              action :run
             end
           end
         EOM
@@ -101,8 +105,9 @@ describe "notifications" do
             action :nothing
           end
           notifying_test "whatever"
-          log "baz" do
+          notify_group "baz" do
             notifies :write, 'log[foo]', :delayed
+            action :run
           end
         EOM
 
@@ -118,7 +123,7 @@ describe "notifications" do
 
       result = shell_out("#{chef_client} -c \"#{path_to("config/client.rb")}\" --no-color -F doc -o 'x::default'", cwd: chef_dir)
       # our delayed notification should run at the end of the parent run_context after the baz resource
-      expect(result.stdout).to match(/\* log\[bar\] action write\s+\* log\[baz\] action write\s+\* log\[foo\] action write/)
+      expect(result.stdout).to match(/\* notify_group\[bar\] action run\s+\* notify_group\[baz\] action run\s+\* log\[foo\] action write/)
       # and only run once
       expect(result.stdout).not_to match(/\* log\[foo\] action write.*\* log\[foo\] action write/)
       result.error!
@@ -135,8 +140,9 @@ describe "notifications" do
           resource_name :notifying_test
 
           action :run do
-            log "bar" do
+            notify_group "bar" do
               notifies :write, 'log[foo]', :delayed
+              action :run
             end
           end
         EOM
@@ -145,9 +151,10 @@ describe "notifications" do
           log "foo" do
             action :nothing
           end
-          log "quux" do
+          notify_group "quux" do
             notifies :write, 'log[foo]', :delayed
             notifies :write, 'log[baz]', :delayed
+            action :run
           end
           notifying_test "whatever"
           log "baz"
@@ -165,7 +172,7 @@ describe "notifications" do
 
       result = shell_out("#{chef_client} -c \"#{path_to("config/client.rb")}\" --no-color -F doc -o 'x::default'", cwd: chef_dir)
       # the delayed notification from the sub-resource is de-duplicated by the notification already in the parent run_context
-      expect(result.stdout).to match(/\* log\[quux\] action write\s+\* notifying_test\[whatever\] action run\s+\* log\[bar\] action write\s+\* log\[baz\] action write\s+\* log\[foo\] action write\s+\* log\[baz\] action write/)
+      expect(result.stdout).to match(/\* notify_group\[quux\] action run\s+\* notifying_test\[whatever\] action run\s+\* notify_group\[bar\] action run\s+\* log\[baz\] action write\s+\* log\[foo\] action write\s+\* log\[baz\] action write/)
       # and only run once
       expect(result.stdout).not_to match(/\* log\[foo\] action write.*\* log\[foo\] action write/)
       result.error!
@@ -179,11 +186,13 @@ describe "notifications" do
           log "foo" do
             action :nothing
           end
-          log "bar" do
+          notify_group "bar" do
             notifies :write, 'log[foo]', :delayed
+            action :run
           end
-          log "baz" do
+          notify_group "baz" do
             notifies :write, 'log[foo]', :delayed
+            action :run
           end
         EOM
 
@@ -199,7 +208,7 @@ describe "notifications" do
 
       result = shell_out("#{chef_client} -c \"#{path_to("config/client.rb")}\" --no-color -F doc -o 'x::default'", cwd: chef_dir)
       # the delayed notification from the sub-resource is de-duplicated by the notification already in the parent run_context
-      expect(result.stdout).to match(/\* log\[bar\] action write\s+\* log\[baz\] action write\s+\* log\[foo\] action write/)
+      expect(result.stdout).to match(/\* notify_group\[bar\] action run\s+\* notify_group\[baz\] action run\s+\* log\[foo\] action write/)
       # and only run once
       expect(result.stdout).not_to match(/\* log\[foo\] action write.*\* log\[foo\] action write/)
       result.error!
@@ -216,8 +225,9 @@ describe "notifications" do
           resource_name :notifying_test
 
           action :run do
-            log "bar" do
+            notify_group "bar" do
               notifies :write, 'log[foo]', :immediately
+              action :run
             end
           end
         EOM
@@ -241,7 +251,7 @@ describe "notifications" do
       EOM
 
       result = shell_out("#{chef_client} -c \"#{path_to("config/client.rb")}\" --no-color -F doc -o 'x::default'", cwd: chef_dir)
-      expect(result.stdout).to match(/\* log\[bar\] action write\s+\* log\[foo\] action write\s+\* log\[baz\] action write/)
+      expect(result.stdout).to match(/\* notify_group\[bar\] action run\s+\* log\[foo\] action write\s+\* log\[baz\] action write/)
       result.error!
     end
   end
@@ -256,8 +266,9 @@ describe "notifications" do
           resource_name :notifying_test
 
           action :run do
-            log "bar" do
+            notify_group "bar" do
               notifies :write, resources(log: "foo"), :immediately
+              action :run
             end
           end
         EOM
@@ -281,7 +292,7 @@ describe "notifications" do
       EOM
 
       result = shell_out("#{chef_client} -c \"#{path_to("config/client.rb")}\" --no-color -F doc -o 'x::default'", cwd: chef_dir)
-      expect(result.stdout).to match(/\* log\[bar\] action write\s+\* log\[foo\] action write\s+\* log\[baz\] action write/)
+      expect(result.stdout).to match(/\* notify_group\[bar\] action run\s+\* log\[foo\] action write\s+\* log\[baz\] action write/)
       result.error!
     end
   end
@@ -296,8 +307,9 @@ describe "notifications" do
           resource_name :notifying_test
 
           action :run do
-            log "bar" do
+            notify_group "bar" do
               notifies :write, "log[foo]"
+              action :run
             end
           end
         EOM
@@ -371,8 +383,9 @@ describe "notifications" do
             action :nothing
           end
 
-          log "doit" do
+          notify_group "doit" do
             notifies :write, "log[a, b]"
+            action :run
           end
         EOM
       end
diff --git a/spec/integration/recipes/notifying_block_spec.rb b/spec/integration/recipes/notifying_block_spec.rb
index 465eca97ed..20bdde0c26 100644
--- a/spec/integration/recipes/notifying_block_spec.rb
+++ b/spec/integration/recipes/notifying_block_spec.rb
@@ -1,6 +1,6 @@
 #
 # Author:: John Keiser (<jkeiser@chef.io>)
-# Copyright:: Copyright 2013-2019, Chef Software Inc.
+# Copyright:: Copyright 2013-2020, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -33,11 +33,13 @@ describe "notifying_block" do
             log "gamma" do
               action :nothing
             end
-            log "alpha" do
+            notify_group "alpha" do
               notifies :write, "log[gamma]", :delayed
+              action :run
             end
-            log "beta" do
+            notify_group "beta" do
               notifies :write, "log[gamma]", :delayed
+              action :run
             end
           end
           log "delta"
@@ -56,7 +58,7 @@ describe "notifying_block" do
     #  3. delayed notifications (to resources inside the subcontext) are run at the end of the subcontext
     it "should run alpha, beta, gamma, and delta in that order" do
       result = shell_out("#{chef_client} -c \"#{path_to("config/client.rb")}\" --no-color -F doc -o 'x::default'", cwd: chef_dir)
-      expect(result.stdout).to match(/\* log\[alpha\] action write\s+\* log\[beta\] action write\s+\* log\[gamma\] action write\s+Converging 1 resources\s+\* log\[delta\] action write/)
+      expect(result.stdout).to match(/\* notify_group\[alpha\] action run\s+\* notify_group\[beta\] action run\s+\* log\[gamma\] action write\s+Converging 1 resources\s+\* log\[delta\] action write/)
       result.error!
     end
   end
@@ -71,8 +73,9 @@ describe "notifying_block" do
 
           action :run do
             notifying_block do
-              log "foo" do
+              notify_group "foo" do
                 notifies :write, 'log[bar]', :delayed
+                action :run
               end
             end
           end
@@ -104,7 +107,7 @@ describe "notifying_block" do
     #  2. delayed notifications from a subcontext inside a resource will notify resources in their outer run_context
     it "should run foo, quux, bar, and baz in that order" do
       result = shell_out("#{chef_client} -c \"#{path_to("config/client.rb")}\" --no-color -F doc -o 'x::default'", cwd: chef_dir)
-      expect(result.stdout).to match(/\* log\[foo\] action write\s+\* log\[quux\] action write\s+\* log\[bar\] action write\s+\* log\[baz\] action write/)
+      expect(result.stdout).to match(/\* notify_group\[foo\] action run\s+\* log\[quux\] action write\s+\* log\[bar\] action write\s+\* log\[baz\] action write/)
       result.error!
     end
   end
diff --git a/spec/unit/provider/log_spec.rb b/spec/unit/provider/log_spec.rb
index deb2024640..f72cf3b558 100644
--- a/spec/unit/provider/log_spec.rb
+++ b/spec/unit/provider/log_spec.rb
@@ -1,6 +1,6 @@
 #
 # Author:: Cary Penniman (<cary@rightscale.com>)
-# Copyright:: Copyright 2008-2016, Chef Software Inc.
+# Copyright:: Copyright 2008-2020, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -18,7 +18,7 @@
 
 require "spec_helper"
 
-describe Chef::Provider::Log::ChefLog do
+describe Chef::Resource::Log do
 
   let(:log_str) { "this is my test string to log" }
 
@@ -28,9 +28,9 @@ describe Chef::Provider::Log::ChefLog do
 
   let(:run_context) { Chef::RunContext.new(node, {}, events) }
 
-  let(:new_resource) { Chef::Resource::Log.new(log_str) }
+  let(:new_resource) { Chef::Resource::Log.new(log_str, run_context) }
 
-  let(:provider) { Chef::Provider::Log::ChefLog.new(new_resource, run_context) }
+  let(:provider) { new_resource.provider_for_action(:run) }
 
   let(:logger) { double("Mixlib::Log::Child").as_null_object }
   before do
diff --git a/spec/unit/provider_resolver_spec.rb b/spec/unit/provider_resolver_spec.rb
index 79b43ccce2..1fd494c500 100644
--- a/spec/unit/provider_resolver_spec.rb
+++ b/spec/unit/provider_resolver_spec.rb
@@ -560,7 +560,7 @@ describe Chef::ProviderResolver do
         ips_package: [ Chef::Resource::IpsPackage, Chef::Provider::Package::Ips ],
         link: [ Chef::Resource::Link, Chef::Provider::Link ],
         linux_user: [ Chef::Resource::User::LinuxUser, Chef::Provider::User::Linux ],
-        log: [ Chef::Resource::Log, Chef::Provider::Log::ChefLog ],
+        log: [ Chef::Resource::Log ],
         macports_package: [ Chef::Resource::MacportsPackage, Chef::Provider::Package::Macports ],
         mdadm: [ Chef::Resource::Mdadm ],
         mount: [ Chef::Resource::Mount, Chef::Provider::Mount::Mount ],