Merge pull request #10642 from jaymzh/groupfixes

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

4 files changed, 31 insertions, 6 deletions

diff --git a/lib/chef/provider/group.rb b/lib/chef/provider/group.rb
index 905a22dc33..9e3d7f9a00 100644
--- a/lib/chef/provider/group.rb
+++ b/lib/chef/provider/group.rb
@@ -78,7 +78,7 @@ class Chef
       # <false>:: If a change is not required
       def compare_group
         @change_desc = [ ]
-        if new_resource.gid.to_s != current_resource.gid.to_s
+        unless group_gid_match?
           @change_desc << "change gid #{current_resource.gid} to #{new_resource.gid}"
         end
 
@@ -103,13 +103,21 @@ class Chef
           unless members_to_be_removed.empty?
             @change_desc << "remove existing member(s): #{members_to_be_removed.join(", ")}"
           end
-        elsif new_resource.members != current_resource.members
-          @change_desc << "replace group members with new list of members"
+        elsif !group_members_match?
+          @change_desc << "replace group members with new list of members: #{new_resource.members.join(", ")}"
         end
 
         !@change_desc.empty?
       end
 
+      def group_gid_match?
+        new_resource.gid.to_s == current_resource.gid.to_s
+      end
+
+      def group_members_match?
+        [new_resource.members].flatten.sort == [current_resource.members].flatten.sort
+      end
+
       def has_current_group_member?(member)
         current_resource.members.include?(member)
       end
@@ -131,7 +139,7 @@ class Chef
           if compare_group
             converge_by(["alter group #{new_resource.group_name}"] + change_desc) do
               manage_group
-              logger.info("#{new_resource} altered")
+              logger.info("#{new_resource} altered: #{change_desc.join(", ")}")
             end
           end
         end
diff --git a/lib/chef/provider/group/windows.rb b/lib/chef/provider/group/windows.rb
index 45914b16e3..dacfc348f7 100644
--- a/lib/chef/provider/group/windows.rb
+++ b/lib/chef/provider/group/windows.rb
@@ -33,6 +33,17 @@ class Chef
           @net_group = Chef::Util::Windows::NetGroup.new(new_resource.group_name)
         end
 
+        def group_members_match?
+          sorted_members_sids = new_resource.members.map { |x| lookup_account_name(x) }.sort
+          sorted_current_sids = current_resource.members.sort
+          Chef::Log.debug("#{new_resource.name}: current_members: #{sorted_current_sids} vs new_members #{sorted_members_sids}")
+          sorted_members_sids == sorted_current_sids
+        end
+
+        def group_gid_match?
+          true
+        end
+
         def load_current_resource
           @current_resource = Chef::Resource::Group.new(new_resource.name)
           current_resource.group_name(new_resource.group_name)
@@ -74,7 +85,7 @@ class Chef
               members_to_be_removed << member if has_current_group_member?(member)
             end
             @net_group.local_delete_members(members_to_be_removed) unless members_to_be_removed.empty?
-          else
+          elsif !group_members_match?
             @net_group.local_set_members(new_resource.members)
           end
           @net_group.local_group_set_info(new_resource.comment) if new_resource.comment
diff --git a/spec/unit/provider/group/windows_spec.rb b/spec/unit/provider/group/windows_spec.rb
index 61853c90bd..85b88000a8 100644
--- a/spec/unit/provider/group/windows_spec.rb
+++ b/spec/unit/provider/group/windows_spec.rb
@@ -39,6 +39,12 @@ describe Chef::Provider::Group::Windows do
   end
 
   describe "when creating the group" do
+    before do
+      @current_resource = Chef::Resource::Group.new("staff")
+      @current_resource.members %w{all your base}
+      @provider.current_resource = @current_resource
+    end
+
     it "should call @net_group.local_add" do
       expect(@net_group).to receive(:local_set_members).with([])
       expect(@net_group).to receive(:local_add)
diff --git a/spec/unit/provider/group_spec.rb b/spec/unit/provider/group_spec.rb
index 76fde1e141..a3701c1f45 100644
--- a/spec/unit/provider/group_spec.rb
+++ b/spec/unit/provider/group_spec.rb
@@ -266,7 +266,7 @@ describe Chef::Provider::User do
       @new_resource.members << "user1"
       allow(@new_resource).to receive(:append).and_return false
       expect(@provider.compare_group).to be_truthy
-      expect(@provider.change_desc).to eq([ "replace group members with new list of members" ])
+      expect(@provider.change_desc).to eq([ "replace group members with new list of members: aj, user1" ])
     end
 
     it "should report the gid will be changed when it does not match" do