Merge pull request #6204 from chef/lcg/deprecate-resource-namespace-magic

Deprecate property namespace magic

8 files changed, 52 insertions, 39 deletions

diff --git a/chef-config/lib/chef-config/package_task.rb b/chef-config/lib/chef-config/package_task.rb
index 33c28b23fa..3ab273532d 100644
--- a/chef-config/lib/chef-config/package_task.rb
+++ b/chef-config/lib/chef-config/package_task.rb
@@ -222,7 +222,7 @@ module ChefConfig
 # this repo. Do not edit this manually. Edit the VERSION file and run the rake
 # task instead.
 #!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
-#{"\nrequire 'chef/version_string'\n" if use_versionstring}
+#{"\nrequire \"chef/version_string\"\n" if use_versionstring}
 #{class_or_module} #{module_name}
   #{module_name.upcase}_ROOT = File.expand_path("../..", __FILE__)
   VERSION = #{use_versionstring ? "Chef::VersionString.new(\"#{version}\")" : "\"#{version}\""}
diff --git a/lib/chef/deprecated.rb b/lib/chef/deprecated.rb
index 372609c10a..2cf0f3d1bc 100644
--- a/lib/chef/deprecated.rb
+++ b/lib/chef/deprecated.rb
@@ -248,6 +248,16 @@ class Chef
       end
     end
 
+    class NamespaceCollisions < Base
+      def id
+        19
+      end
+
+      def target
+        "namespace_collisions.html"
+      end
+    end
+
     # id 3694 was deleted
 
     class Generic < Base
diff --git a/lib/chef/dsl/declare_resource.rb b/lib/chef/dsl/declare_resource.rb
index 6869e77eca..0d727b4861 100644
--- a/lib/chef/dsl/declare_resource.rb
+++ b/lib/chef/dsl/declare_resource.rb
@@ -155,7 +155,15 @@ class Chef
       def edit_resource(type, name, created_at: nil, run_context: self.run_context, &resource_attrs_block)
         edit_resource!(type, name, created_at: created_at, run_context: run_context, &resource_attrs_block)
       rescue Chef::Exceptions::ResourceNotFound
-        declare_resource(type, name, created_at: created_at, run_context: run_context, &resource_attrs_block)
+        resource = declare_resource(type, name, created_at: created_at, run_context: run_context)
+        if resource_attrs_block
+          if defined?(new_resource)
+            resource.instance_exec(new_resource, &resource_attrs_block)
+          else
+            resource.instance_exec(&resource_attrs_block)
+          end
+        end
+        resource
       end
 
       # Lookup a resource in the resource collection by name.  If the resource is not
diff --git a/lib/chef/provider.rb b/lib/chef/provider.rb
index c7048d50e5..1ebdfa6feb 100644
--- a/lib/chef/provider.rb
+++ b/lib/chef/provider.rb
@@ -328,8 +328,6 @@ class Chef
           resource.class.properties.each do |name, property|
             class_eval(<<-EOM, __FILE__, __LINE__)
               def #{name}(*args, &block)
-                # FIXME:  DEPRECATE THIS IN CHEF 13.1
-                #
                 # If no arguments were passed, we process "get" by defaulting
                 # the value to current_resource, not new_resource. This helps
                 # avoid issues where resources accidentally overwrite perfectly
@@ -350,9 +348,11 @@ class Chef
                 #
                 if args.empty? && !block
                   if !new_resource.property_is_set?(__method__) && current_resource
+                    Chef.deprecated(:namespace_collisions, "rename #{name} to current_resource.#{name}")
                     return current_resource.public_send(__method__)
                   end
                 end
+                Chef.deprecated(:namespace_collisions, "rename #{name} to new_resource.#{name}")
                 new_resource.public_send(__method__, *args, &block)
               end
             EOM
diff --git a/lib/chef/version.rb b/lib/chef/version.rb
index dfd7af6fef..8b941445ab 100644
--- a/lib/chef/version.rb
+++ b/lib/chef/version.rb
@@ -19,7 +19,7 @@
 # task instead.
 #!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
 
-require 'chef/version_string'
+require "chef/version_string"
 
 class Chef
   CHEF_ROOT = File.expand_path("../..", __FILE__)
diff --git a/spec/integration/recipes/accumulator_spec.rb b/spec/integration/recipes/accumulator_spec.rb
index e6afe09b8c..4a193bd7f0 100644
--- a/spec/integration/recipes/accumulator_spec.rb
+++ b/spec/integration/recipes/accumulator_spec.rb
@@ -62,7 +62,7 @@ describe "Accumulators" do
           default_action :create
 
           action :create do
-            email_alias address do
+            email_alias new_resource.address do
               recipients new_resource.recipients
             end
           end
@@ -78,7 +78,7 @@ describe "Accumulators" do
           default_action :create
 
           action :create do
-            nested address do
+            nested new_resource.address do
               recipients new_resource.recipients
             end
           end
@@ -149,8 +149,8 @@ describe "Accumulators" do
                 delayed_action :create
               end
             end
-            r.variables[:aliases][address] ||= []
-            r.variables[:aliases][address] += new_resource.recipients
+            r.variables[:aliases][new_resource.address] ||= []
+            r.variables[:aliases][new_resource.address] += new_resource.recipients
           end
         EOM
 
@@ -164,7 +164,7 @@ describe "Accumulators" do
           default_action :create
 
           action :create do
