Merge pull request #10656 from jaymzh/fixuser

Signed-off-by: Tim Smith <tsmith@chef.io>

5 files changed, 48 insertions, 19 deletions

diff --git a/lib/chef/provider/group.rb b/lib/chef/provider/group.rb
index 9e3d7f9a00..0cda3182ba 100644
--- a/lib/chef/provider/group.rb
+++ b/lib/chef/provider/group.rb
@@ -159,7 +159,7 @@ class Chef
 
         converge_by(["manage group #{new_resource.group_name}"] + change_desc) do
           manage_group
-          logger.info("#{new_resource} managed")
+          logger.info("#{new_resource} managed: #{change_desc.join(", ")}")
         end
       end
 
@@ -168,7 +168,7 @@ class Chef
 
         converge_by(["modify group #{new_resource.group_name}"] + change_desc) do
           manage_group
-          logger.info("#{new_resource} modified")
+          logger.info("#{new_resource} modified: #{change_desc.join(", ")}")
         end
       end
 
diff --git a/lib/chef/provider/user.rb b/lib/chef/provider/user.rb
index d4a2651378..803d87d820 100644
--- a/lib/chef/provider/user.rb
+++ b/lib/chef/provider/user.rb
@@ -24,6 +24,7 @@ class Chef
     class User < Chef::Provider
 
       attr_accessor :user_exists, :locked
+      attr_accessor :change_desc
 
       def initialize(new_resource, run_context)
         super
@@ -107,13 +108,20 @@ class Chef
       # <true>:: If a change is required
       # <false>:: If the users are identical
       def compare_user
-        return true if !new_resource.home.nil? && Pathname.new(new_resource.home).cleanpath != Pathname.new(current_resource.home).cleanpath
+        @change_desc = []
+        if !new_resource.home.nil? && Pathname.new(new_resource.home).cleanpath != Pathname.new(current_resource.home).cleanpath
+          @change_desc << "change homedir from #{current_resource.home} to #{new_resource.home}"
+        end
 
         %i{comment shell password uid gid}.each do |user_attrib|
-          return true if !new_resource.send(user_attrib).nil? && new_resource.send(user_attrib).to_s != current_resource.send(user_attrib).to_s
+          new_val = new_resource.send(user_attrib)
+          cur_val = current_resource.send(user_attrib)
+          if !new_val.nil? && new_val.to_s != cur_val.to_s
+            @change_desc << "change #{user_attrib} from #{cur_val} to #{new_val}"
+          end
         end
 
-        false
+        !@change_desc.empty?
       end
 
       action :create do
@@ -123,9 +131,9 @@ class Chef
             logger.info("#{new_resource} created")
           end
         elsif compare_user
-          converge_by("alter user #{new_resource.username}") do
+          converge_by(["alter user #{new_resource.username}"] + change_desc) do
             manage_user
-            logger.info("#{new_resource} altered")
+            logger.info("#{new_resource} altered, #{change_desc.join(", ")}")
           end
         end
       end
@@ -142,18 +150,18 @@ class Chef
       action :manage do
         return unless @user_exists && compare_user
 
-        converge_by("manage user #{new_resource.username}") do
+        converge_by(["manage user #{new_resource.username}"] + change_desc) do
           manage_user
-          logger.info("#{new_resource} managed")
+          logger.info("#{new_resource} managed: #{change_desc.join(", ")}")
         end
       end
 
       action :modify do
         return unless compare_user
 
-        converge_by("modify user #{new_resource.username}") do
+        converge_by(["modify user #{new_resource.username}"] + change_desc) do
           manage_user
-          logger.info("#{new_resource} modified")
+          logger.info("#{new_resource} modified: #{change_desc.join(", ")}")
         end
       end
 
diff --git a/lib/chef/provider/user/mac.rb b/lib/chef/provider/user/mac.rb
index a3980aa12f..aef831907a 100644
--- a/lib/chef/provider/user/mac.rb
+++ b/lib/chef/provider/user/mac.rb
@@ -221,7 +221,17 @@ class Chef
         end
 
         def compare_user
