[CHEF-3497] apply config in the desired order

Takes advantage of new mixlib-cli option to keep default values from the
mixlib-cli DSL separate from user-supplied values. Config settings are
merged:
1. Defaults from mixlib-cli DSL
2. Settings from Chef::Config[:knife]
3. Values from CLI options

5 files changed, 90 insertions, 29 deletions

diff --git a/lib/chef/knife.rb b/lib/chef/knife.rb
index 15ad38e25e..421a329f22 100644
--- a/lib/chef/knife.rb
+++ b/lib/chef/knife.rb
@@ -57,6 +57,11 @@ class Chef
     attr_accessor :name_args
     attr_accessor :ui
 
+    # Configure mixlib-cli to always separate defaults from user-supplied CLI options
+    def self.use_separate_defaults?
+      true
+    end
+
     def self.ui
       @ui ||= Chef::Knife::UI.new(STDOUT, STDERR, STDIN, {})
     end
@@ -274,7 +279,6 @@ class Chef
       command_name_words = command_name_words.join('_')
       @name_args.reject! { |name_arg| command_name_words == name_arg }
 
-
       if config[:help]
         msg opt_parser
         exit 1
@@ -293,19 +297,15 @@ class Chef
       exit(1)
     end
 
-    def allow_auto_knife_config?
-      false
-    end
-
-    def configure_from_file_settings!
-      unless(config[:config_file])
-        locate_config_file
-      end
-      if(config[:config_file])
-        self.class.options.keys.each do |key|
-          config[key] = Chef::Config[:knife][key] if Chef::Config[:knife].has_key?(key)
-        end
+    # Returns a subset of the Chef::Config[:knife] Hash that is relevant to the
+    # currently executing knife command. This is used by #configure_chef to
+    # apply settings from knife.rb to the +config+ hash.
+    def config_file_settings
+      config_file_settings = {}
+      self.class.options.keys.each do |key|
+        config_file_settings[key] = Chef::Config[:knife][key] if Chef::Config[:knife].has_key?(key)
       end
+      config_file_settings
     end
 
     def locate_config_file
@@ -337,20 +337,26 @@ class Chef
       end
     end
 
-    def configure_chef
-      unless config[:config_file]
-        locate_config_file
-      end
-
-      # Don't try to load a knife.rb if it doesn't exist.
-      if config[:config_file]
-        read_config_file(config[:config_file])
-      else
-        # ...but do log a message if no config was found.
-        Chef::Config[:color] = config[:color]
-        ui.warn("No knife configuration file found")
-      end
-
+    # Apply Config in this order:
+    # defaults from mixlib-cli
+    # settings from config file, via Chef::Config[:knife]
+    # config from command line
+    def merge_configs
+      # Apply config file settings on top of mixlib-cli defaults
+      combined_config = default_config.merge(config_file_settings)
+      # Apply user-supplied options on top of the above combination
+      combined_config = combined_config.merge(config)
+      # replace the config hash from mixlib-cli with our own.
+      # Need to use the mutate-in-place #replace method instead of assigning to
+      # the instance variable because other code may have a reference to the
+      # original config hash object.
+      config.replace(combined_config)
+    end
+
+    # Catch-all method that does any massaging needed for various config
+    # components, such as expanding file paths and converting verbosity level
+    # into log level.
+    def apply_computed_config
       Chef::Config[:color] = config[:color]
 
       case Chef::Config[:verbosity]
@@ -377,8 +383,6 @@ class Chef
       Chef::Log.init(Chef::Config[:log_location])
       Chef::Log.level(Chef::Config[:log_level] || :error)
 
-      Chef::Log.debug("Using configuration from #{config[:config_file]}")
-      
       if Chef::Config[:node_name] && Chef::Config[:node_name].bytesize > 90
         # node names > 90 bytes only work with authentication protocol >= 1.1
         # see discussion in config.rb.
@@ -386,6 +390,25 @@ class Chef
       end
     end
 
