Merge pull request #1530 from opscode/btm/CHEF-5158

CHEF-5158: Prefer CLI argument over configuration file setting for ssh_attribute

3 files changed, 18 insertions, 21 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 8fe9920ac9..56492db8b6 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -87,7 +87,8 @@
   Add support for automatically using the Systemd service provider when available. (CHEF-3637)
 * [**Matt Hoyle**](https://github.com/deployable):
   Add timeout for Chef::Provider::Service::Windows. (CHEF-1165)
-
+* [**Jesse Hu**](https://github.com/jessehu):
+  knife[:attribute] in knife.rb should not override --attribute (CHEF-5158)
 
 * Update rpm provider checking regex to allow for special characters (CHEF-4893)
 * Allow for spaces in selinux controlled directories (CHEF-5095)
diff --git a/lib/chef/knife/ssh.rb b/lib/chef/knife/ssh.rb
index 4edc63d8d3..6f3dac09db 100644
--- a/lib/chef/knife/ssh.rb
+++ b/lib/chef/knife/ssh.rb
@@ -170,12 +170,16 @@ class Chef
           # if a command line attribute was not passed, and we have a
           # cloud public_hostname, use that.  see #configure_attribute
           # for the source of config[:attribute] and
-          # config[:override_attribute]
-          if config[:override_attribute]
-            host = extract_nested_value(item, config[:override_attribute])
+          # config[:attribute_from_cli]
+          if config[:attribute_from_cli]
+            Chef::Log.debug("Using node attribute '#{config[:attribute_from_cli]}' from the command line as the ssh target")
+            host = extract_nested_value(item, config[:attribute_from_cli])
           elsif item[:cloud] && item[:cloud][:public_hostname]
+            Chef::Log.debug("Using node attribute 'cloud[:public_hostname]' automatically as the ssh target")
             host = item[:cloud][:public_hostname]
           else
+            # ssh attribute from a configuration file or the default will land here
+            Chef::Log.debug("Using node attribute '#{config[:attribute]}' as the ssh target")
             host = extract_nested_value(item, config[:attribute])
           end
           # next if we couldn't find the specified attribute in the
@@ -413,10 +417,8 @@ class Chef
         # Thus we can differentiate between a config file value and a command line override at this point by checking config[:attribute]
         # We can tell here if fqdn was passed from the command line, rather than being the default, by checking config[:attribute]
         # However, after here, we cannot tell these things, so we must preserve config[:attribute]
-        config[:override_attribute] = config[:attribute] || Chef::Config[:knife][:ssh_attribute]
-        config[:attribute] = (Chef::Config[:knife][:ssh_attribute] ||
-                              config[:attribute] ||
-                              "fqdn").strip
+        config[:attribute_from_cli] = config[:attribute]
+        config[:attribute] = (config[:attribute_from_cli] || Chef::Config[:knife][:ssh_attribute] || "fqdn").strip
       end
 
       def cssh
diff --git a/spec/unit/knife/ssh_spec.rb b/spec/unit/knife/ssh_spec.rb
index 9247db3c90..10d63c9c74 100644
--- a/spec/unit/knife/ssh_spec.rb
+++ b/spec/unit/knife/ssh_spec.rb
@@ -52,7 +52,7 @@ describe Chef::Knife::Ssh do
       def self.should_return_specified_attributes
         it "returns an array of the attributes specified on the command line OR config file, if only one is set" do
           @knife.config[:attribute] = "ipaddress"
-          @knife.config[:override_attribute] = "ipaddress"
+          @knife.config[:attribute_from_cli] = "ipaddress"
           configure_query([@node_foo, @node_bar])
           @knife.should_receive(:session_from_list).with([['10.0.0.1', nil], ['10.0.0.2', nil]])
           @knife.configure_session
@@ -60,7 +60,7 @@ describe Chef::Knife::Ssh do
 
         it "returns an array of the attributes specified on the command line even when a config value is set" do
           @knife.config[:attribute] = "config_file" # this value will be the config file
-          @knife.config[:override_attribute] = "ipaddress" # this is the value of the command line via #configure_attribute
+          @knife.config[:attribute_from_cli] = "ipaddress" # this is the value of the command line via #configure_attribute
           configure_query([@node_foo, @node_bar])
           @knife.should_receive(:session_from_list).with([['10.0.0.1', nil], ['10.0.0.2', nil]])
           @knife.configure_session
@@ -155,25 +155,19 @@ describe Chef::Knife::Ssh do
       @knife.config[:attribute].should == "command_line"
     end
 
-    it "should set override_attribute to the value of attribute from the command line" do
+    it "should set attribute_from_cli to the value of attribute from the command line" do
       @knife.config[:attribute] = "command_line"
       @knife.configure_attribute
       @knife.config[:attribute].should == "command_line"
-      @knife.config[:override_attribute].should == "command_line"
+      @knife.config[:attribute_from_cli].should == "command_line"
     end
 
-    it "should set override_attribute to the value of attribute from the config file" do
-      Chef::Config[:knife][:ssh_attribute] = "config_file"
-      @knife.configure_attribute
-      @knife.config[:attribute].should == "config_file"
-      @knife.config[:override_attribute].should == "config_file"
-    end
-
-    it "should prefer the command line over the config file for the value of override_attribute" do
+    it "should prefer the command line over the config file for the value of attribute_from_cli" do
       Chef::Config[:knife][:ssh_attribute] = "config_file"
       @knife.config[:attribute] = "command_line"
       @knife.configure_attribute
-      @knife.config[:override_attribute].should == "command_line"
+      @knife.config[:attribute].should == "command_line"
+      @knife.config[:attribute_from_cli].should == "command_line"
     end
   end