Uniquely identify servers based on hostname, port, and username, instead of merely on hostname

git-svn-id: http://svn.rubyonrails.org/rails/tools/capistrano@6704 5ecf4fe2-1ee6-0310-87b1-e25e094e27de

CHANGELOG

@@ -1,5 +1,7 @@
 *SVN*
 
+* Uniquely identify servers based on hostname, IP address, and username, instead of merely on hostname [Jamis Buck]
+
 * Allow (e.g.) scm_command and local_scm_command to be set in the event of different paths to the scm command on local vs. remote hosts. [Jamis Buck]
 
 * Kill the "deploy:app" namespace and move those tasks into deploy, directly. [Jamis Buck]

lib/capistrano/configuration/actions/file_transfer.rb

@@ -10,7 +10,7 @@ module Capistrano
         # set the mode on the file.
         def put(data, path, options={})
           execute_on_servers(options) do |servers|
-            targets = servers.map { |s| sessions[s.host] }
+            targets = servers.map { |s| sessions[s] }
             Upload.process(targets, path, :data => data, :mode => options[:mode], :logger => logger)
           end
         end
@@ -24,7 +24,7 @@ module Capistrano
         def get(remote_path, path, options = {})
           execute_on_servers(options.merge(:once => true)) do |servers|
             logger.info "downloading `#{servers.first.host}:#{remote_path}' to `#{path}'"
-            sftp = sessions[servers.first.host].sftp
+            sftp = sessions[servers.first].sftp
             sftp.connect unless sftp.state == :open
             sftp.get_file remote_path, path
             logger.debug "download finished" 

lib/capistrano/configuration/actions/invocation.rb

@@ -38,7 +38,7 @@ module Capistrano
           logger.debug "executing #{cmd.strip.inspect}"
 
           execute_on_servers(options) do |servers|
-            targets = servers.map { |s| sessions[s.host] }
+            targets = servers.map { |s| sessions[s] }
             Command.process(cmd, targets, options.merge(:logger => logger), &block)
           end
         end

lib/capistrano/configuration/connections.rb

@@ -67,7 +67,7 @@ module Capistrano
 
         if sessions.empty?
           server, servers = servers.first, servers[1..-1]
-          sessions[server.host] = connection_factory.connect_to(server)
+          sessions[server] = connection_factory.connect_to(server)
         end
 
         servers.map { |server| establish_connection_to(server) }.each { |t| t.join }
@@ -104,7 +104,7 @@ module Capistrano
         # prevents problems with the thread's scope seeing the wrong 'server'
         # variable if the thread just happens to take too long to start up.
         def establish_connection_to(server)
-          Thread.new { sessions[server.host] ||= connection_factory.connect_to(server) }
+          Thread.new { sessions[server] ||= connection_factory.connect_to(server) }
         end
     end
   end

lib/capistrano/server_definition.rb

@@ -1,5 +1,7 @@
 module Capistrano
   class ServerDefinition
+    include Comparable
+
     attr_reader :host
     attr_reader :user
     attr_reader :port
@@ -17,6 +19,10 @@ module Capistrano
       @port = @port.to_i if @port
     end
 
+    def <=>(server)
+      [host, port, user] <=> [server.host, server.port, server.user]
+    end
+
     # Redefined, so that Array#uniq will work to remove duplicate server
     # definitions, based solely on their host names.
     def eql?(server)
@@ -28,7 +34,7 @@ module Capistrano
     alias :== :eql?
 
     # Redefined, so that Array#uniq will work to remove duplicate server
-    # definitions, based solely on their host names.
+    # definitions, based on their connection information.
     def hash
       @hash ||= [host, user, port].hash
     end

lib/capistrano/shell.rb

@@ -129,7 +129,7 @@ HELP
       # servers (names).
       def connect(task)
         servers = configuration.find_servers_for_task(task)
