10 files changed, 77 insertions, 35 deletions

diff --git a/lib/chef/provider/group.rb b/lib/chef/provider/group.rb
index a89b4a57b3..09c2a0052d 100644
--- a/lib/chef/provider/group.rb
+++ b/lib/chef/provider/group.rb
@@ -125,7 +125,7 @@ class Chef
           end
         else
           if compare_group
-            converge_by(["alter group #{@new_resource}"] + @change_desc) do
+            converge_by(["alter group #{@new_resource}"] + change_desc) do
               manage_group
               Chef::Log.info("#{@new_resource} altered")
             end
@@ -144,7 +144,7 @@ class Chef
 
       def action_manage
         if @group_exists && compare_group
-          converge_by(["manage group #{@new_resource}"] + @change_desc) do
+          converge_by(["manage group #{@new_resource}"] + change_desc) do
             manage_group
             Chef::Log.info("#{@new_resource} managed")
           end
@@ -153,7 +153,7 @@ class Chef
 
       def action_modify
         if compare_group
-          converge_by(["modify group #{@new_resource}"] + @change_desc) do
+          converge_by(["modify group #{@new_resource}"] + change_desc) do
             manage_group
             Chef::Log.info("#{@new_resource} modified")
           end
diff --git a/lib/chef/provider/group/groupadd.rb b/lib/chef/provider/group/groupadd.rb
index eb1972d96e..cb480aab54 100644
--- a/lib/chef/provider/group/groupadd.rb
+++ b/lib/chef/provider/group/groupadd.rb
@@ -65,7 +65,7 @@ class Chef
         end
 
         def modify_group_members
-          if(@new_resource.append)
+          if @new_resource.append
             if @new_resource.members && !@new_resource.members.empty?
               members_to_be_added = [ ]
               @new_resource.members.each do |member|
@@ -89,7 +89,8 @@ class Chef
               end
             end
           else
-            Chef::Log.debug("#{@new_resource} setting group members to: #{@new_resource.members.join(", ")}")
+            members_description = @new_resource.members.empty? ? "none" : @new_resource.members.join(", ")
+            Chef::Log.debug("#{@new_resource} setting group members to: #{members_description}")
             set_members(@new_resource.members)
           end
         end
diff --git a/lib/chef/provider/group/usermod.rb b/lib/chef/provider/group/usermod.rb
index 4863c6ee4a..ca035d0fc8 100644
--- a/lib/chef/provider/group/usermod.rb
+++ b/lib/chef/provider/group/usermod.rb
@@ -17,12 +17,15 @@
 #
 
 require 'chef/provider/group/groupadd'
+require 'chef/mixin/shell_out'
 
 class Chef
   class Provider
     class Group
       class Usermod < Chef::Provider::Group::Groupadd
 
+        include Chef::Mixin::ShellOut
+
         def load_current_resource
           super
         end
@@ -44,7 +47,7 @@ class Chef
 
           requirements.assert(:all_actions) do |a|
             a.assertion { @new_resource.excluded_members.empty? }
-            a.failure_message Chef::Exceptions::Group "excluded_members is not supported by #{self.to_s}"
+            a.failure_message Chef::Exceptions::Group, "excluded_members is not supported by #{self.to_s}"
             # No whyrun alternative - this action is simply not supported.
           end
         end
diff --git a/lib/chef/resource/group.rb b/lib/chef/resource/group.rb
index 707def9e39..4ec7780514 100644
--- a/lib/chef/resource/group.rb
+++ b/lib/chef/resource/group.rb
@@ -24,6 +24,7 @@ class Chef
       identity_attr :group_name
 
       state_attrs :members
+      state_attrs :excluded_members
 
       def initialize(name, run_context=nil)
         super
@@ -31,6 +32,7 @@ class Chef
         @group_name = name
         @gid = nil
         @members = []
+        @excluded_members = []
         @action = :create
         @append = false
         @non_unique = false
diff --git a/spec/unit/provider/group/dscl_spec.rb b/spec/unit/provider/group/dscl_spec.rb
index 28d78cbdbd..360a799b5f 100644
--- a/spec/unit/provider/group/dscl_spec.rb
+++ b/spec/unit/provider/group/dscl_spec.rb
@@ -265,6 +265,7 @@ describe Chef::Provider::Group::Dscl do
     it "should manage the group_name if it changed and the new resources group_name is not null" do
       @current_resource.group_name("oldval")
       @new_resource.group_name("newname")
