Merge pull request #9581 from chef/lcg/redundant-identity

Change the name_property to be the identity property by default, and to have desired_state: false by default

24 files changed, 56 insertions, 38 deletions

diff --git a/lib/chef/mixin/properties.rb b/lib/chef/mixin/properties.rb
index ecb589e015..94afa4640b 100644
--- a/lib/chef/mixin/properties.rb
+++ b/lib/chef/mixin/properties.rb
@@ -264,10 +264,22 @@ class Chef
           end
 
           result = properties.values.select(&:identity?)
-          result = [ properties[:name] ] if result.empty?
+          # if there are no other identity properites set, then the name_property becomes the identity, or
+          # failing that we use the actual name.
+          if result.empty?
+            result = name_property ? [ properties[name_property] ] : [ properties[:name] ]
+          end
           result
         end
 
+        # Returns the name of the name property.  Returns nil if there is no name property.
+        #
+        # @return [Symbol] the name property for this resource
+        def name_property
+          p = properties.find { |n, p| p.name_property? }
+          p ? p.first : nil
+        end
+
         def included(other)
           other.extend ClassMethods
         end
diff --git a/lib/chef/property.rb b/lib/chef/property.rb
index 336331b59f..a29286c21b 100644
--- a/lib/chef/property.rb
+++ b/lib/chef/property.rb
@@ -1,7 +1,7 @@
 #
 # Author:: John Keiser <jkeiser@chef.io>
 # Copyright:: Copyright 2015-2016, John Keiser
-# Copyright:: Copyright 2015-2020, Chef Software, Inc.
+# Copyright:: Copyright 2015-2020, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -141,6 +141,10 @@ class Chef
         raise ArgumentError, "A property cannot be both a name_property/name_attribute and have a default value. Use one or the other on property #{self}"
       end
 
+      if options[:name_property]
+        options[:desired_state] = false unless options.key?(:desired_state)
+      end
+
       # Recursively freeze the default if it isn't a lazy value.
       unless default.is_a?(DelayedEvaluator)
         visitor = lambda do |obj|
diff --git a/lib/chef/resource.rb b/lib/chef/resource.rb
index 5153dd4910..9cdf28dd9a 100644
--- a/lib/chef/resource.rb
+++ b/lib/chef/resource.rb
@@ -498,7 +498,7 @@ class Chef
       state = {}
       state_properties = self.class.state_properties
       state_properties.each do |property|
-        if property.identity? || property.is_set?(self)
+        if property.is_set?(self)
           state[property.name] = property.sensitive? ? "*sensitive value suppressed*" : send(property.name)
         end
       end
diff --git a/lib/chef/resource/directory.rb b/lib/chef/resource/directory.rb
index c8356f64ac..36cbe076f6 100644
--- a/lib/chef/resource/directory.rb
+++ b/lib/chef/resource/directory.rb
@@ -42,7 +42,7 @@ class Chef
       default_action :create
       allowed_actions :create, :delete
 
-      property :path, String, name_property: true, identity: true,
+      property :path, String, name_property: true,
                description: "The path to the directory. Using a fully qualified path is recommended, but is not always required."
 
       property :recursive, [ TrueClass, FalseClass ],
diff --git a/lib/chef/resource/execute.rb b/lib/chef/resource/execute.rb
index 7073ddc604..646e798bc7 100644
--- a/lib/chef/resource/execute.rb
+++ b/lib/chef/resource/execute.rb
@@ -49,7 +49,7 @@ class Chef
       end
 
       property :command, [ String, Array ],
-        name_property: true, identity: true,
+        name_property: true,
         description: "An optional property to set the command to be executed if it differs from the resource block's name."
 
       property :umask, [ String, Integer ],
diff --git a/lib/chef/resource/file.rb b/lib/chef/resource/file.rb
index 73d6f2de27..c3100d1160 100644
--- a/lib/chef/resource/file.rb
+++ b/lib/chef/resource/file.rb
@@ -55,7 +55,7 @@ class Chef
       default_action :create
       allowed_actions :create, :delete, :touch, :create_if_missing
 
