Merge pull request #9562 from chef/lcg/after-resource

Add after_resource to pair with current_resource

5 files changed, 64 insertions, 30 deletions

diff --git a/lib/chef/action_collection.rb b/lib/chef/action_collection.rb
index 7b1997cfaf..eb0d3bef58 100644
--- a/lib/chef/action_collection.rb
+++ b/lib/chef/action_collection.rb
@@ -1,5 +1,5 @@
 #
-# Copyright:: Copyright 2018-2019, Chef Software Inc.
+# Copyright:: Copyright 2018-2020, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -24,10 +24,6 @@ class Chef
 
     class ActionRecord
 
-      # XXX: this is buggy since we (ab)use this resource for "after" state and it may be
-      # inaccurate and it may be mutated by the user.  A third after_resource should be added
-      # to new_resource + current_resource to properly implement this.
-      #
       # @return [Chef::Resource] The declared resource state.
       #
       attr_accessor :new_resource
@@ -38,6 +34,10 @@ class Chef
       # implementation, but must be handled), or for unprocessed resources.
       attr_accessor :current_resource
 
+      # @return [Chef::Resource] the after_resource object (after-state).  This can be nil for
+      # non custom-resources or resources that do not implement load_after_resource.
+      attr_accessor :after_resource
+
       # @return [Symbol] # The action that was run (or scheduled to run in the case of "unprocessed" resources).
       attr_accessor :action
 
@@ -161,7 +161,7 @@ class Chef
       pending_updates << ActionRecord.new(new_resource, action, pending_updates.length)
     end
 
-    # Hook called after a resource is loaded.  If load_current_resource fails, this hook will
+    # Hook called after a current resource is loaded.  If load_current_resource fails, this hook will
     # not be called and current_resource will be nil, and the resource_failed hook will be called.
     #
     # (see EventDispatch::Base#)
@@ -172,6 +172,17 @@ class Chef
       current_record.current_resource = current_resource
     end
 
+    # Hook called after an after resource is loaded.  If load_after_resource fails, this hook will
+    # not be called and after_resource will be nil, and the resource_failed hook will be called.
+    #
+    # (see EventDispatch::Base#)
+    #
+    def resource_after_state_loaded(new_resource, action, after_resource)
+      return if consumers.empty?
+
+      current_record.after_resource = after_resource
+    end
+
     # Hook called after an action is determined to be up to date.
     #
     # (see EventDispatch::Base#)
diff --git a/lib/chef/data_collector/run_end_message.rb b/lib/chef/data_collector/run_end_message.rb
index 762058bfdb..cfd4b9d8b3 100644
--- a/lib/chef/data_collector/run_end_message.rb
+++ b/lib/chef/data_collector/run_end_message.rb
@@ -1,5 +1,5 @@
 #
-# Copyright:: Copyright 2012-2019, Chef Software Inc.
+# Copyright:: Copyright 2012-2020, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -106,12 +106,13 @@ class Chef
         def action_record_for_json(action_record)
           new_resource = action_record.new_resource
           current_resource = action_record.current_resource
+          after_resource = action_record.after_resource
 
           hash = {
             "type" => new_resource.resource_name.to_sym,
             "name" => new_resource.name.to_s,
             "id" => safe_resource_identity(new_resource),
-            "after" => safe_state_for_resource_reporter(new_resource),
+            "after" => safe_state_for_resource_reporter(after_resource || new_resource),
             "before" => safe_state_for_resource_reporter(current_resource),
             "duration" => action_record.elapsed_time.nil? ? "" : (action_record.elapsed_time * 1000).to_i.to_s,
             "delta" => new_resource.respond_to?(:diff) && updated_or_failed?(action_record) ? new_resource.diff : "",
@@ -120,6 +121,11 @@ class Chef
             "status" => action_record_status_for_json(action_record),
           }
 
+          # don't use the new_resource for the after_resource if it is skipped or failed
+          if action_record.status == :skipped || action_record.status == :failed || action_record.status == :unprocessed
+            hash["after"] = {}
+          end
+
           if new_resource.cookbook_name
             hash["cookbook_name"]    = new_resource.cookbook_name
             hash["cookbook_version"] = new_resource.cookbook_version.version
diff --git a/lib/chef/event_dispatch/base.rb b/lib/chef/event_dispatch/base.rb
index cb1cc80017..f7b706cb2c 100644
--- a/lib/chef/event_dispatch/base.rb
+++ b/lib/chef/event_dispatch/base.rb
@@ -266,6 +266,9 @@ class Chef
       # Called after #load_current_resource has run.
       def resource_current_state_loaded(resource, action, current_resource); end
 