+      @provider.should_receive(:set_members).and_return(true)
       @provider.should_receive(:safe_dscl).with("create /Groups/newname")
       @provider.should_receive(:safe_dscl).with("create /Groups/newname Password '*'")
       @provider.manage_group
diff --git a/spec/unit/provider/group/gpasswd_spec.rb b/spec/unit/provider/group/gpasswd_spec.rb
index 8889ba322e..f765667300 100644
--- a/spec/unit/provider/group/gpasswd_spec.rb
+++ b/spec/unit/provider/group/gpasswd_spec.rb
@@ -76,8 +76,7 @@ describe Chef::Provider::Group::Gpasswd, "modify_group_members" do
         @new_resource.members([])
       end
 
-      it "logs a message and does not modify group membership" do
-        Chef::Log.should_receive(:debug).with("group[wheel] not changing group members, the group has no members to add")
+      it "does not modify group membership" do
         @provider.should_not_receive(:shell_out!)
         @provider.modify_group_members
       end
@@ -85,7 +84,7 @@ describe Chef::Provider::Group::Gpasswd, "modify_group_members" do
 
     describe "when the resource specifies group members" do
       it "should log an appropriate debug message" do
-        Chef::Log.should_receive(:debug).with("group[wheel] setting group members to lobster, rage, fist")
+        Chef::Log.should_receive(:debug).with("group[wheel] setting group members to: lobster, rage, fist")
         @provider.stub!(:shell_out!)
         @provider.modify_group_members
       end
@@ -95,12 +94,20 @@ describe Chef::Provider::Group::Gpasswd, "modify_group_members" do
         @provider.modify_group_members
       end
 
-      it "should run gpasswd individually for each user when the append option is set" do
-        @new_resource.append(true)
-        @provider.should_receive(:shell_out!).with("gpasswd -a lobster wheel")
-        @provider.should_receive(:shell_out!).with("gpasswd -a rage wheel")
-        @provider.should_receive(:shell_out!).with("gpasswd -a fist wheel")
-        @provider.modify_group_members
+      describe "when no user exists in the system" do
+        before do
+          current_resource = @new_resource.dup
+          current_resource.members([ ])
+          @provider.current_resource = current_resource
+        end
+
+        it "should run gpasswd individually for each user when the append option is set" do
+          @new_resource.append(true)
+          @provider.should_receive(:shell_out!).with("gpasswd -a lobster wheel")
+          @provider.should_receive(:shell_out!).with("gpasswd -a rage wheel")
+          @provider.should_receive(:shell_out!).with("gpasswd -a fist wheel")
+          @provider.modify_group_members
+        end
       end
 
     end
diff --git a/spec/unit/provider/group/groupadd_spec.rb b/spec/unit/provider/group/groupadd_spec.rb
index 0cc1167142..91073727c8 100644
--- a/spec/unit/provider/group/groupadd_spec.rb
+++ b/spec/unit/provider/group/groupadd_spec.rb
@@ -143,10 +143,9 @@ describe Chef::Provider::Group::Groupadd do
     end
   end
 
-  describe "modify_group_members" do
-
-    it "should raise an error when calling modify_group_members" do
-      lambda { @provider.modify_group_members }.should raise_error(Chef::Exceptions::Group, "you must override modify_group_members in #{@provider.to_s}")
+  [:add_member, :remove_member, :set_members].each do |m|
+    it "should raise an error when calling #{m}" do
+      lambda { @provider.send(m, [ ]) }.should raise_error(Chef::Exceptions::Group, "you must override #{m} in #{@provider.to_s}")
     end
   end
 
diff --git a/spec/unit/provider/group/usermod_spec.rb b/spec/unit/provider/group/usermod_spec.rb
index 7f2931f2e2..1687cc0b65 100644
--- a/spec/unit/provider/group/usermod_spec.rb
+++ b/spec/unit/provider/group/usermod_spec.rb
@@ -25,6 +25,7 @@ describe Chef::Provider::Group::Usermod do
     @run_context = Chef::RunContext.new(@node, {}, @events)
     @new_resource = Chef::Resource::Group.new("wheel")
     @new_resource.members [ "all", "your", "base" ]
+    @new_resource.excluded_members [ ]
     @provider = Chef::Provider::Group::Usermod.new(@new_resource, @run_context)
     @provider.stub!(:run_command)
   end
