Merge pull request #4651 from chef/tball/rehash_error

Always rehash from gem source and not existing hash file

8 files changed, 105 insertions, 4 deletions

diff --git a/lib/chef/knife.rb b/lib/chef/knife.rb
index 657216bcf0..8f7232d927 100644
--- a/lib/chef/knife.rb
+++ b/lib/chef/knife.rb
@@ -145,6 +145,11 @@ class Chef
     end
 
     def self.subcommand_class_from(args)
+      if args.size == 1 and args[0].strip.downcase == "rehash"
+        # To prevent issues with the rehash file not pointing to the correct plugins,
+        # we always use the glob loader when regenerating the rehash file
+        @subcommand_loader = Chef::Knife::SubcommandLoader.gem_glob_loader(chef_config_dir)
+      end
       subcommand_loader.command_class_from(args) || subcommand_not_found!(args)
     end
 
diff --git a/lib/chef/knife/core/hashed_command_loader.rb b/lib/chef/knife/core/hashed_command_loader.rb
index 7b6c1c4c08..1165af6eb2 100644
--- a/lib/chef/knife/core/hashed_command_loader.rb
+++ b/lib/chef/knife/core/hashed_command_loader.rb
@@ -40,9 +40,29 @@ class Chef
 
         def list_commands(pref_category = nil)
           if pref_category || manifest[KEY]["plugins_by_category"].key?(pref_category)
-            { pref_category => manifest[KEY]["plugins_by_category"][pref_category] }
+            commands = { pref_category => manifest[KEY]["plugins_by_category"][pref_category] }
           else
-            manifest[KEY]["plugins_by_category"]
+            commands = manifest[KEY]["plugins_by_category"]
+          end
+          # If any of the specified plugins in the manifest dont have a valid path we will
+          # eventually get an error and the user will need to rehash - instead, lets just
+          # print out 1 error here telling them to rehash
+          errors = {}
+          commands.collect { |k, v| v }.flatten.each do |command|
+            paths = manifest[KEY]["plugins_paths"][command]
+            if paths && paths.is_a?(Array)
+              # It is only an error if all the paths don't exist
+              if paths.all? { |sc| !File.exists?(sc) }
+                errors[command] = paths
+              end
+            end
+          end
+          if errors.empty?
+            commands
+          else
+            Chef::Log.error "There are files specified in the manifest that are missing.  Please rehash to update the subcommands cache.  If you see this error after rehashing delete the cache at #{Chef::Knife::SubcommandLoader.plugin_manifest_path}"
+            Chef::Log.error "Missing files:\n\t#{errors.values.flatten.join("\n\t")}"
+            {}
           end
         end
 
@@ -59,7 +79,6 @@ class Chef
               if File.exists?(sc)
                 Kernel.load sc
               else
-                Chef::Log.error "The file #{sc} is missing for subcommand '#{subcommand_for_args(args)}'. Please rehash to update the subcommands cache."
                 return false
               end
             end
diff --git a/lib/chef/knife/core/subcommand_loader.rb b/lib/chef/knife/core/subcommand_loader.rb
index 95ab219c80..dc0b0cc39c 100644
--- a/lib/chef/knife/core/subcommand_loader.rb
+++ b/lib/chef/knife/core/subcommand_loader.rb
@@ -58,6 +58,12 @@ class Chef
         end
       end
 
+      # There are certain situations where we want to shortcut the loader selection
+      # in self.for_config and force using the GemGlobLoader
+      def self.gem_glob_loader(chef_config_dir)
+        Knife::SubcommandLoader::GemGlobLoader.new(chef_config_dir)
+      end
+
       def self.plugin_manifest?
         plugin_manifest_path && File.exist?(plugin_manifest_path)
       end
diff --git a/lib/chef/knife/rehash.rb b/lib/chef/knife/rehash.rb
index 3e7bab7e0f..79286368f8 100644
--- a/lib/chef/knife/rehash.rb
+++ b/lib/chef/knife/rehash.rb
@@ -35,7 +35,10 @@ class Chef
       end
 
       def reload_plugins
-        Chef::Knife::SubcommandLoader::GemGlobLoader.new(@@chef_config_dir).load_commands
+        # The subcommand_loader for this knife command should _always_ be the GemGlobLoader.  The GemGlobLoader loads
+        # plugins from disc and ensures the hash we write is always correct.  By this point it should also already have
+        # loaded plugins and `load_commands` shouldn't have an effect.
+        Chef::Knife.subcommand_loader.load_commands
       end
 
       def generate_hash
