Merge pull request #2624 from opscode/lcg/suppress-3694

Skip 3694 warnings on trivial resource cloning

7 files changed, 232 insertions, 50 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 050d60a3c9..d39d0b7820 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -28,6 +28,7 @@
 * Audit mode feature added - see the RELEASE_NOTES for details
 * shell_out now sets `LANGUAGE` and `LANG` to the `Chef::Config[:internal_locale]` in addition to `LC_ALL` forcing
 * chef_gem supports a compile_time flag and will warn if it is not set (behavior will change in the future)
+* suppress 3694 warnings on the most trivial resource cloning
 
 ## 12.0.3
 * [**Phil Dibowitz**](https://github.com/jaymzh):
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
diff --git a/spec/unit/recipe_spec.rb b/spec/unit/recipe_spec.rb
index e8c1358ba2..22389a1a82 100644
--- a/spec/unit/recipe_spec.rb
+++ b/spec/unit/recipe_spec.rb
@@ -161,7 +161,82 @@ describe Chef::Recipe do
         zm_resource # force let binding evaluation
         expect { run_context.resource_collection.resources(:zen_master => "klopp") }.to raise_error(Chef::Exceptions::ResourceNotFound)
       end
+    end
+
+    describe "when cloning resources" do
+      def expect_warning
+        expect(Chef::Log).to receive(:warn).with(/3694/)
+        expect(Chef::Log).to receive(:warn).with(/Previous/)
+        expect(Chef::Log).to receive(:warn).with(/Current/)
+      end
+
+      it "should emit a 3694 warning when attributes change" do
+        recipe.zen_master "klopp" do
+          something "bvb"
+        end
+        expect_warning
+        recipe.zen_master "klopp" do
+          something "vbv"
+        end
+      end
+
+      it "should emit a 3694 warning when attributes change" do
+        recipe.zen_master "klopp" do
+          something "bvb"
+        end
+        expect_warning
+        recipe.zen_master "klopp" do
+          something "bvb"
+          peace true
+        end
+      end
 
+      it "should emit a 3694 warning when attributes change" do
+        recipe.zen_master "klopp" do
+          something "bvb"
+          peace true
+        end
+        expect_warning
+        recipe.zen_master "klopp" do
+          something "bvb"
+        end
+      end
+
+      it "should emit a 3694 warning for non-trivial attributes (unfortunately)" do
+        recipe.zen_master "klopp" do
+          something "bvb"
+        end
+        expect_warning
+        recipe.zen_master "klopp" do
+          something "bvb"
+        end
+      end
+
+      it "should not emit a 3694 warning for completely trivial resource cloning" do
+        recipe.zen_master "klopp"
+        expect(Chef::Log).to_not receive(:warn)
+        recipe.zen_master "klopp"
+      end
+
+      it "should not emit a 3694 warning when attributes do not change and the first action is :nothing" do
+        recipe.zen_master "klopp" do
+          action :nothing
+        end
+        expect(Chef::Log).to_not receive(:warn)
+        recipe.zen_master "klopp" do
+          action :score
+        end
+      end
+
+      it "should not emit a 3694 warning when attributes do not change and the second action is :nothing" do
+        recipe.zen_master "klopp" do
+          action :score
+        end
+        expect(Chef::Log).to_not receive(:warn)
+        recipe.zen_master "klopp" do
+          action :nothing
+        end
+      end
     end
 
     describe "creating resources via declare_resource" do
diff --git a/spec/unit/resource_builder_spec.rb b/spec/unit/resource_builder_spec.rb
new file mode 100644
index 0000000000..e38c7fb63a
--- /dev/null
+++ b/spec/unit/resource_builder_spec.rb
@@ -0,0 +1 @@
+# see spec/unit/recipe_spec.rb
diff --git a/spec/unit/resource_spec.rb b/spec/unit/resource_spec.rb
index 79d47ad4dc..56c7401c92 100644
--- a/spec/unit/resource_spec.rb
+++ b/spec/unit/resource_spec.rb
@@ -162,7 +162,7 @@ describe Chef::Resource do
     end
   end
 
-  describe "load_prior_resource" do
+  describe "load_from" do
     before(:each) do
       @prior_resource = Chef::Resource.new("funk")
       @prior_resource.supports(:funky => true)
@@ -174,12 +174,12 @@ describe Chef::Resource do
     end
 
     it "should load the attributes of a prior resource" do
-      @resource.load_prior_resource(@resource.resource_name, @resource.name)
+      @resource.load_from(@prior_resource)
       expect(@resource.supports).to eq({ :funky => true })
     end
 
     it "should not inherit the action from the prior resource" do
-      @resource.load_prior_resource(@resource.resource_name, @resource.name)
+      @resource.load_from(@prior_resource)
       expect(@resource.action).not_to eq(@prior_resource.action)
     end
   end