-        needing_connections = servers.reject { |s| configuration.sessions.key?(s.host) }
+        needing_connections = servers - configuration.sessions.keys
         unless needing_connections.empty?
           puts "[establishing connection(s) to #{needing_connections.map { |s| s.host }.join(', ')}]"
           configuration.establish_connections_to(needing_connections)
@@ -182,7 +182,7 @@ HELP
         end
 
         previous = trap("INT") { cmd.stop! }
-        sessions = servers.map { |server| configuration.sessions[server.host] }
+        sessions = servers.map { |server| configuration.sessions[server] }
         Command.process(command, sessions, :logger => configuration.logger, &Capistrano::Configuration.default_io_proc)
       rescue Capistrano::Error => error
         warn "error: #{error.message}"

test/configuration/actions/file_transfer_test.rb

@@ -18,7 +18,7 @@ class ConfigurationActionsFileTransferTest < Test::Unit::TestCase
 
   def test_put_should_delegate_to_Upload_process
     @config.expects(:execute_on_servers).yields(%w(s1 s2 s3).map { |s| mock(:host => s) })
-    @config.expects(:sessions).times(3).returns(Hash.new{|h,k| h[k] = k.to_sym})
+    @config.expects(:sessions).times(3).returns(Hash.new{|h,k| h[k] = k.host.to_sym})
     Capistrano::Upload.expects(:process).with([:s1,:s2,:s3], "test.txt", :data => "some data", :mode => 0777, :logger => @config.logger)
     @config.put("some data", "test.txt", :mode => 0777)
   end
@@ -31,8 +31,10 @@ class ConfigurationActionsFileTransferTest < Test::Unit::TestCase
   def test_get_should_use_sftp_get_file_to_local_path
     sftp = mock("sftp", :state => :closed, :connect => true)
     sftp.expects(:get_file).with("remote.txt", "local.txt")
-    @config.expects(:execute_on_servers).yields([stub("server", :host => "capistrano")])
-    @config.expects(:sessions).returns("capistrano" => mock("session", :sftp => sftp))
+
+    s = server("capistrano")
+    @config.expects(:execute_on_servers).yields([s])
+    @config.expects(:sessions).returns(s => mock("session", :sftp => sftp))
     @config.get("remote.txt", "local.txt")
   end
 end

test/configuration/actions/invocation_test.rb

@@ -37,7 +37,7 @@ class ConfigurationActionsInvocationTest < Test::Unit::TestCase
 
   def test_run_without_block_should_use_default_io_proc
     @config.expects(:execute_on_servers).yields(%w(s1 s2 s3).map { |s| mock(:host => s) })
-    @config.expects(:sessions).returns(Hash.new { |h,k| h[k] = k.to_sym }).times(3)
+    @config.expects(:sessions).returns(Hash.new { |h,k| h[k] = k.host.to_sym }).times(3)
     prepare_command("ls", [:s1, :s2, :s3], {:logger => @config.logger})
     MockConfig.default_io_proc = inspectable_proc
     @config.run "ls"
@@ -45,7 +45,7 @@ class ConfigurationActionsInvocationTest < Test::Unit::TestCase
 
   def test_run_with_block_should_use_block
     @config.expects(:execute_on_servers).yields(%w(s1 s2 s3).map { |s| mock(:host => s) })
-    @config.expects(:sessions).returns(Hash.new { |h,k| h[k] = k.to_sym }).times(3)
+    @config.expects(:sessions).returns(Hash.new { |h,k| h[k] = k.host.to_sym }).times(3)
     prepare_command("ls", [:s1, :s2, :s3], {:logger => @config.logger})
     MockConfig.default_io_proc = Proc.new { |a,b,c| raise "shouldn't get here" }
     @config.run("ls", &inspectable_proc)

test/configuration/connections_test.rb

@@ -88,9 +88,9 @@ class ConfigurationConnectionsTest < Test::Unit::TestCase
 
   def test_establish_connections_to_should_not_attempt_to_reestablish_existing_connections
     Capistrano::SSH.expects(:connect).times(2).returns(:success)