-          %i{comment shell uid gid salt password admin secure_token hidden}.any? { |m| diverged?(m) }
+          @change_desc = []
+          %i{comment shell uid gid salt password admin secure_token hidden}.each do |attr|
+            if diverged?(m)
+              desc = "Update #{attr}"
+              unless %i{password gid secure_token hidden}.include?(attr)
+                desc << " from #{current_resource.send(attr)} to #{new_resource.send(attr)}"
+              end
+              @change_desc << desc
+            end
+          end
+          !@change_desc.empty?
         end
 
         def manage_user
@@ -290,9 +300,7 @@ class Chef
           end
 
           if diverged?(:hidden)
-            converge_by("alter hidden") do
-              set_hidden
-            end
+            converge_by("alter hidden") { set_hidden }
           end
 
           reload_user_plist
diff --git a/lib/chef/provider/user/windows.rb b/lib/chef/provider/user/windows.rb
index 6d50e2c8ee..32b2c35264 100644
--- a/lib/chef/provider/user/windows.rb
+++ b/lib/chef/provider/user/windows.rb
@@ -61,13 +61,20 @@ class Chef
         # <true>:: If a change is required
         # <false>:: If the users are identical
         def compare_user
+          @change_desc = []
           unless @net_user.validate_credentials(new_resource.password)
-            logger.trace("#{new_resource} password has changed")
-            return true
+            @change_desc << "update password"
           end
+
           %i{uid comment home shell full_name}.any? do |user_attrib|
-            !new_resource.send(user_attrib).nil? && new_resource.send(user_attrib) != current_resource.send(user_attrib)
+            new_val = new_resource.send(user_attrib)
+            cur_val = current_resource.send(user_attrib)
+            if !new_val.nil? && new_val != cur_val
+              @change_desc << "change #{user_attrib} from #{cur_val} to #{new_val}"
+            end
           end
+
+          !@change_desc.empty?
         end
 
         def create_user
diff --git a/spec/unit/provider/user_spec.rb b/spec/unit/provider/user_spec.rb
index ac1c943dbf..16428de1a9 100644
--- a/spec/unit/provider/user_spec.rb
+++ b/spec/unit/provider/user_spec.rb
@@ -259,6 +259,7 @@ describe Chef::Provider::User do
     it "should call manage_user if the user exists and has mismatched properties" do
       @provider.user_exists = true
       allow(@provider).to receive(:compare_user).and_return(true)
+      allow(@provider).to receive(:change_desc).and_return([ ])
       expect(@provider).to receive(:manage_user).and_return(true)
       @provider.action_create
     end
@@ -266,6 +267,7 @@ describe Chef::Provider::User do
     it "should set the new_resources updated flag when it creates the user if we call manage_user" do
       @provider.user_exists = true
       allow(@provider).to receive(:compare_user).and_return(true)
+      allow(@provider).to receive(:change_desc).and_return([ ])
       allow(@provider).to receive(:manage_user).and_return(true)
       @provider.action_create
       @provider.set_updated_status
@@ -315,14 +317,16 @@ describe Chef::Provider::User do
       # @provider.stub(:manage_user).and_return(true)
     end
 
-    it "should run manage_user if the user exists and has mismatched properties" do
+    it "should call manage_user if the user exists and has mismatched properties" do
       expect(@provider).to receive(:compare_user).and_return(true)
+      allow(@provider).to receive(:change_desc).and_return([ ])
       expect(@provider).to receive(:manage_user).and_return(true)
       @provider.action_manage
     end
 
     it "should set the new resources updated flag to true if manage_user is called" do
       allow(@provider).to receive(:compare_user).and_return(true)
+      allow(@provider).to receive(:change_desc).and_return([ ])
       allow(@provider).to receive(:manage_user).and_return(true)
       @provider.action_manage
       @provider.set_updated_status
@@ -360,12 +364,14 @@ describe Chef::Provider::User do
 
     it "should run manage_user if the user exists and has mismatched properties" do
       expect(@provider).to receive(:compare_user).and_return(true)
+      allow(@provider).to receive(:change_desc).and_return([ ])
       expect(@provider).to receive(:manage_user).and_return(true)
       @provider.action_modify
     end
 
     it "should set the new resources updated flag to true if manage_user is called" do
       allow(@provider).to receive(:compare_user).and_return(true)
+      allow(@provider).to receive(:change_desc).and_return([ ])
       allow(@provider).to receive(:manage_user).and_return(true)
       @provider.action_modify
       @provider.set_updated_status