Merge pull request #1368 from townsen/consolidate_servers_on_hostname

Consolidate servers on hostname

CHANGELOG.md

@@ -14,6 +14,15 @@ Reverse Chronological Order:
   * release_roles did not honour additional property filtering (@townsen)
   * Refactored and simplified property filtering code (@townsen)
 
+* Breaking Changes
+  * Hosts with the same name are now consolidated into one irrespective of the
+    user and port. This allows multiple declarations of a server to be made safely.
+    The last declared properties will win. See capistrnorb.com Properties documentation
+    for details.
+  * Inside the on() block the host variable is now a copy of the host, so changes can be
+    made within the block (such as dynamically overriding the user) that will not persist.
+    This is very convenient for switching the SSH user temporarily to 'root' for example.
+
 * Minor changes
   * Add role_properties() method (see capistrano.github.io PR for doc) (@townsen)
   * Add equality syntax ( eg. port: 1234) for property filtering (@townsen)

features/sshconnect.feature

@@ -0,0 +1,17 @@
+Feature: SSH Connection
+
+  Background:
+    Given a test app with the default configuration
+    And servers with the roles app and web
+    And a task which executes as root
+
+  Scenario: Switching from default user to root and back again
+    When I run cap "git:check"
+    Then the task is successful
+    And references in the remote repo are listed
+    When I run cap "am_i_root"
+    Then the task is successful
+    And contains "root" in the output
+    Then I run cap "git:check"
+    Then the task is successful
+    And references in the remote repo are listed

features/step_definitions/setup.rb

@@ -27,6 +27,10 @@ Given(/^a custom task to generate a file$/) do
   TestApp.copy_task_to_test_app('spec/support/tasks/database.rake')
 end
 
+Given(/^a task which executes as root$/) do
+  TestApp.copy_task_to_test_app('spec/support/tasks/root.rake')
+end
+
 Given(/config stage file has line "(.*?)"/) do |line|
   TestApp.append_to_deploy_file(line)
 end

lib/capistrano/configuration/server.rb

@@ -62,7 +62,7 @@ module Capistrano
       end
 
       def matches?(other)
-        user == other.user && hostname == other.hostname && port == other.port
+        hostname == other.hostname
       end
 
       private
@@ -98,7 +98,7 @@ module Capistrano
           @properties[key]
         end
 
-        def respond_to?(method)
+        def respond_to?(method, include_all=false)
           @properties.has_key?(method)
         end
 

lib/capistrano/configuration/servers.rb

@@ -8,7 +8,14 @@ module Capistrano
       include Enumerable
 
       def add_host(host, properties={})
-        servers.add server(host, properties).with(properties)
+        new_host = Server[host]
+        if server = servers.find { |s| s.matches? new_host }
+          server.user = new_host.user if new_host.user
+          server.port = new_host.port if new_host.port
+          server.with(properties)
+        else
+          servers << new_host.with(properties)
+        end
       end
 
       def add_role(role, hosts, options={})
@@ -50,15 +57,8 @@ module Capistrano
 
       private
 
-      def server(host, properties)
-        new_host = Server[host]
-        new_host.with({user: properties[:user]}) unless properties[:user].nil?
-        new_host.with({port: properties[:port]}) unless properties[:port].nil?
-        servers.find { |server| server.matches? new_host } || new_host
-      end
-
       def servers
-        @servers ||= Set.new
+        @servers ||= []
       end
 
       def extract_options(array)

lib/capistrano/dsl.rb

@@ -51,8 +51,8 @@ module Capistrano
     end
 
     def on(hosts, options={}, &block)
-      subset = Configuration.env.filter hosts
-      SSHKit::Coordinator.new(subset).each(options, &block)
+      subset_copy = Marshal.dump(Configuration.env.filter(hosts))
+      SSHKit::Coordinator.new(Marshal.load(subset_copy)).each(options, &block)
     end
 
     def run_locally(&block)

spec/integration/dsl_spec.rb

@@ -241,25 +241,45 @@ describe Capistrano::DSL do
       end
 
       describe 'fetching all servers' do