-    @config.sessions["cap1"] = :ok
+    @config.sessions[server("cap1")] = :ok
     @config.establish_connections_to(%w(cap1 cap2 cap3).map { |s| server(s) })
-    assert %w(cap1 cap2 cap3), @config.sessions.keys.sort
+    assert %w(cap1 cap2 cap3), @config.sessions.keys.sort.map { |s| s.host }
   end
 
   def test_execute_on_servers_should_require_a_block
@@ -127,7 +127,7 @@ class ConfigurationConnectionsTest < Test::Unit::TestCase
     @config.expects(:find_servers_for_task).with(@config.current_task, {}).returns([server("cap1"), server("cap2"), server("cap3")])
     Capistrano::SSH.expects(:connect).times(3).returns(:success)
     @config.execute_on_servers {}
-    assert_equal %w(cap1 cap2 cap3), @config.sessions.keys.sort
+    assert_equal %w(cap1 cap2 cap3), @config.sessions.keys.sort.map { |s| s.host }
   end
 
   def test_execute_on_servers_should_yield_server_list_to_block
@@ -141,7 +141,7 @@ class ConfigurationConnectionsTest < Test::Unit::TestCase
       assert servers.detect { |s| s.host == "cap1" }
       assert servers.detect { |s| s.host == "cap2" }
       assert servers.detect { |s| s.host == "cap3" }
-      assert servers.all? { |s| @config.sessions[s.host] }
+      assert servers.all? { |s| @config.sessions[s] }
     end
     assert block_called
   end
@@ -157,7 +157,7 @@ class ConfigurationConnectionsTest < Test::Unit::TestCase
       assert_equal %w(cap1), servers.map { |s| s.host }
     end
     assert block_called
-    assert_equal %w(cap1), @config.sessions.keys.sort
+    assert_equal %w(cap1), @config.sessions.keys.sort.map { |s| s.host }
   end
 
   def test_connect_should_establish_connections_to_all_servers_in_scope
@@ -166,7 +166,7 @@ class ConfigurationConnectionsTest < Test::Unit::TestCase
     @config.expects(:find_servers_for_task).with(@config.current_task, {}).returns([server("cap1"), server("cap2"), server("cap3")])
     Capistrano::SSH.expects(:connect).times(3).returns(:success)
     @config.connect!
-    assert_equal %w(cap1 cap2 cap3), @config.sessions.keys.sort
+    assert_equal %w(cap1 cap2 cap3), @config.sessions.keys.sort.map { |s| s.host }
   end
 
   def test_connect_should_honor_once_option
@@ -175,6 +175,6 @@ class ConfigurationConnectionsTest < Test::Unit::TestCase
     @config.expects(:find_servers_for_task).with(@config.current_task, :once => true).returns([server("cap1"), server("cap2"), server("cap3")])
     Capistrano::SSH.expects(:connect).returns(:success)
     @config.connect! :once => true
-    assert_equal %w(cap1), @config.sessions.keys.sort
+    assert_equal %w(cap1), @config.sessions.keys.sort.map { |s| s.host }
   end
 end

test/server_definition_test.rb

@@ -71,6 +71,20 @@ class ServerDefinitionTest < Test::Unit::TestCase
     assert s1.eql?(s2)
   end
 
+  def test_servers_should_be_comparable
+    s1 = server("jamis@www.capistrano.test:8080")
+    s2 = server("www.alphabet.test:1234")
+    s3 = server("jamis@www.capistrano.test:8075")
+    s4 = server("billy@www.capistrano.test:8080")
+
+    assert s2 < s1
+    assert s3 < s1
+    assert s4 < s1
+    assert s2 < s3
+    assert s2 < s4
+    assert s3 < s4
+  end
+
   def test_comparison_should_not_match_when_any_of_host_user_port_differ
     s1 = server("jamis@www.capistrano.test:8080")
     s2 = server("bob@www.capistrano.test:8080")