+      # Called after #load_after_resource has run.
+      def resource_after_state_loaded(resource, action, after_resource); end
+
       # Called when resource current state load is skipped due to the provider
       # not supporting whyrun mode.
       def resource_current_state_load_bypassed(resource, action, current_resource); end
diff --git a/lib/chef/provider.rb b/lib/chef/provider.rb
index 07cdea6dfc..3783bd9d5f 100644
--- a/lib/chef/provider.rb
+++ b/lib/chef/provider.rb
@@ -35,6 +35,7 @@ class Chef
 
     attr_accessor :new_resource
     attr_accessor :current_resource
+    attr_accessor :after_resource
     attr_accessor :run_context
 
     attr_reader :recipe_name
@@ -94,6 +95,7 @@ class Chef
       @new_resource = new_resource
       @action = action
       @current_resource = nil
+      @after_resource = nil
       @run_context = run_context
       @converge_actions = nil
 
@@ -148,6 +150,13 @@ class Chef
 
     def cleanup_after_converge; end
 
+    def load_after_resource
+      # This is a backwards compatible hack, custom resources properly wire up a new after_resource
+      # via load_current_value.  It is acceptible for old style resources that cannot be easily made
+      # into custom resources to override this method and provide a proper after_resource.
+      @after_resource = @new_resource
+    end
+
     # the :nothing action which is available on all resources by default
     def action_nothing
       logger.trace("Doing nothing for #{@new_resource}")
@@ -191,6 +200,9 @@ class Chef
       set_updated_status
 
       cleanup_after_converge
+
+      load_after_resource
+      events.resource_after_state_loaded(@new_resource, @action, @after_resource)
     end
 
     def process_resource_requirements
diff --git a/lib/chef/resource/action_class.rb b/lib/chef/resource/action_class.rb
index 1b4ddd453b..eefa968197 100644
--- a/lib/chef/resource/action_class.rb
+++ b/lib/chef/resource/action_class.rb
@@ -29,42 +29,44 @@ class Chef
         "#{new_resource || "<no resource>"} action #{action ? action.inspect : "<no action>"}"
       end
 
-      #
-      # If load_current_value! is defined on the resource, use that.
-      #
-      def load_current_resource
+      def return_load_current_value
+        resource = nil
         if new_resource.respond_to?(:load_current_value!)
-          # dup the resource and then reset desired-state properties.
-          current_resource = new_resource.dup
+          resource = new_resource.class.new(new_resource.name, new_resource.run_context)
 
-          # We clear desired state in the copy, because it is supposed to be actual state.
-          # We keep identity properties and non-desired-state, which are assumed to be
-          # "control" values like `recurse: true`
-          current_resource.class.properties.each_value do |property|
-            if property.desired_state? && !property.identity? && !property.name_property?
-              property.reset(current_resource)
+          # copy the non-desired state, the identity properties and name property to the new resource
+          # (the desired state values must be loaded by load_current_value)
+          resource.class.properties.each_value do |property|
+            if !property.desired_state? || property.identity? || property.name_property?
+              property.set(resource, new_resource.send(property.name)) if new_resource.class.properties[property.name].is_set?(new_resource)
             end
           end
 
-          # Call the actual load_current_value! method. If it raises
-          # CurrentValueDoesNotExist, set current_resource to `nil`.
+          # we support optionally passing the new_resource as an arg to load_current_value and
+          # load_current_value can raise in order to clear the current_resource to nil
           begin
-            # If the user specifies load_current_value do |desired_resource|, we
-            # pass in the desired resource as well as the current one.
-            if current_resource.method(:load_current_value!).arity > 0
-              current_resource.load_current_value!(new_resource)
+            if resource.method(:load_current_value!).arity > 0
+              resource.load_current_value!(new_resource)
             else
-              current_resource.load_current_value!
+              resource.load_current_value!
             end
           rescue Chef::Exceptions::CurrentValueDoesNotExist
-            current_resource = nil
+            resource = nil
           end
         end
+        resource
+      end
+
+      # build the before state (current_resource)
+      def load_current_resource
+        @current_resource = return_load_current_value
+      end
 
-        @current_resource = current_resource
+      # build the after state (after_resource)
+      def load_after_resource
+        @after_resource = return_load_current_value
       end
 
-      # @todo: remove in Chef-15
       def self.include_resource_dsl?
         true
       end