Merge pull request #5666 from chef/lcg/deprecate-run-command

formally deprecate run_command

5 files changed, 70 insertions, 52 deletions

diff --git a/lib/chef/deprecated.rb b/lib/chef/deprecated.rb
index 3a988fdfa3..5b0bac552e 100644
--- a/lib/chef/deprecated.rb
+++ b/lib/chef/deprecated.rb
@@ -166,6 +166,16 @@ class Chef
       end
     end
 
+    class RunCommand < Base
+      def id
+        14
+      end
+
+      def target
+        "run_command.html"
+      end
+    end
+
     class ResourceCloning < Base
       def id
         3694
diff --git a/lib/chef/mixin/command.rb b/lib/chef/mixin/command.rb
index 0cc3143ec7..269f1d1194 100644
--- a/lib/chef/mixin/command.rb
+++ b/lib/chef/mixin/command.rb
@@ -107,6 +107,7 @@ class Chef
       end
 
       def output_of_command(command, args)
+        Chef.deprecated(:run_command, "Chef::Mixin::Command.run_command is deprecated, please use shell_out")
         Chef::Log.debug("Executing #{command}")
         stderr_string, stdout_string, status = "", "", nil
 
diff --git a/lib/chef/mixin/shell_out.rb b/lib/chef/mixin/shell_out.rb
index cc2bf085e9..d341e3d1ec 100644
--- a/lib/chef/mixin/shell_out.rb
+++ b/lib/chef/mixin/shell_out.rb
@@ -85,6 +85,7 @@ class Chef
       # @param args [String] variable number of string arguments
       # @return [String] nicely concatenated string or empty string
       def a_to_s(*args)
+        # FIXME: this should be cleaned up and deprecated
         clean_array(*args).join(" ")
       end
 
@@ -108,6 +109,14 @@ class Chef
         args.flatten.reject { |i| i.nil? || i == "" }.map(&:to_s)
       end
 
+      def shell_out_compact(*args)
+        shell_out(*clean_array(*args))
+      end
+
+      def shell_out_compact!(*args)
+        shell_out!(*clean_array(*args))
+      end
+
       private
 
       def shell_out_command(*command_args)
diff --git a/lib/chef/provider/group/groupadd.rb b/lib/chef/provider/group/groupadd.rb
index 162875f9f1..e94ed93cbd 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
-          run_command(:command => command)
+          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
-          run_command(:command => command)
+          shell_out_compact!("groupmod", set_options)
           modify_group_members
         end
 
         # Remove the group
         def remove_group
-          run_command(:command => "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..70a98ff2f5 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