-      property :path, String, name_property: true, identity: true,
+      property :path, String, name_property: true,
                description: "The full path to the file, including the file name and its extension. For example: /files/file.txt. Default value: the name of the resource block. Microsoft Windows: A path that begins with a forward slash (/) will point to the root of the current working directory of the #{Chef::Dist::CLIENT} process. This path can vary from system to system. Therefore, using a path that begins with a forward slash (/) is not recommended."
 
       property :atomic_update, [ TrueClass, FalseClass ], desired_state: false, default: lazy { docker? && special_docker_files?(path) ? false : Chef::Config[:file_atomic_update] },
diff --git a/lib/chef/resource/group.rb b/lib/chef/resource/group.rb
index 058a5a63d4..7ba1eead9a 100644
--- a/lib/chef/resource/group.rb
+++ b/lib/chef/resource/group.rb
@@ -31,7 +31,7 @@ class Chef
       default_action :create
 
       property :group_name, String,
-        name_property: true, identity: true,
+        name_property: true,
         description: "The name of the group."
 
       property :gid, [ String, Integer ],
diff --git a/lib/chef/resource/kernel_module.rb b/lib/chef/resource/kernel_module.rb
index be8b007035..0d08ae8147 100644
--- a/lib/chef/resource/kernel_module.rb
+++ b/lib/chef/resource/kernel_module.rb
@@ -66,7 +66,7 @@ class Chef
 
       property :modname, String,
         description: "An optional property to set the kernel module name if it differs from the resource block's name.",
-        name_property: true, identity: true
+        name_property: true
 
       property :options, Array,
         description: "An optional property to set options for the kernel module.",
diff --git a/lib/chef/resource/launchd.rb b/lib/chef/resource/launchd.rb
index d939559b02..613ea415d2 100644
--- a/lib/chef/resource/launchd.rb
+++ b/lib/chef/resource/launchd.rb
@@ -31,7 +31,7 @@ class Chef
       allowed_actions :create, :create_if_missing, :delete, :enable, :disable, :restart
 
       property :label, String,
-        identity: true, name_property: true,
+        name_property: true,
         description: "The unique identifier for the job."
 
       property :backup, [Integer, FalseClass],
diff --git a/lib/chef/resource/link.rb b/lib/chef/resource/link.rb
index 7758e27433..636b748eb3 100644
--- a/lib/chef/resource/link.rb
+++ b/lib/chef/resource/link.rb
@@ -45,7 +45,7 @@ class Chef
 
       property :target_file, String,
         description: "An optional property to set the target file if it differs from the resource block's name.",
-        name_property: true, identity: true
+        name_property: true
 
       property :to, String,
         description: "The actual file to which the link is to be created."
diff --git a/lib/chef/resource/mdadm.rb b/lib/chef/resource/mdadm.rb
index 6ec547d133..ab5e074c04 100644
--- a/lib/chef/resource/mdadm.rb
+++ b/lib/chef/resource/mdadm.rb
@@ -59,7 +59,7 @@ class Chef
         description: "The path to a file in which a write-intent bitmap is stored."
 
       property :raid_device, String,
-        identity: true, name_property: true,
+        name_property: true,
         description: "An optional property to specify the name of the RAID device if it differs from the resource block's name."
 
       property :layout, String,
diff --git a/lib/chef/resource/osx_profile.rb b/lib/chef/resource/osx_profile.rb
index 5389696b72..8060db5076 100644
--- a/lib/chef/resource/osx_profile.rb
+++ b/lib/chef/resource/osx_profile.rb
@@ -34,7 +34,7 @@ class Chef
 
       property :profile_name, String,
         description: "Use to specify the name of the profile, if different from the name of the resource block.",
-        name_property: true, identity: true
+        name_property: true
 
       property :profile, [ String, Hash ],
         description: "Use to specify a profile. This may be the name of a profile contained in a cookbook or a Hash that contains the contents of the profile."
diff --git a/lib/chef/resource/registry_key.rb b/lib/chef/resource/registry_key.rb
index 1ecd631096..39246b1b50 100644
--- a/lib/chef/resource/registry_key.rb
+++ b/lib/chef/resource/registry_key.rb
@@ -70,7 +70,7 @@ class Chef
         @values, @unscrubbed_values = [], []
       end
 
-      property :key, String, name_property: true, identity: true
+      property :key, String, name_property: true
 
       def values(arg = nil)
         if not arg.nil?
