Skip 3694 warnings on trivial resource cloning

Turns the 3694 warning into a debug message if the prior resource is
identical to the current resource.  Suggestion for opscode/chef-rfc#76

This also moves resource building out to its own class.

3 files changed, 152 insertions, 47 deletions

diff --git a/lib/chef/dsl/recipe.rb b/lib/chef/dsl/recipe.rb
index d70266c14c..edd7e9aed1 100644
--- a/lib/chef/dsl/recipe.rb
+++ b/lib/chef/dsl/recipe.rb
@@ -19,6 +19,7 @@
 
 require 'chef/mixin/convert_to_class_name'
 require 'chef/exceptions'
+require 'chef/resource_builder'
 
 class Chef
   module DSL
@@ -92,37 +93,16 @@ class Chef
       def build_resource(type, name, created_at=nil, &resource_attrs_block)
         created_at ||= caller[0]
 
-        # Checks the new platform => short_name => resource mapping initially
-        # then fall back to the older approach (Chef::Resource.const_get) for
-        # backward compatibility
-        resource_class = resource_class_for(type)
-
-        raise ArgumentError, "You must supply a name when declaring a #{type} resource" if name.nil?
-
-        resource = resource_class.new(name, run_context)
-        resource.source_line = created_at
-        resource.declared_type = type
-        # If we have a resource like this one, we want to steal its state
-        # This behavior is very counter-intuitive and should be removed.
-        # See CHEF-3694, https://tickets.opscode.com/browse/CHEF-3694
-        # Moved to this location to resolve CHEF-5052, https://tickets.opscode.com/browse/CHEF-5052
-        resource.load_prior_resource(type, name)
-        resource.cookbook_name = cookbook_name
-        resource.recipe_name = recipe_name
-        # Determine whether this resource is being created in the context of an enclosing Provider
-        resource.enclosing_provider = self.is_a?(Chef::Provider) ? self : nil
-
-        # XXX: This is very crufty, but it's required for resource definitions
-        # to work properly :(
-        resource.params = @params
-
-        # Evaluate resource attribute DSL
-        resource.instance_eval(&resource_attrs_block) if block_given?
-
-        # Run optional resource hook
-        resource.after_created
-
-        resource
+        Chef::ResourceBuilder.new(
+          type:                type,
+          name:                name,
+          created_at:          created_at,
+          params:              @params,
+          run_context:         run_context,
+          cookbook_name:       cookbook_name,
+          recipe_name:         recipe_name,
+          enclosing_provider:  self.is_a?(Chef::Provider) ? self :  nil
+        ).build(&resource_attrs_block)
       end
 
       def resource_class_for(snake_case_name)
diff --git a/lib/chef/resource.rb b/lib/chef/resource.rb
index 17f109242f..bf49cd9d26 100644
--- a/lib/chef/resource.rb
+++ b/lib/chef/resource.rb
@@ -213,23 +213,11 @@ class Chef
       end
     end
 
-    def load_prior_resource(resource_type, instance_name)
-      begin
-        key = "#{resource_type}[#{instance_name}]"
-        prior_resource = run_context.resource_collection.lookup(key)
-        # if we get here, there is a prior resource (otherwise we'd have jumped
-        # to the rescue clause).
-        Chef::Log.warn("Cloning resource attributes for #{key} from prior resource (CHEF-3694)")
-        Chef::Log.warn("Previous #{prior_resource}: #{prior_resource.source_line}") if prior_resource.source_line
-        Chef::Log.warn("Current  #{self}: #{self.source_line}") if self.source_line
-        prior_resource.instance_variables.each do |iv|
-          unless iv.to_sym == :@source_line || iv.to_sym == :@action || iv.to_sym == :@not_if || iv.to_sym == :@only_if
-            self.instance_variable_set(iv, prior_resource.instance_variable_get(iv))
-          end
+    def load_from(resource)
+      resource.instance_variables.each do |iv|
+        unless iv == :@source_line || iv == :@action || iv == :@not_if || iv == :@only_if
+          self.instance_variable_set(iv, resource.instance_variable_get(iv))
         end
-        true
-      rescue Chef::Exceptions::ResourceNotFound
-        true
       end
     end
 
