remove rm -rf in chef solo recipe_url

- deprecates '-r' used for the recipe_url in chef-solo
- adds --delete-entire-chef-repo option for users who want the
  old behavior back.
- cleans up some old code

closes #3802
closes #1515
closes #1751

4 files changed, 69 insertions, 61 deletions

diff --git a/lib/chef/application/client.rb b/lib/chef/application/client.rb
index 1f232c651d..1a7d8d046d 100644
--- a/lib/chef/application/client.rb
+++ b/lib/chef/application/client.rb
@@ -25,6 +25,7 @@ require "chef/log"
 require "chef/config_fetcher"
 require "chef/handler/error_report"
 require "chef/workstation_config_loader"
+require "chef/mixin/shell_out"
 
 class Chef::Application::Client < Chef::Application
   include Chef::Mixin::ShellOut
@@ -279,6 +280,11 @@ class Chef::Application::Client < Chef::Application
     :description  => "Enable fips mode",
     :boolean      => true
 
+  option :delete_entire_chef_repo,
+    :long           => "--delete-entire-chef-repo",
+    :description    => "DANGEROUS: does what it says, only useful with --recipe-url",
+    :boolean        => true
+
   IMMEDIATE_RUN_SIGNAL = "1".freeze
 
   attr_reader :chef_client_json
@@ -307,15 +313,21 @@ class Chef::Application::Client < Chef::Application
       Chef::Config.chef_repo_path = Chef::Config.find_chef_repo_path(Dir.pwd)
     end
 
-    if !Chef::Config.local_mode && Chef::Config.has_key?(:recipe_url)
-      Chef::Application.fatal!("chef-client recipe-url can be used only in local-mode", 1)
-    elsif Chef::Config.local_mode && Chef::Config.has_key?(:recipe_url)
-      Chef::Log.debug "Creating path #{Chef::Config.chef_repo_path} to extract recipes into"
-      FileUtils.mkdir_p(Chef::Config.chef_repo_path)
-      tarball_path = File.join(Chef::Config.chef_repo_path, "recipes.tgz")
-      fetch_recipe_tarball(Chef::Config[:recipe_url], tarball_path)
-      result = shell_out!("tar zxvf #{tarball_path} -C #{Chef::Config.chef_repo_path}")
-      Chef::Log.debug "#{result.stdout}"
+    if Chef::Config[:recipe_url]
+      if !Chef::Config.local_mode
+        Chef::Application.fatal!("chef-client recipe-url can be used only in local-mode", 1)
+      else
+        if Chef::Config[:delete_entire_chef_repo]
+          Chef::Log.debug "Cleanup path #{Chef::Config.chef_repo_path} before extract recipes into it"
+          FileUtils.rm_rf(recipes_path, :secure => true)
+        end
+        Chef::Log.debug "Creating path #{Chef::Config.chef_repo_path} to extract recipes into"
+        FileUtils.mkdir_p(Chef::Config.chef_repo_path)
+        tarball_path = File.join(Chef::Config.chef_repo_path, "recipes.tgz")
+        fetch_recipe_tarball(Chef::Config[:recipe_url], tarball_path)
+        result = shell_out!("tar zxvf #{tarball_path} -C #{Chef::Config.chef_repo_path}")
+        Chef::Log.debug "#{result.stdout}"
+      end
     end
 
     Chef::Config.chef_zero.host = config[:chef_zero_host] if config[:chef_zero_host]
diff --git a/lib/chef/application/solo.rb b/lib/chef/application/solo.rb
index 26bd6ba52e..c5d7549ffe 100644
--- a/lib/chef/application/solo.rb
+++ b/lib/chef/application/solo.rb
@@ -25,8 +25,10 @@ require "chef/log"
 require "chef/rest"
 require "chef/config_fetcher"
 require "fileutils"
+require "chef/mixin/shell_out"
 
 class Chef::Application::Solo < Chef::Application
+  include Chef::Mixin::ShellOut
 
   option :config_file,
     :short => "-c CONFIG",
@@ -135,10 +137,9 @@ class Chef::Application::Solo < Chef::Application
     :proc => lambda { |s| s.to_i }
 
   option :recipe_url,
-      :short => "-r RECIPE_URL",
-      :long => "--recipe-url RECIPE_URL",
-      :description => "Pull down a remote gzipped tarball of recipes and untar it to the cookbook cache.",
-      :proc => nil
+    :short        => "-r RECIPE_URL",
+    :long         => "--recipe-url RECIPE_URL",
+    :description  => "Pull down a remote gzipped tarball of recipes and untar it to the cookbook cache."
 
   option :version,
     :short        => "-v",