diff --git a/lib/chef/resource/route.rb b/lib/chef/resource/route.rb
index 979dc12342..0d9d9d431c 100644
--- a/lib/chef/resource/route.rb
+++ b/lib/chef/resource/route.rb
@@ -33,7 +33,7 @@ class Chef
 
       property :target, String,
         description: "The IP address of the target route.",
-        identity: true, name_property: true
+        name_property: true
 
       property :comment, [String, nil],
         description: "Add a comment for the route.",
diff --git a/lib/chef/resource/ruby_block.rb b/lib/chef/resource/ruby_block.rb
index 5ecf1582d4..d4a3c47ec5 100644
--- a/lib/chef/resource/ruby_block.rb
+++ b/lib/chef/resource/ruby_block.rb
@@ -1,7 +1,7 @@
 #
 # Author:: Adam Jacob (<adam@chef.io>)
 # Author:: AJ Christensen (<aj@chef.io>)
-# Copyright:: Copyright 2008-2016, Chef Software Inc.
+# Copyright:: Copyright 2008-2020, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -44,7 +44,7 @@ class Chef
         end
       end
 
-      property :block_name, String, name_property: true, identity: true
+      property :block_name, String, name_property: true
     end
   end
 end
diff --git a/lib/chef/resource/scm.rb b/lib/chef/resource/scm.rb
index 40b4f8e77c..eb2bdca8b8 100644
--- a/lib/chef/resource/scm.rb
+++ b/lib/chef/resource/scm.rb
@@ -1,6 +1,6 @@
 #
 # Author:: Daniel DeLeo (<dan@kallistec.com>)
-# Copyright:: Copyright 2008-2016, Chef Software Inc.
+# Copyright:: Copyright 2008-2020, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -28,7 +28,7 @@ class Chef
 
       property :destination, String,
         description: "The location path to which the source is to be cloned, checked out, or exported. Default value: the name of the resource block.",
-        name_property: true, identity: true
+        name_property: true
 
       property :repository, String
 
diff --git a/lib/chef/resource/service.rb b/lib/chef/resource/service.rb
index 71eb344483..4c57cd07b0 100644
--- a/lib/chef/resource/service.rb
+++ b/lib/chef/resource/service.rb
@@ -31,8 +31,6 @@ class Chef
 
       provides :service, target_mode: true
 
-      identity_attr :service_name
-
       description "Use the service resource to manage a service."
 
       default_action :nothing
@@ -46,7 +44,7 @@ class Chef
 
       property :service_name, String,
         description: "An optional property to set the service name if it differs from the resource block's name.",
-        name_property: true, identity: true
+        name_property: true
 
       # regex for match against ps -ef when !supports[:has_status] && status == nil
       property :pattern, String,
diff --git a/lib/chef/resource/systemd_unit.rb b/lib/chef/resource/systemd_unit.rb
index 0d477323f9..9e7067bd70 100644
--- a/lib/chef/resource/systemd_unit.rb
+++ b/lib/chef/resource/systemd_unit.rb
@@ -63,7 +63,7 @@ class Chef
         description: "Specifies if the unit will be verified before installation. Systemd can be overly strict when verifying units, so in certain cases it is preferable not to verify the unit."
 
       property :unit_name, String, desired_state: false,
-               identity: true, name_property: true,
+               name_property: true,
                description: "The name of the unit file if it differs from the resource block's name.",
                introduced: "13.7"
 
diff --git a/lib/chef/resource/user.rb b/lib/chef/resource/user.rb
index b72cc22b2d..035ec53875 100644
--- a/lib/chef/resource/user.rb
+++ b/lib/chef/resource/user.rb
@@ -30,7 +30,7 @@ class Chef
 
       property :username, String,
         description: "An optional property to set the username value if it differs from the resource block's name.",
-        name_property: true, identity: true
+        name_property: true
 
       property :comment, String,
         description: "The contents of the user comments field."
diff --git a/lib/chef/resource/windows_env.rb b/lib/chef/resource/windows_env.rb
index 4eda6e22b9..b7f347b80b 100644
--- a/lib/chef/resource/windows_env.rb
+++ b/lib/chef/resource/windows_env.rb
@@ -32,7 +32,7 @@ class Chef
 
       property :key_name, String,
         description: "An optional property to set the name of the key that is to be created, deleted, or modified if it differs from the resource block's name.",