@@ -33,11 +34,12 @@ describe Chef::Provider::Group::Usermod do
 
     describe "with an empty members array" do
       before do
+        @new_resource.stub!(:append).and_return(true)
         @new_resource.stub!(:members).and_return([])
       end
 
       it "should log an appropriate message" do
-        Chef::Log.should_receive(:debug).with("group[wheel] not changing group members, the group has no members")
+        @provider.should_not_receive(:shell_out!)
         @provider.modify_group_members
       end
     end
@@ -65,13 +67,26 @@ describe Chef::Provider::Group::Usermod do
         lambda { @provider.run_action(@provider.process_resource_requirements) }.should raise_error(Chef::Exceptions::Group, "setting group members directly is not supported by #{@provider.to_s}, must set append true in group")
       end
 
+      it "should raise an error when excluded_members are set" do
+        @provider.define_resource_requirements
+        @provider.load_current_resource
+        @provider.instance_variable_set("@group_exists", true)
+        @provider.action = :modify
+        @new_resource.stub!(:append).and_return(true)
+        @new_resource.stub!(:excluded_members).and_return(["someone"])
+        lambda { @provider.run_action(@provider.process_resource_requirements) }.should raise_error(Chef::Exceptions::Group, "excluded_members is not supported by #{@provider.to_s}")
+      end
+
       platforms.each do |platform, flags|
         it "should usermod each user when the append option is set on #{platform}" do
+          current_resource = @new_resource.dup
+          current_resource.members([ ])
+          @provider.current_resource = current_resource
           @node.automatic_attrs[:platform] = platform
           @new_resource.stub!(:append).and_return(true)
-          @provider.should_receive(:run_command).with({:command => "usermod #{flags} wheel all"})
-          @provider.should_receive(:run_command).with({:command => "usermod #{flags} wheel your"})
-          @provider.should_receive(:run_command).with({:command => "usermod #{flags} wheel base"})
+          @provider.should_receive(:shell_out!).with("usermod #{flags} wheel all")
+          @provider.should_receive(:shell_out!).with("usermod #{flags} wheel your")
+          @provider.should_receive(:shell_out!).with("usermod #{flags} wheel base")
           @provider.modify_group_members
         end
       end
diff --git a/spec/unit/provider/group/windows_spec.rb b/spec/unit/provider/group/windows_spec.rb
index a65cfc241d..14a4f4bb40 100644
--- a/spec/unit/provider/group/windows_spec.rb
+++ b/spec/unit/provider/group/windows_spec.rb
@@ -70,14 +70,6 @@ describe Chef::Provider::Group::Windows do
       @provider.manage_group
     end
 
-    it "should call @net_group.local_set_members if append fails" do
-      @new_resource.stub!(:append).and_return(true)
-      @net_group.stub!(:local_add_members).and_raise(ArgumentError)
-      @net_group.should_receive(:local_add_members).with(@new_resource.members)
-      @net_group.should_receive(:local_set_members).with(@new_resource.members + @current_resource.members)
-      @provider.manage_group
-    end
-
   end
 
   describe "remove_group" do
diff --git a/spec/unit/provider/group_spec.rb b/spec/unit/provider/group_spec.rb
index caad9878b6..a076593511 100644
--- a/spec/unit/provider/group_spec.rb
+++ b/spec/unit/provider/group_spec.rb
@@ -108,6 +108,19 @@ describe Chef::Provider::User do
       @provider.compare_group.should be_true
     end
 
+    it "should return false if append is true and excluded_members include a non existing member" do
+      @new_resource.excluded_members << "extra_user"
+      @new_resource.stub!(:append).and_return(true)
+      @provider.compare_group.should be_false
+    end
+
+    it "should return true if the append is true and excluded_members include an existing user" do
+      @new_resource.members.each {|m| @new_resource.excluded_members << m }
+      @new_resource.members.clear
+      @new_resource.stub!(:append).and_return(true)
+      @provider.compare_group.should be_true
+    end
+
   end
 
   describe "when creating a group" do
@@ -127,6 +140,7 @@ describe Chef::Provider::User do
     it "should check to see if the group has mismatched attributes if the group exists" do
       @provider.group_exists = true
       @provider.stub!(:compare_group).and_return(false)
