fix groupadd specs and modernize

Signed-off-by: Lamont Granquist <lamont@scriptkiddie.org>

2 files changed, 48 insertions, 50 deletions

diff --git a/lib/chef/provider/group/groupadd.rb b/lib/chef/provider/group/groupadd.rb
index d7a41fb4a7..53eb3ec626 100644
--- a/lib/chef/provider/group/groupadd.rb
+++ b/lib/chef/provider/group/groupadd.rb
@@ -36,7 +36,7 @@ class Chef
           required_binaries.each do |required_binary|
             requirements.assert(:all_actions) do |a|
               a.assertion { ::File.exists?(required_binary) }
-              a.failure_message Chef::Exceptions::Group, "Could not find binary #{required_binary} for #{@new_resource}"
+              a.failure_message Chef::Exceptions::Group, "Could not find binary #{required_binary} for #{new_resource}"
               # No whyrun alternative: this component should be available in the base install of any given system that uses it
             end
           end
@@ -44,54 +44,49 @@ class Chef
 
         # Create the group
         def create_group
-          command = "groupadd"
-          command << set_options
-          command << groupadd_options
           shell_out_compact!("groupadd", set_options, groupadd_options)
           modify_group_members
         end
 
         # Manage the group when it already exists
         def manage_group
-          command = "groupmod"
-          command << set_options
           shell_out_compact!("groupmod", set_options)
           modify_group_members
         end
 
         # Remove the group
         def remove_group
-          shell_out!("groupdel", new_resource.group_name)
+          shell_out_compact!("groupdel", new_resource.group_name)
         end
 
         def modify_group_members
-          if @new_resource.append
-            if @new_resource.members && !@new_resource.members.empty?
+          if new_resource.append
+            if new_resource.members && !new_resource.members.empty?
               members_to_be_added = [ ]
-              @new_resource.members.each do |member|
-                members_to_be_added << member if !@current_resource.members.include?(member)
+              new_resource.members.each do |member|
+                members_to_be_added << member if !current_resource.members.include?(member)
               end
               members_to_be_added.each do |member|
-                Chef::Log.debug("#{@new_resource} appending member #{member} to group #{@new_resource.group_name}")
+                Chef::Log.debug("#{new_resource} appending member #{member} to group #{new_resource.group_name}")
                 add_member(member)
               end
             end
 
-            if @new_resource.excluded_members && !@new_resource.excluded_members.empty?
+            if new_resource.excluded_members && !new_resource.excluded_members.empty?
               members_to_be_removed = [ ]
-              @new_resource.excluded_members.each do |member|
-                members_to_be_removed << member if @current_resource.members.include?(member)
+              new_resource.excluded_members.each do |member|
+                members_to_be_removed << member if current_resource.members.include?(member)
               end
 
               members_to_be_removed.each do |member|
-                Chef::Log.debug("#{@new_resource} removing member #{member} from group #{@new_resource.group_name}")
+                Chef::Log.debug("#{new_resource} removing member #{member} from group #{new_resource.group_name}")
                 remove_member(member)
               end
             end
           else
-            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)
+            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
 
@@ -112,23 +107,25 @@ class Chef
         # ==== Returns
         # <string>:: A string containing the option and then the quoted value
         def set_options
-          opts = ""
+          opts = []
           { :gid => "-g" }.sort { |a, b| a[0] <=> b[0] }.each do |field, option|
-            if @current_resource.send(field) != @new_resource.send(field)
-              if @new_resource.send(field)
-                opts << " #{option} '#{@new_resource.send(field)}'"
-                Chef::Log.debug("#{@new_resource} set #{field} to #{@new_resource.send(field)}")
+            if current_resource.send(field) != new_resource.send(field)
+              if new_resource.send(field)
+                opts << option
+                opts << "'#{new_resource.send(field)}'"
+                Chef::Log.debug("#{new_resource} set #{field} to #{new_resource.send(field)}")
               end
             end
           end
-          opts << " #{@new_resource.group_name}"
+          opts << new_resource.group_name
+          opts
         end
 
         def groupadd_options
-          opts = ""
+          opts = []
           # Solaris doesn't support system groups.
-          opts << " -r" if @new_resource.system && !node.platform?("solaris2")
-          opts << " -o" if @new_resource.non_unique
+          opts << "-r" if new_resource.system && !node.platform?("solaris2")
+          opts << "-o" if new_resource.non_unique
           opts
         end
 
diff --git a/spec/unit/provider/group/groupadd_spec.rb b/spec/unit/provider/group/groupadd_spec.rb
index 054d953414..2afb35a03f 100644
--- a/spec/unit/provider/group/groupadd_spec.rb
+++ b/spec/unit/provider/group/groupadd_spec.rb
@@ -58,36 +58,37 @@ describe Chef::Provider::Group::Groupadd do
 
       it "should set the option for #{attribute} if the new resources #{attribute} is not null" do
         allow(new_resource).to receive(attribute).and_return("wowaweea")