diff --git a/lib/chef/resource_builder.rb b/lib/chef/resource_builder.rb
new file mode 100644
index 0000000000..f700da53c9
--- /dev/null
+++ b/lib/chef/resource_builder.rb
@@ -0,0 +1,137 @@
+#
+# Author:: Lamont Granquist (<lamont@chef.io>)
+# Copyright:: Copyright (c) 2015 Opscode, 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.
+#
+
+# NOTE: this was extracted from the Recipe DSL mixin, relevant specs are in spec/unit/recipe_spec.rb
+
+class Chef
+  class ResourceBuilder
+    attr_reader :type
+    attr_reader :name
+    attr_reader :created_at
+    attr_reader :params
+    attr_reader :run_context
+    attr_reader :cookbook_name
+    attr_reader :recipe_name
+    attr_reader :enclosing_provider
+    attr_reader :resource
+
+    # FIXME (ruby-2.1 syntax): most of these are mandatory
+    def initialize(type:nil, name:nil, created_at: nil, params: nil, run_context: nil, cookbook_name: nil, recipe_name: nil, enclosing_provider: nil)
+      @type               = type
+      @name               = name
+      @created_at         = created_at
+      @params             = params
+      @run_context        = run_context
+      @cookbook_name      = cookbook_name
+      @recipe_name        = recipe_name
+      @enclosing_provider = enclosing_provider
+    end
+
+    def build(&block)
+      raise ArgumentError, "You must supply a name when declaring a #{type} resource" if name.nil?
+
+      @resource = resource_class.new(name, run_context)
+      resource.source_line = created_at
+      resource.declared_type = type
+
+      # If we have a resource like this one, we want to steal its state
+      # This behavior is very counter-intuitive and should be removed.
+      # See CHEF-3694, https://tickets.opscode.com/browse/CHEF-3694
+      # Moved to this location to resolve CHEF-5052, https://tickets.opscode.com/browse/CHEF-5052
+      if prior_resource
+        resource.load_from(prior_resource)
+      end
+
+      resource.cookbook_name = cookbook_name
+      resource.recipe_name = recipe_name
+      # Determine whether this resource is being created in the context of an enclosing Provider
+      resource.enclosing_provider = enclosing_provider
+
+      # XXX: this is required for definition params inside of the scope of a
+      # subresource to work correctly.
+      resource.params = params
+
+      # Evaluate resource attribute DSL
+      resource.instance_eval(&block) if block_given?
+
+      # emit a cloned resource warning if it is warranted
+      if prior_resource
+        if is_trivial_resource?(prior_resource) && identicalish_resources?(prior_resource, resource)
+          emit_harmless_cloning_debug
+        else
+          emit_cloned_resource_warning
+        end
+      end
+
+      # Run optional resource hook
+      resource.after_created
+
+      resource
+    end
+
+    private
+
+    def resource_class
+      # Checks the new platform => short_name => resource mapping initially
+      # then fall back to the older approach (Chef::Resource.const_get) for
+      # backward compatibility
+      resource_class ||= Chef::Resource.resource_for_node(type, run_context.node)
+    end
+
+    def is_trivial_resource?(resource)
+      identicalish_resources?(resource_class.new(name, run_context), resource)
+    end
+
+    # this is an equality test specific to checking for 3694 cloning warnings
+    def identicalish_resources?(first, second)
+      skipped_ivars = [ :@source_line, :@cookbook_name, :@recipe_name, :@params, :@elapsed_time, :@declared_type ]
+      checked_ivars = ( first.instance_variables | second.instance_variables ) - skipped_ivars
+      non_matching_ivars = checked_ivars.reject do |iv|
+        if iv == :@action && ( [first.instance_variable_get(iv)].flatten == [:nothing] || [second.instance_variable_get(iv)].flatten == [:nothing] )
+          # :nothing action on either side of the comparison always matches
+          true
+        else
+          first.instance_variable_get(iv) == second.instance_variable_get(iv)
+        end
+      end
+      Chef::Log.debug("ivars which did not match with the prior resource: #{non_matching_ivars}")
+      non_matching_ivars.empty?
+    end
+
+    def emit_cloned_resource_warning
+      Chef::Log.warn("Cloning resource attributes for #{self} from prior resource (CHEF-3694)")
+      Chef::Log.warn("Previous #{prior_resource}: #{prior_resource.source_line}") if prior_resource.source_line
+      Chef::Log.warn("Current  #{resource}: #{resource.source_line}") if resource.source_line
+    end
+
+    def emit_harmless_cloning_debug
+      Chef::Log.debug("Harmless resource cloning from #{prior_resource}:#{prior_resource.source_line} to #{resource}:#{resource.source_line}")
+    end
+
+    def prior_resource
+      @prior_resource ||=
+        begin
+          key = "#{type}[#{name}]"
+          prior_resource = run_context.resource_collection.lookup(key)
+        rescue Chef::Exceptions::ResourceNotFound
+          nil
+        end
+    end
+
+  end
+end