+    def configure_chef
+      unless config[:config_file]
+        locate_config_file
+      end
+
+      # Don't try to load a knife.rb if it doesn't exist.
+      if config[:config_file]
+        Chef::Log.debug("Using configuration from #{config[:config_file]}")
+        read_config_file(config[:config_file])
+      else
+        # ...but do log a message if no config was found.
+        Chef::Config[:color] = config[:color]
+        ui.warn("No knife configuration file found")
+      end
+
+      merge_configs
+      apply_computed_config
+    end
+
     def read_config_file(file)
       Chef::Config.from_file(file)
     rescue SyntaxError => e
diff --git a/spec/unit/application/knife_spec.rb b/spec/unit/application/knife_spec.rb
index 78a65e7045..9560de8d13 100644
--- a/spec/unit/application/knife_spec.rb
+++ b/spec/unit/application/knife_spec.rb
@@ -23,6 +23,11 @@ describe Chef::Application::Knife do
 
   before(:all) do
     class NoopKnifeCommand < Chef::Knife
+      option :opt_with_default,
+        :short => "-D VALUE",
+        :long => "-optwithdefault VALUE",
+        :default => "default-value"
+
       def run
       end
     end
diff --git a/spec/unit/knife/bootstrap_spec.rb b/spec/unit/knife/bootstrap_spec.rb
index 76261d96ab..98955d4f73 100644
--- a/spec/unit/knife/bootstrap_spec.rb
+++ b/spec/unit/knife/bootstrap_spec.rb
@@ -35,6 +35,8 @@ describe Chef::Knife::Bootstrap do
   before(:each) do
     Chef::Log.logger = Logger.new(StringIO.new)
     @knife = Chef::Knife::Bootstrap.new
+    # Merge default settings in.
+    @knife.merge_configs
     @knife.config[:template_file] = File.expand_path(File.join(CHEF_SPEC_DATA, "bootstrap", "test.erb"))
     @stdout = StringIO.new
     @knife.ui.stub!(:stdout).and_return(@stdout)
diff --git a/spec/unit/knife/ssh_spec.rb b/spec/unit/knife/ssh_spec.rb
index a4c4c33c8b..7f1ed0e321 100644
--- a/spec/unit/knife/ssh_spec.rb
+++ b/spec/unit/knife/ssh_spec.rb
@@ -34,6 +34,7 @@ describe Chef::Knife::Ssh do
 
   before do
     @knife = Chef::Knife::Ssh.new
+    @knife.merge_configs
     @knife.config[:attribute] = "fqdn"
     @node_foo = Chef::Node.new
     @node_foo.automatic_attrs[:fqdn] = "foo.example.org"
diff --git a/spec/unit/knife_spec.rb b/spec/unit/knife_spec.rb
index e8fdc38d2b..d4d721a20d 100644
--- a/spec/unit/knife_spec.rb
+++ b/spec/unit/knife_spec.rb
@@ -180,6 +180,36 @@ describe Chef::Knife do
       lambda {Chef::Knife.run(%w{fuuu uuuu fuuuu})}.should raise_error(SystemExit) { |e| e.status.should_not == 0 }
     end
 
+    describe "merging configuration options" do
+      before do
+        KnifeSpecs::TestYourself.option(:opt_with_default,
+                                        :short => "-D VALUE",
+                                        :default => "default-value")
+        Chef::Config[:knife] = {}
+      end
+
+      it "prefers the default value if no config or command line value is present" do
+        knife_command = KnifeSpecs::TestYourself.new([]) #empty argv
+        knife_command.configure_chef
+        knife_command.config[:opt_with_default].should == "default-value"
+      end
+
+      it "prefers a value in Chef::Config[:knife] to the default" do
+        Chef::Config[:knife][:opt_with_default] = "from-knife-config"
+        knife_command = KnifeSpecs::TestYourself.new([]) #empty argv
+        knife_command.configure_chef
+        knife_command.config[:opt_with_default].should == "from-knife-config"
+      end
+
+      it "prefers a value from command line over Chef::Config and the default" do
+        Chef::Config[:knife][:opt_with_default] = "from-knife-config"
+        knife_command = KnifeSpecs::TestYourself.new(["-D", "from-cli"])
+        knife_command.configure_chef
+        knife_command.config[:opt_with_default].should == "from-cli"
+      end
+    end
+
+
   end
 
   describe "when first created" do