-        subject { dsl.roles(:all).map { |server| "#{server.user}@#{server.hostname}:#{server.port}" } }
-
-        it 'creates a server instance for each unique user@host:port combination' do
-          expect(subject).to eq %w{db@example1.com:1234 root@example1.com:1234 @example1.com:5678 deployer@example1.com:1234}
+        it 'creates one server per hostname, ignoring user and port combinations' do
+          expect(dsl.roles(:all).size).to eq(1)
         end
       end
 
       describe 'fetching servers for a role' do
         it 'roles defined using the `server` syntax are included' do
-          expect(dsl.roles(:web).size).to eq(2)
+          as = dsl.roles(:web).map { |server| "#{server.user}@#{server.hostname}:#{server.port}" }
+          expect(as.size).to eq(1)
+          expect(as[0]).to eq("deployer@example1.com:5678")
         end
 
         it 'roles defined using the `role` syntax are included' do
-          expect(dsl.roles(:app).size).to eq(2)
+          as = dsl.roles(:app).map { |server| "#{server.user}@#{server.hostname}:#{server.port}" }
+          expect(as.size).to eq(1)
+          expect(as[0]).to eq("deployer@example1.com:5678")
         end
       end
 
     end
 
+    describe 'when setting user and port' do
+      subject { dsl.roles(:all).map { |server| "#{server.user}@#{server.hostname}:#{server.port}" }.first }
+
+      describe "using the :user property" do
+        it "takes precedence over in the host string" do
+          dsl.server 'db@example1.com:1234', roles: %w{db}, active: true, user: 'brian'
+          expect(subject).to eq("brian@example1.com:1234")
+        end
+      end
+
+      describe "using the :port property" do
+        it "takes precedence over in the host string" do
+          dsl.server 'db@example1.com:9090', roles: %w{db}, active: true, port: 1234
+          expect(subject).to eq("db@example1.com:1234")
+        end
+      end
+    end
+
   end
 
   describe 'setting and fetching variables' do
@@ -538,6 +558,26 @@ describe Capistrano::DSL do
         recipient.doit(host, role, props)
       end
     end
+
+    it 'yields the merged properties for multiple roles' do
+      recipient = mock('recipient')
+      recipient.expects(:doit).with('example1.com', :redis, { port: 6379, type: :slave})
+      recipient.expects(:doit).with('example2.com', :redis, { port: 6379, type: :master})
+      recipient.expects(:doit).with('example1.com', :web, { port: 80 })
+      recipient.expects(:doit).with('example2.com', :web, { port: 81 })
+      dsl.role_properties(:redis, :web) do |host, role, props|
+        recipient.doit(host, role, props)
+      end
+    end
+
+    it 'honours a property filter before yielding' do
+      recipient = mock('recipient')
+      recipient.expects(:doit).with('example1.com', :redis, { port: 6379, type: :slave})
+      recipient.expects(:doit).with('example1.com', :web, { port: 80 })
+      dsl.role_properties(:redis, :web, select: :active) do |host, role, props|
+        recipient.doit(host, role, props)
+      end
+    end
   end
 
 end

spec/lib/capistrano/configuration/server_spec.rb

@@ -33,7 +33,7 @@ module Capistrano
       end
 
       describe 'comparing identity' do
-        subject { server.matches? Server[hostname] }
+        subject { server.hostname == Server[hostname].hostname }
 
         context 'with the same user, hostname and port' do
           let(:hostname) { 'root@hostname:1234' }
@@ -42,12 +42,12 @@ module Capistrano
 
         context 'with a different user' do
           let(:hostname) { 'deployer@hostname:1234' }
-          it { expect(subject).to be_falsey }
+          it { expect(subject).to be_truthy }
         end
 
         context 'with a different port' do
           let(:hostname) { 'root@hostname:5678' }
-          it { expect(subject).to be_falsey }
+          it { expect(subject).to be_truthy }
         end
 
         context 'with a different hostname' do
@@ -94,6 +94,10 @@ module Capistrano
           it 'sets the user' do
             expect(server.user).to eq 'tomc'
           end