diff --git a/spec/functional/knife/rehash_spec.rb b/spec/functional/knife/rehash_spec.rb
new file mode 100644
index 0000000000..22ffa125fa
--- /dev/null
+++ b/spec/functional/knife/rehash_spec.rb
@@ -0,0 +1,39 @@
+#
+# Author:: Daniel DeLeo (<dan@chef.io>)
+# Copyright:: Copyright 2010-2016, Chef Software Inc.
+# License:: Apache License, Version 2.0
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+require "spec_helper"
+
+require "chef/knife/rehash"
+require "chef/knife/core/subcommand_loader"
+
+describe "knife rehash" do
+  before do
+    allow(Chef::Knife::SubcommandLoader).to receive(:load_commands)
+  end
+
+  after do
+    # We need to clean up the generated manifest or else is messes with later tests
+    FileUtils.rm_f(Chef::Knife::SubcommandLoader.plugin_manifest_path)
+  end
+
+  it "writes the loaded plugins to disc" do
+    knife_rehash = Chef::Knife::Rehash.new
+    knife_rehash.run
+    expect(File.read(Chef::Knife::SubcommandLoader.plugin_manifest_path)).to match(/node_list.rb/)
+  end
+end
diff --git a/spec/unit/knife/core/hashed_command_loader_spec.rb b/spec/unit/knife/core/hashed_command_loader_spec.rb
index 0135868bf5..081f9deae5 100644
--- a/spec/unit/knife/core/hashed_command_loader_spec.rb
+++ b/spec/unit/knife/core/hashed_command_loader_spec.rb
@@ -46,6 +46,10 @@ describe Chef::Knife::SubcommandLoader::HashedCommandLoader do
     plugin_manifest)}
 
   describe "#list_commands" do
+    before do
+      allow(File).to receive(:exists?).and_return(true)
+    end
+
     it "lists all commands by category when no argument is given" do
       expect(loader.list_commands).to eq({ "cool" => ["cool_a"], "cooler" => ["cooler_b"] })
     end
@@ -53,6 +57,18 @@ describe Chef::Knife::SubcommandLoader::HashedCommandLoader do
     it "lists only commands in the given category when a category is given" do
       expect(loader.list_commands("cool")).to eq({ "cool" => ["cool_a"] })
     end
+
+    context "when the plugin path is invalid" do
+      before do
+        expect(File).to receive(:exists?).with("/file/for/plugin/b").and_return(false)
+      end
+
+      it "lists all commands by category when no argument is given" do
+        expect(Chef::Log).to receive(:error).with(/There are files specified in the manifest that are missing/)
+        expect(Chef::Log).to receive(:error).with("Missing files:\n\t/file/for/plugin/b")
+        expect(loader.list_commands).to eq({})
+      end
+    end
   end
 
   describe "#subcommand_files" do
diff --git a/spec/unit/knife/core/subcommand_loader_spec.rb b/spec/unit/knife/core/subcommand_loader_spec.rb
index 6cda244132..b235102a0b 100644
--- a/spec/unit/knife/core/subcommand_loader_spec.rb
+++ b/spec/unit/knife/core/subcommand_loader_spec.rb
@@ -61,4 +61,10 @@ describe Chef::Knife::SubcommandLoader do
       end
     end
   end
+
+  describe "#gem_glob_loader" do
+    it "always creates a GemGlobLoader" do
+      expect(Chef::Knife::SubcommandLoader.gem_glob_loader(config_dir)).to be_a Chef::Knife::SubcommandLoader::GemGlobLoader
+    end
+  end
 end
diff --git a/spec/unit/knife_spec.rb b/spec/unit/knife_spec.rb
index 698ae205f1..ec1e59d863 100644
--- a/spec/unit/knife_spec.rb
+++ b/spec/unit/knife_spec.rb
@@ -23,6 +23,7 @@ end
 
 require "spec_helper"
 require "uri"
+require "chef/knife/core/gem_glob_loader"
 
 describe Chef::Knife do
 
@@ -148,6 +149,12 @@ describe Chef::Knife do
       expect(Chef::Knife.subcommand_class_from(%w{cookbook site vendor --help foo bar baz})).to eq(:CookbookSiteVendor)
     end
 
+    it "special case sets the subcommand_loader to GemGlobLoader when running rehash" do
+      Chef::Knife.subcommands["rehash"] = :Rehash
+      expect(Chef::Knife.subcommand_class_from(%w{rehash })).to eq(:Rehash)
+      expect(Chef::Knife.subcommand_loader).to be_a(Chef::Knife::SubcommandLoader::GemGlobLoader)
+    end
+
   end
 
   describe "the headers include X-Remote-Request-Id" do