From 554f3e5107b31d3ab4c2cd0ce35bac3bb65db93a Mon Sep 17 00:00:00 2001 From: tyler-ball Date: Wed, 2 Mar 2016 10:02:35 -0700 Subject: First part of fixing the rehash command, it now always loads gems from disc instead of using the hash file --- lib/chef/knife.rb | 5 +++++ lib/chef/knife/core/subcommand_loader.rb | 6 ++++++ lib/chef/knife/rehash.rb | 5 ++++- 3 files changed, 15 insertions(+), 1 deletion(-) 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/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..8ede30643a 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 -- cgit v1.2.1 From 3cda9adee23631665f6b5e7435eac09673876ba8 Mon Sep 17 00:00:00 2001 From: tyler-ball Date: Wed, 2 Mar 2016 10:45:14 -0700 Subject: Changing the knife behavior when using the cached plugins to only display 1 error message when plugin files are missing from the cache --- lib/chef/knife/core/hashed_command_loader.rb | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/lib/chef/knife/core/hashed_command_loader.rb b/lib/chef/knife/core/hashed_command_loader.rb index 7b6c1c4c08..dee9e973fb 100644 --- a/lib/chef/knife/core/hashed_command_loader.rb +++ b/lib/chef/knife/core/hashed_command_loader.rb @@ -40,9 +40,28 @@ 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) + 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 +78,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 -- cgit v1.2.1 From 68b188dbd933a5ec97e5540d22541c57480bdad3 Mon Sep 17 00:00:00 2001 From: tyler-ball Date: Wed, 2 Mar 2016 12:19:48 -0700 Subject: Adding specs for rehash update --- lib/chef/knife/core/hashed_command_loader.rb | 5 +-- lib/chef/knife/rehash.rb | 2 +- spec/functional/knife/rehash_spec.rb | 39 ++++++++++++++++++++++ spec/unit/knife/core/hashed_command_loader_spec.rb | 16 +++++++++ spec/unit/knife/core/subcommand_loader_spec.rb | 6 ++++ spec/unit/knife_spec.rb | 7 ++++ 6 files changed, 72 insertions(+), 3 deletions(-) create mode 100644 spec/functional/knife/rehash_spec.rb diff --git a/lib/chef/knife/core/hashed_command_loader.rb b/lib/chef/knife/core/hashed_command_loader.rb index dee9e973fb..1165af6eb2 100644 --- a/lib/chef/knife/core/hashed_command_loader.rb +++ b/lib/chef/knife/core/hashed_command_loader.rb @@ -48,10 +48,11 @@ class Chef # 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| + commands.collect { |k, v| v }.flatten.each do |command| paths = manifest[KEY]["plugins_paths"][command] if paths && paths.is_a?(Array) - if paths.all? {|sc| !File.exists?(sc)} + # It is only an error if all the paths don't exist + if paths.all? { |sc| !File.exists?(sc) } errors[command] = paths end end diff --git a/lib/chef/knife/rehash.rb b/lib/chef/knife/rehash.rb index 8ede30643a..79286368f8 100644 --- a/lib/chef/knife/rehash.rb +++ b/lib/chef/knife/rehash.rb @@ -38,7 +38,7 @@ class Chef # 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 + 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 () +# 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 -- cgit v1.2.1