+
+          it 'sets the netssh_options user' do
+            expect(server.netssh_options[:user]).to eq 'tomc'
+          end
         end
 
         context 'properties contains port' do
@@ -285,6 +289,9 @@ module Capistrano
           it 'contains correct user' do
             expect(server.netssh_options[:user]).to eq 'another_user'
           end
+          it 'does not affect server user in host' do
+            expect(server.user).to eq 'user_name'
+          end
           it 'contains keys' do
             expect(server.netssh_options[:keys]).to eq %w(/home/another_user/.ssh/id_rsa)
           end

spec/lib/capistrano/configuration/servers_spec.rb

@@ -18,6 +18,12 @@ module Capistrano
           expect(servers.count).to eq 1
         end
 
+        it 'handles de-duplification within roles with users' do
+          servers.add_role(:app, %w{1}, user: 'nick')
+          servers.add_role(:app, %w{1}, user: 'fred')
+          expect(servers.count).to eq 1
+        end
+
         it 'accepts instances of server objects' do
           servers.add_role(:app, [Capistrano::Configuration::Server.new('example.net'), 'example.com'])
           expect(servers.roles_for([:app]).length).to eq 2
@@ -134,7 +140,23 @@ module Capistrano
           servers.add_host('1', roles: [:app, 'web'], test: :value, user: 'root', port: 34)
           servers.add_host('1', roles: [:app, 'web'], test: :value, user: 'deployer', port: 34)
           servers.add_host('1', roles: [:app, 'web'], test: :value, user: 'deployer', port: 56)
-          expect(servers.count).to eq(8)
+          expect(servers.count).to eq(1)
+        end
+
+        describe "with a :user property" do
+
+          it 'sets the server ssh username' do
+            servers.add_host('1', roles: [:app, 'web'], user: 'nick')
+            expect(servers.count).to eq(1)
+            expect(servers.roles_for([:all]).first.user).to eq 'nick'
+          end
+
+          it 'overwrites the value of a user specified in the hostname' do
+            servers.add_host('brian@1', roles: [:app, 'web'], user: 'nick')
+            expect(servers.count).to eq(1)
+            expect(servers.roles_for([:all]).first.user).to eq 'nick'
+          end
+
         end
 
         it 'overwrites the value of a previously defined scalar property' do

spec/support/Vagrantfile

@@ -1,3 +1,5 @@
+require 'open-uri'
+
 Vagrant.configure("2") do |config|
 
   config.ssh.insert_key = false
@@ -8,6 +10,15 @@ Vagrant.configure("2") do |config|
       config.vm.box = 'hashicorp/precise64'
       config.vm.network "forwarded_port", guest: 22, host: "222#{i}".to_i
       config.vm.provision :shell, inline: 'sudo apt-get -y install git-core'
+
+      vagrantkey = open("https://raw.githubusercontent.com/mitchellh/vagrant/master/keys/vagrant.pub", "r",&:read)
+
+      config.vm.provision :shell,
+        inline: <<-INLINE
+          install -d -m 700 /root/.ssh
+          echo -e "#{vagrantkey}" > /root/.ssh/authorized_keys
+          chmod 0600 /root/.ssh/authorized_keys
+      INLINE
     end
   end
 end

spec/support/tasks/root.rake

@@ -0,0 +1,7 @@
+task :am_i_root do
+  on roles(:all) do |host|
+    host.user = 'root'
+    ident = capture :id, '-a'
+    info "I am #{ident}"
+  end
+end

spec/support/test_app.rb

@@ -13,7 +13,7 @@ module TestApp
       set :deploy_to, '#{deploy_to}'
       set :repo_url, 'git://github.com/capistrano/capistrano.git'
       set :branch, 'master'
-      set :ssh_options, { keys: "\#{ENV['HOME']}/.vagrant.d/insecure_private_key" }
+      set :ssh_options, { keys: "\#{ENV['HOME']}/.vagrant.d/insecure_private_key", auth_methods: ['publickey'] }
       server 'vagrant@localhost:2220', roles: %w{web app}
       set :linked_files, #{linked_files}
       set :linked_dirs, #{linked_dirs}