-        expect(provider.set_options).to eql(" #{option} '#{new_resource.send(attribute)}' #{new_resource.group_name}")
+        expect(provider.set_options).to eql([ option, "'#{new_resource.send(attribute)}'", new_resource.group_name])
       end
     end
 
     it "should combine all the possible options" do
-      match_string = ""
+      match_array = []
       field_list.sort { |a, b| a[0] <=> b[0] }.each do |attribute, option|
         allow(new_resource).to receive(attribute).and_return("hola")
-        match_string << " #{option} 'hola'"
+        match_array << option
+        match_array << "'hola'"
       end
-      match_string << " aj"
-      expect(provider.set_options).to eql(match_string)
+      match_array << "aj"
+      expect(provider.set_options).to eql(match_array)
     end
 
     describe "when we want to create a system group" do
       it "should not set groupadd_options '-r' when system is false" do
         new_resource.system(false)
-        expect(provider.groupadd_options).not_to match(/-r/)
+        expect(provider.groupadd_options).to eq([])
       end
 
       it "should set groupadd -r if system is true" do
         new_resource.system(true)
-        expect(provider.groupadd_options).to eq(" -r")
+        expect(provider.groupadd_options).to eq(["-r"])
       end
 
       context "on Solaris" do
         before { node.automatic["platform"] = "solaris2" }
         it "should not set groupadd -r if system is true" do
           new_resource.system(true)
-          expect(provider.groupadd_options).not_to match(/-r/)
+          expect(provider.groupadd_options).to eql([])
         end
       end
     end
@@ -95,26 +96,26 @@ describe Chef::Provider::Group::Groupadd do
     describe "when we want to create a non_unique gid group" do
       it "should not set groupadd_options '-o' when non_unique is false" do
         new_resource.non_unique(false)
-        expect(provider.groupadd_options).not_to match(/-o/)
+        expect(provider.groupadd_options).to eq([])
       end
 
       it "should set groupadd -o if non_unique is true" do
         new_resource.non_unique(true)
-        expect(provider.groupadd_options).to eq(" -o")
+        expect(provider.groupadd_options).to eq(["-o"])
       end
     end
   end
 
   describe "#create_group" do
     before do
-      allow(provider).to receive(:run_command).and_return(true)
-      allow(provider).to receive(:set_options).and_return(" monkey")
-      allow(provider).to receive(:groupadd_options).and_return("")
+      allow(provider).to receive(:shell_out!).and_return(true)
+      allow(provider).to receive(:set_options).and_return("monkey")
+      allow(provider).to receive(:groupadd_options).and_return([])
       allow(provider).to receive(:modify_group_members).and_return(true)
     end
 
     it "should run groupadd with the return of set_options" do
-      expect(provider).to receive(:run_command).with({ :command => "groupadd monkey" }).and_return(true)
+      expect(provider).to receive(:shell_out!).with("groupadd", "monkey").and_return(true)
       provider.create_group
     end
 
@@ -126,13 +127,13 @@ describe Chef::Provider::Group::Groupadd do
 
   describe "#manage_group" do
     before do
-      allow(provider).to receive(:run_command).and_return(true)
-      allow(provider).to receive(:set_options).and_return(" monkey")
+      allow(provider).to receive(:shell_out!).and_return(true)
+      allow(provider).to receive(:set_options).and_return("monkey")
     end
 
     it "should run groupmod with the return of set_options" do
       allow(provider).to receive(:modify_group_members).and_return(true)
-      expect(provider).to receive(:run_command).with({ :command => "groupmod monkey" }).and_return(true)
+      expect(provider).to receive(:shell_out!).with("groupmod", "monkey").and_return(true)
       provider.manage_group
     end
 
@@ -144,12 +145,12 @@ describe Chef::Provider::Group::Groupadd do
 
   describe "#remove_group" do
     before do
-      allow(provider).to receive(:run_command).and_return(true)
-      allow(provider).to receive(:set_options).and_return(" monkey")
+      allow(provider).to receive(:shell_out!).and_return(true)
+      allow(provider).to receive(:set_options).and_return("monkey")
     end
 
     it "should run groupdel with the new resources group name" do
-      expect(provider).to receive(:run_command).with({ :command => "groupdel aj" }).and_return(true)
+      expect(provider).to receive(:shell_out!).with("groupdel", "aj").and_return(true)
       provider.remove_group
     end
   end
@@ -162,8 +163,8 @@ describe Chef::Provider::Group::Groupadd do
 
   describe "#load_current_resource" do
     before do
-      allow(provider).to receive(:run_command).and_return(true)
-      allow(provider).to receive(:set_options).and_return(" monkey")
+      allow(provider).to receive(:shell_out!).and_return(true)
+      allow(provider).to receive(:set_options).and_return("monkey")
     end
 
     before do