-            email_alias address do
+            email_alias new_resource.address do
               recipients new_resource.recipients
             end
           end
@@ -180,7 +180,7 @@ describe "Accumulators" do
           default_action :create
 
           action :create do
-            nested address do
+            nested new_resource.address do
               recipients new_resource.recipients
             end
           end
diff --git a/spec/integration/recipes/resource_action_spec.rb b/spec/integration/recipes/resource_action_spec.rb
index 03331c8625..f5f2d4a764 100644
--- a/spec/integration/recipes/resource_action_spec.rb
+++ b/spec/integration/recipes/resource_action_spec.rb
@@ -416,6 +416,7 @@ module ResourceActionSpec
         attr_reader :x_warning_line
 
         it "Using the enclosing resource to set x to x emits a warning that you're using the wrong x" do
+          Chef::Config[:treat_deprecation_warnings_as_errors] = false
           recipe = converge do
             resource_action_spec_also_with_x "hi" do
               x 1
@@ -423,34 +424,43 @@ module ResourceActionSpec
             end
           end
           warnings = recipe.logs.lines.select { |l| l =~ /warn/i }
-          expect(warnings.size).to eq 1
+          expect(warnings.size).to eq 2
           expect(warnings[0]).to match(/property x is declared in both resource_action_spec_with_x\[hi\] and resource_action_spec_also_with_x\[hi\] action :set_x_to_x. Use new_resource.x instead. At #{__FILE__}:#{ResourceActionSpecAlsoWithX.x_warning_line}/)
         end
 
         it "Using the enclosing resource to set x to x outside the initializer emits no warning" do
-          expect_recipe do
+          Chef::Config[:treat_deprecation_warnings_as_errors] = false
+          recipe = converge do
             resource_action_spec_also_with_x "hi" do
               x 1
               action :set_x_to_x_in_non_initializer
             end
-          end.to emit_no_warnings_or_errors
+          end
+          warnings = recipe.logs.lines.select { |l| l =~ /warn/i }
+          expect(warnings.size).to eq 1  # the deprecation warning, not the property masking one
         end
 
         it "Using the enclosing resource to set x to 10 emits no warning" do
-          expect_recipe do
+          Chef::Config[:treat_deprecation_warnings_as_errors] = false
+          recipe = converge do
             resource_action_spec_also_with_x "hi" do
               x 1
               action :set_x_to_10
             end
-          end.to emit_no_warnings_or_errors
+          end
+          warnings = recipe.logs.lines.select { |l| l =~ /warn/i }
+          expect(warnings.size).to eq 1  # the deprecation warning, not the property masking one
         end
 
         it "Using the enclosing resource to set x to 10 emits no warning" do
-          expect_recipe do
+          Chef::Config[:treat_deprecation_warnings_as_errors] = false
+          recipe = converge do
             r = resource_action_spec_also_with_x "hi"
             r.x 1
             r.action :set_x_to_10
-          end.to emit_no_warnings_or_errors
+          end
+          warnings = recipe.logs.lines.select { |l| l =~ /warn/i }
+          expect(warnings.size).to eq 1  # the deprecation warning, not the property masking one
         end
       end
 
@@ -496,6 +506,7 @@ module ResourceActionSpec
       end
 
       it "Raises an error when attempting to use a template in the action" do
+        Chef::Config[:treat_deprecation_warnings_as_errors] = false
         expect_converge do
           has_property_named_template "hi"
         end.to raise_error(/Property `template` of `has_property_named_template\[hi\]` was incorrectly passed a block.  Possible property-resource collision.  To call a resource named `template` either rename the property or else use `declare_resource\(:template, ...\)`/)
diff --git a/spec/integration/recipes/resource_load_spec.rb b/spec/integration/recipes/resource_load_spec.rb
index 791b83c23a..4fc14c0687 100644
--- a/spec/integration/recipes/resource_load_spec.rb
+++ b/spec/integration/recipes/resource_load_spec.rb
@@ -32,7 +32,7 @@ describe "Resource.load_current_value" do
         @created
       end
       action :create do
-        new_resource.class.created_x = x
+        new_resource.class.created_x = new_resource.x
       end
     end
     result.resource_name resource_name
@@ -45,10 +45,10 @@ describe "Resource.load_current_value" do
   context "with a resource with load_current_value" do
     before :each do
       resource_class.load_current_value do
-        x "loaded #{Namer.incrementing_value} (#{self.class.properties.sort_by { |name, p| name }.
-          select { |name, p| p.is_set?(self) }.
-          map { |name, p| "#{name}=#{p.get(self)}" }.
-          join(", ")})"
+        x "loaded #{Namer.incrementing_value} (#{self.class.properties.sort_by { |name, p| name }
+          .select { |name, p| p.is_set?(self) }
+          .map { |name, p| "#{name}=#{p.get(self)}" }
+          .join(", ")})"
       end
     end
 
@@ -119,22 +119,6 @@ describe "Resource.load_current_value" do
       end
     end
 
-    context "and a resource with no values set" do
-      let(:resource) do
-        e = self
-        r = nil
-        converge do
-          r = public_send(e.resource_name, "blah") do
-          end
-        end
-        r
-      end
-
-      it "the provider accesses values from load_current_value" do
-        expect(resource.class.created_x).to eq "loaded 1 (name=blah)"
-      end
-    end
-
     let (:subresource_name) do
       :"load_current_value_subresource_dsl#{Namer.current_index}"
     end