Don't warn when setting a property to nil unless its value wouldjk/updates2

actually change. Gets rid of cases where we are initializing a
resource with values from another resource.

2 files changed, 25 insertions, 20 deletions

diff --git a/lib/chef/property.rb b/lib/chef/property.rb
index b2658235f2..742eba42fd 100644
--- a/lib/chef/property.rb
+++ b/lib/chef/property.rb
@@ -251,16 +251,19 @@ class Chef
         return get(resource)
       end
 
-      # myprop nil is sometimes a get (backcompat)
       if value.nil? && !explicitly_accepts_nil?(resource)
-        # If you say "my_property nil" and the property explicitly accepts
-        # nil values, we consider this a get.
-        Chef.log_deprecation("#{name} nil currently does not overwrite the value of #{name}. This will change in Chef 13, and the value will be set to nil instead. Please change your code to explicitly accept nil using \"property :#{name}, [MyType, nil]\", or stop setting this value to nil.")
-        return get(resource)
+        # In Chef 12, value(nil) does a *get* instead of a set, so we
+        # warn if the value would have been changed. In Chef 13, it will be
+        # equivalent to value = nil.
+        result = get(resource)
+        if !result.nil?
+          Chef.log_deprecation("#{name} nil currently does not overwrite the value of #{name}. This will change in Chef 13, and the value will be set to nil instead. Please change your code to explicitly accept nil using \"property :#{name}, [MyType, nil]\", or stop setting this value to nil.")
+        end
+        result
+      else
+        # Anything else, such as myprop(value) is a set
+        set(resource, value)
       end
-
-      # Anything else (myprop value) is a set
-      set(resource, value)
     end
 
     #
diff --git a/spec/unit/property/validation_spec.rb b/spec/unit/property/validation_spec.rb
index 31bb3f0739..a54a38eb85 100644
--- a/spec/unit/property/validation_spec.rb
+++ b/spec/unit/property/validation_spec.rb
@@ -137,12 +137,8 @@ describe "Chef::Resource.property validation" do
         it "set to invalid value raises ValidationFailed" do
           expect { resource.x 10 }.to raise_error Chef::Exceptions::ValidationFailed
         end
-        it "set to nil emits a deprecation warning and does a get" do
-          expect { resource.x nil }.to raise_error Chef::Exceptions::DeprecatedFeatureError
-          Chef::Config[:treat_deprecation_warnings_as_errors] = false
-          resource.x 'str'
-          expect(resource.x nil).to eq 'str'
-          expect(resource.x).to eq 'str'
+        it "set to nil emits no warning because the value would not change" do
+          expect(resource.x nil).to be_nil
         end
       end
     end
@@ -534,12 +530,18 @@ describe "Chef::Resource.property validation" do
         expect(resource.x 1).to eq 1
         expect(resource.x).to eq 1
       end
-      it "value nil emits a deprecation warning and does a get" do
-        expect { resource.x nil }.to raise_error Chef::Exceptions::DeprecatedFeatureError
-        Chef::Config[:treat_deprecation_warnings_as_errors] = false
-        resource.x 1
-        expect(resource.x nil).to eq 1
-        expect(resource.x).to eq 1
+      it "value nil emits a validation failed error because it must have a value" do
+        expect { resource.x nil }.to raise_error Chef::Exceptions::ValidationFailed
+      end
+      context "and value is set to something other than nil" do
+        before { resource.x 10 }
+        it "value nil emits a deprecation warning and does a get" do
+          expect { resource.x nil }.to raise_error Chef::Exceptions::DeprecatedFeatureError
+          Chef::Config[:treat_deprecation_warnings_as_errors] = false
+          resource.x 1
+          expect(resource.x nil).to eq 1
+          expect(resource.x).to eq 1
+        end
       end
     end