-        identity: true, name_property: true
+        name_property: true
 
       property :value, String,
         description: "The value of the environmental variable to set.",
diff --git a/lib/chef/resource_reporter.rb b/lib/chef/resource_reporter.rb
index 246b2d5500..c8517dfc83 100644
--- a/lib/chef/resource_reporter.rb
+++ b/lib/chef/resource_reporter.rb
@@ -3,7 +3,7 @@
 # Author:: Prajakta Purohit (prajakta@chef.io>)
 # Auther:: Tyler Cloke (<tyler@opscode.com>)
 #
-# 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");
diff --git a/spec/unit/property/state_spec.rb b/spec/unit/property/state_spec.rb
index a6428e8617..f120039f8b 100644
--- a/spec/unit/property/state_spec.rb
+++ b/spec/unit/property/state_spec.rb
@@ -285,8 +285,8 @@ describe "Chef::Resource#identity and #state" do
       it "identity when x is not defined returns the value of x" do
         expect(resource.identity).to eq "blah"
       end
-      it "state when x is not defined returns the value of x" do
-        expect(resource.state_for_resource_reporter).to eq(x: "blah")
+      it "the name property is not included in the desired state" do
+        expect(resource.state_for_resource_reporter).to eq({})
       end
     end
   end
@@ -332,12 +332,17 @@ describe "Chef::Resource#identity and #state" do
     end
 
     with_property ":x, name_property: true" do
-      # it "Unset values with name_property are included in state" do
-      #   expect(resource.state_for_resource_reporter).to eq({ x: 'blah' })
-      # end
-      it "Set values with name_property are included in state" do
+      it "Is by default the identity property if no other identity property is included" do
+        expect(resource.identity).to eq("blah")
+      end
+
+      it "Unset values with name_property are not included in state" do
+        expect(resource.state_for_resource_reporter).to eq({})
+      end
+
+      it "Set values with name_property are not included in state" do
         resource.x 1
-        expect(resource.state_for_resource_reporter).to eq(x: 1)
+        expect(resource.state_for_resource_reporter).to eq({})
       end
     end
 
diff --git a/spec/unit/property_spec.rb b/spec/unit/property_spec.rb
index 965da7313e..177314d8bc 100644
--- a/spec/unit/property_spec.rb
+++ b/spec/unit/property_spec.rb
@@ -1022,6 +1022,9 @@ describe "Chef::Resource.property" do
         it "defaults x to resource.name" do
           expect(resource.x).to eq "blah"
         end
+        it "defaults to being part of the identity if there is no other identity" do
+          expect(resource.identity).to eq "blah"
+        end
         it "does not pick up resource.name if set" do
           expect(resource.x 10).to eq 10
           expect(resource.x).to eq 10
diff --git a/spec/unit/resource_reporter_spec.rb b/spec/unit/resource_reporter_spec.rb
index 775b14ef19..22bba578eb 100644
--- a/spec/unit/resource_reporter_spec.rb
+++ b/spec/unit/resource_reporter_spec.rb
@@ -3,7 +3,7 @@
 # Author:: Prajakta Purohit (<prajakta@chef.io>)
 # Author:: Tyler Cloke (<tyler@chef.io>)
 #
-# 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");
@@ -294,7 +294,7 @@ describe Chef::ResourceReporter do
       resource_reporter.run_started(run_status)
     end
 
-    context "when the new_resource is sensitive" do
+    context "when the new_resource is a sensitive execute resource" do
       before do
         @execute_resource = Chef::Resource::Execute.new("sensitive-resource")
         @execute_resource.name("sensitive-resource")
@@ -312,10 +312,6 @@ describe Chef::ResourceReporter do
       it "resource_name in prepared_run_data should be the same" do
         expect(@first_update_report["name"]).to eq("sensitive-resource")
       end
-
-      it "resource_command in prepared_run_data should be blank" do
-        expect(@first_update_report["after"]).to eq({ command: "sensitive-resource" })
-      end
     end
 
     context "when the new_resource does not have a string for name and identity" do