@@ -191,6 +192,11 @@ class Chef::Application::Solo < Chef::Application
     :description    => "Only run the bare minimum ohai plugins chef needs to function",
     :boolean        => true
 
+  option :delete_entire_chef_repo,
+    :long           => "--delete-entire-chef-repo",
+    :description    => "DANGEROUS: does what it says, only useful with --recipe-url",
+    :boolean        => true
+
   attr_reader :chef_client_json
 
   def initialize
@@ -210,17 +216,22 @@ class Chef::Application::Solo < Chef::Application
 
     Chef::Application.fatal!(unforked_interval_error_message) if !Chef::Config[:client_fork] && Chef::Config[:interval]
 
+    Chef::Log.deprecation("-r MUST be changed to --recipe-url, the -r option will be changed in Chef 13.0") if ARGV.include?("-r")
+
     if Chef::Config[:recipe_url]
-      cookbooks_path = Array(Chef::Config[:cookbook_path]).detect { |e| e =~ /\/cookbooks\/*$/ }
+      cookbooks_path = Array(Chef::Config[:cookbook_path]).detect{|e| e =~ /#{Chef::Config.platform_path_separator_escaped}cookbooks#{Chef::Config.platform_path_separator_escaped}*$/ }
       recipes_path = File.expand_path(File.join(cookbooks_path, ".."))
 
-      Chef::Log.debug "Cleanup path #{recipes_path} before extract recipes into it"
-      FileUtils.rm_rf(recipes_path, :secure => true)
+      if Chef::Config[:delete_entire_chef_repo]
+        Chef::Log.debug "Cleanup path #{recipes_path} before extract recipes into it"
+        FileUtils.rm_rf(recipes_path, :secure => true)
+      end
       Chef::Log.debug "Creating path #{recipes_path} to extract recipes into"
       FileUtils.mkdir_p(recipes_path)
       tarball_path = File.join(recipes_path, "recipes.tgz")
       fetch_recipe_tarball(Chef::Config[:recipe_url], tarball_path)
-      Mixlib::ShellOut.new("tar zxvf #{tarball_path} -C #{recipes_path}").run_command
+      result = shell_out!("tar zxvf #{tarball_path} -C #{recipes_path}")
+      Chef::Log.debug "#{result.stdout}"
     end
 
     # json_attribs shuld be fetched after recipe_url tarball is unpacked.
diff --git a/spec/unit/application/client_spec.rb b/spec/unit/application/client_spec.rb
index b675c9384a..ff6f460c13 100644
--- a/spec/unit/application/client_spec.rb
+++ b/spec/unit/application/client_spec.rb
@@ -36,6 +36,10 @@ describe Chef::Application::Client, "reconfigure" do
     Chef::Config[:interval] = 10
 
     Chef::Config[:once] = false
+
+    # protect the unit tests against accidental --delete-entire-chef-repo from firing
+    # for real during tests.  DO NOT delete this line.
+    expect(FileUtils).not_to receive(:rm_rf)
   end
 
   after do
diff --git a/spec/unit/application/solo_spec.rb b/spec/unit/application/solo_spec.rb
index e2de4aa7b1..4361a2cd33 100644
--- a/spec/unit/application/solo_spec.rb
+++ b/spec/unit/application/solo_spec.rb
@@ -28,9 +28,12 @@ describe Chef::Application::Solo do
     allow(app).to receive(:configure_logging).and_return(true)
     allow(app).to receive(:trap)
 
-    Chef::Config[:recipe_url] = false
     Chef::Config[:json_attribs] = false
     Chef::Config[:solo] = true
+
+    # protect the unit tests against accidental --delete-entire-chef-repo from firing
+    # for real during tests.  DO NOT delete this line.
+    expect(FileUtils).not_to receive(:rm_rf)
   end
 
   describe "configuring the application" do
@@ -103,61 +106,39 @@ Enable chef-client interval runs by setting `:client_fork = true` in your config
       end
     end
 
-    describe "when the recipe_url configuration option is specified" do
-      let(:tarfile) { StringIO.new("remote_tarball_content") }
-      let(:target_file) { StringIO.new }
-      let(:shellout) { double(run_command: nil, error!: nil, stdout: "") }
+    it "downloads a tarball when the recipe_url configuration option is specified" do
+      Chef::Config[:cookbook_path] = "#{Dir.tmpdir}/chef-solo/cookbooks"
+      Chef::Config[:recipe_url] = "http://junglist.gen.nz/recipes.tgz"
 
-      before do
-        Chef::Config[:cookbook_path] = "#{Dir.tmpdir}/chef-solo/cookbooks"
-        Chef::Config[:recipe_url] = "http://junglist.gen.nz/recipes.tgz"
+      expect(FileUtils).to receive(:mkdir_p).with("#{Dir.tmpdir}/chef-solo").and_return(true)
 
-        allow(FileUtils).to receive(:rm_rf).and_return(true)
-        allow(FileUtils).to receive(:mkdir_p).and_return(true)
+      tarfile = StringIO.new("remote_tarball_content")
+      target_file = StringIO.new
 
-        allow(app).to receive(:open).with("http://junglist.gen.nz/recipes.tgz").and_yield(tarfile)
-        allow(File).to receive(:open).with("#{Dir.tmpdir}/chef-solo/recipes.tgz", "wb").and_yield(target_file)
+      expect(app).to receive(:open).with("http://junglist.gen.nz/recipes.tgz").and_yield(tarfile)
+      expect(File).to receive(:open).with("#{Dir.tmpdir}/chef-solo/recipes.tgz", "wb").and_yield(target_file)
 
-        allow(Mixlib::ShellOut).to receive(:new).and_return(shellout)
-      end
+      shellout = instance_double("Mixlib::ShellOut", run_command: nil, error!: nil, stdout: "")
 
-      it "should create the recipes path based on the parent of the cookbook path" do
-        expect(FileUtils).to receive(:mkdir_p).with("#{Dir.tmpdir}/chef-solo").and_return(true)
-        app.reconfigure
-      end
-
-      it "should download the recipes" do
-        expect(app).to receive(:open).with("http://junglist.gen.nz/recipes.tgz").and_yield(tarfile)
-        app.reconfigure
-      end
-
-      it "should write the recipes to the target path" do
-        app.reconfigure
-        expect(target_file.string).to eq("remote_tarball_content")
-      end
-
-      it "should untar the target file to the parent of the cookbook path" do
-        expect(Mixlib::ShellOut).to receive(:new).with("tar zxvf #{Dir.tmpdir}/chef-solo/recipes.tgz -C #{Dir.tmpdir}/chef-solo")
-        app.reconfigure
-      end
+      expect(app).to receive(:shell_out!).with("tar zxvf #{Dir.tmpdir}/chef-solo/recipes.tgz -C #{Dir.tmpdir}/chef-solo").and_return(shellout)
+      app.reconfigure
+      expect(target_file.string).to eq("remote_tarball_content")
     end
-  end
 
-  describe "when the json_attribs and recipe_url configuration options are both specified" do
-    let(:json_attribs) { { "a" => "b" } }
-    let(:config_fetcher) { double(Chef::ConfigFetcher, :fetch_json => json_attribs) }
-    let(:json_source) { "https://foo.com/foo.json" }
+    it "fetches the recipe_url first when both json_attribs and recipe_url are specified" do
+      json_attribs = { "a" => "b" }
+      config_fetcher = instance_double("Chef::ConfigFetcher", :fetch_json => json_attribs)
 
-    before do
-      Chef::Config[:json_attribs] = json_source
+      Chef::Config[:json_attribs] = "https://foo.com/foo.json"
       Chef::Config[:recipe_url] = "http://icanhas.cheezburger.com/lolcats"
       Chef::Config[:cookbook_path] = "#{Dir.tmpdir}/chef-solo/cookbooks"
-      allow(FileUtils).to receive(:rm_rf).and_return(true)
-      allow(FileUtils).to receive(:mkdir_p).and_return(true)
+      expect(FileUtils).to receive(:mkdir_p).with("#{Dir.tmpdir}/chef-solo").and_return(true)
+
       allow(Chef::Mixin::Command).to receive(:run_command).and_return(true)
-    end
 
-    it "should fetch the recipe_url first" do
+      shellout = instance_double("Mixlib::ShellOut", run_command: nil, error!: nil, stdout: "")
+
+      expect(app).to receive(:shell_out!).with("tar zxvf #{Dir.tmpdir}/chef-solo/recipes.tgz -C #{Dir.tmpdir}/chef-solo").and_return(shellout)
       expect(app).to receive(:fetch_recipe_tarball).ordered
       expect(Chef::ConfigFetcher).to receive(:new).ordered.and_return(config_fetcher)
       app.reconfigure