+      @provider.stub!(:change_desc).and_return([ ])
       @provider.run_action(:create)
       @provider.new_resource.should_not be_updated
     end
@@ -134,6 +148,7 @@ describe Chef::Provider::User do
     it "should call manage_group if the group exists and has mismatched attributes" do
       @provider.group_exists = true
       @provider.stub!(:compare_group).and_return(true)
+      @provider.stub!(:change_desc).and_return([ ])
       @provider.should_receive(:manage_group).and_return(true)
       @provider.run_action(:create)
     end
@@ -141,6 +156,7 @@ describe Chef::Provider::User do
     it "should set the new_resources updated flag when it creates the group if we call manage_group" do
       @provider.group_exists = true
       @provider.stub!(:compare_group).and_return(true)
+      @provider.stub!(:change_desc).and_return(["Some changes are going to be done."])
       @provider.stub!(:manage_group).and_return(true)
       @provider.run_action(:create)
       @new_resource.should be_updated
@@ -172,12 +188,14 @@ describe Chef::Provider::User do
 
     it "should run manage_group if the group exists and has mismatched attributes" do
       @provider.should_receive(:compare_group).and_return(true)
+      @provider.stub!(:change_desc).and_return(["Some changes are going to be done."])
       @provider.should_receive(:manage_group).and_return(true)
       @provider.run_action(:manage)
     end
 
     it "should set the new resources updated flag to true if manage_group is called" do
       @provider.stub!(:compare_group).and_return(true)
+      @provider.stub!(:change_desc).and_return(["Some changes are going to be done."])
       @provider.stub!(:manage_group).and_return(true)
       @provider.run_action(:manage)
       @new_resource.should be_updated
@@ -191,6 +209,7 @@ describe Chef::Provider::User do
 
     it "should not run manage_group if the group exists but has no differing attributes" do
       @provider.should_receive(:compare_group).and_return(false)
+      @provider.stub!(:change_desc).and_return(["Some changes are going to be done."])
       @provider.should_not_receive(:manage_group)
       @provider.run_action(:manage)
     end
@@ -204,12 +223,14 @@ describe Chef::Provider::User do
 
     it "should run manage_group if the group exists and has mismatched attributes" do
       @provider.should_receive(:compare_group).and_return(true)
+      @provider.stub!(:change_desc).and_return(["Some changes are going to be done."])
       @provider.should_receive(:manage_group).and_return(true)
       @provider.run_action(:modify)
     end
 
     it "should set the new resources updated flag to true if manage_group is called" do
       @provider.stub!(:compare_group).and_return(true)
+      @provider.stub!(:change_desc).and_return(["Some changes are going to be done."])
       @provider.stub!(:manage_group).and_return(true)
       @provider.run_action(:modify)
       @new_resource.should be_updated
@@ -217,6 +238,7 @@ describe Chef::Provider::User do
 
     it "should not run manage_group if the group exists but has no differing attributes" do
       @provider.should_receive(:compare_group).and_return(false)
+      @provider.stub!(:change_desc).and_return(["Some changes are going to be done."])
       @provider.should_not_receive(:manage_group)
       @provider.run_action(:modify)
     end
@@ -233,26 +255,26 @@ describe Chef::Provider::User do
        @new_resource.members << "user2"
        @new_resource.stub!(:append).and_return true
        @provider.compare_group.should be_true
-       @provider.change_desc.should == "add missing member(s): user1, user2"
+       @provider.change_desc.should == [ "add missing member(s): user1, user2" ]
     end
 
     it "should report that the group members will be overwritten if not appending" do
        @new_resource.members << "user1"
        @new_resource.stub!(:append).and_return false
        @provider.compare_group.should be_true
-       @provider.change_desc.should == "replace group members with new list of members"
+       @provider.change_desc.should == [ "replace group members with new list of members" ]
     end
 
     it "should report the gid will be changed when it does not match" do
       @current_resource.stub!(:gid).and_return("BADF00D")
       @provider.compare_group.should be_true
-      @provider.change_desc.should == "change gid #{@current_resource.gid} to #{@new_resource.gid}"
+      @provider.change_desc.should == [ "change gid #{@current_resource.gid} to #{@new_resource.gid}" ]
 
     end
 
     it "should report no change reason when no change is required" do
       @provider.compare_group.should be_false
-      @provider.change_desc.should == nil
+      @provider.change_desc.should == [ ]
     end
   end