refactor property setting, host adding

lib/capistrano/configuration/server.rb

@@ -3,6 +3,10 @@ module Capistrano
   class Configuration
     class Server < SSHKit::Host
 
+      def add_roles(roles)
+        Array(roles).each { |role| add_role(role) }
+      end
+
       def add_role(role)
         roles << role.to_sym
       end
@@ -19,6 +23,25 @@ module Capistrano
         properties.roles ||= Set.new
       end
 
+      def primary
+        self if properties.primary
+      end
+
+      def with(properties)
+        properties.each { |property, value| add_property(property, value) }
+        self
+      end
+
+      private
+
+      def add_property(property, value)
+        if property.to_sym == :role
+          add_roles(value)
+        else
+          properties.send(:"#{property}=", value) unless properties.respond_to?(property)
+        end
+      end
+
     end
   end
 end

lib/capistrano/configuration/servers.rb

@@ -4,26 +4,12 @@ module Capistrano
     class Servers
       include Enumerable
 
-      def add_host(host, properties = {})
-        find_or_create_server(host).tap do |host|
-          Array(properties.delete(:roles) || properties.delete("roles")).each do |role|
-            host.add_role(role)
-          end
-          properties.each do |key, value|
-            unless host.properties.respond_to?(key)
-              host.properties.send(:"#{key}=", value)
-            end
-          end
-          servers.add host
-        end
+      def add_host(host, properties={})
+        servers.add server(host).with(properties)
       end
 
       def add_role(role, hosts)
-        Array(hosts).each do |host|
-          server = find_or_create_server(host)
-          server.add_role(role)
-          servers.add server
-        end
+        Array(hosts).each { |host| add_host(host, role: role) }
       end
 
       def fetch_roles(names)
@@ -31,7 +17,8 @@ module Capistrano
       end
 
       def fetch_primary(role)
-        fetch(role).select { |h| h.properties.primary }.first || fetch(role).first
+        hosts = fetch(role)
+        hosts.find(&:primary) || hosts.first
       end
 
       def each
@@ -40,12 +27,12 @@ module Capistrano
 
       private
 
-      def find_or_create_server(host)
+      def server(host)
         servers.find { |server| server.matches?(host) } || Server.new(host)
       end
 
-      def fetch(name)
-        servers.find_all { |server| server.has_role? name }
+      def fetch(role)
+        servers.find_all { |server| server.has_role? role}
       end
 
       def roles_for(names)

spec/lib/capistrano/configuration/server_spec.rb

@@ -12,6 +12,14 @@ module Capistrano
         end
       end
 
+      describe 'adding roles' do
+        subject { server.add_roles([:things, :stuff]) }
+        it 'adds the roles' do
+          expect{subject}.to change{server.roles.size}.from(0).to(2)
+        end
+      end
+
+
       describe 'checking roles' do
         subject { server.has_role?(:test) }
 
@@ -43,6 +51,59 @@ module Capistrano
         end
       end
 
+      describe 'identifying as primary' do
+        subject { server.primary }
+        context 'server is primary' do
+          before do
+            server.properties.primary = true
+          end
+          it 'returns self' do
+            expect(subject).to eq server
+          end
+        end
+
+        context 'server is not primary' do
+          it 'is falesy' do
+            expect(subject).to be_false
+          end
+        end
+      end
+
+      describe 'assigning properties' do
+
+        before do
+          server.with(properties)
+        end
+
+        context 'properties contains roles' do
+          let(:properties) { {roles: [:clouds]} }
+
+          it 'adds the roles' do
+            expect(server.roles.first).to eq :clouds
+          end
+        end
+
+        context 'new properties' do
+          let(:properties) { { webscales: 5 } }
+
+          it 'adds the properties' do
+            expect(server.properties.webscales).to eq 5
+          end
+        end
+
+        context 'existing properties' do
+          let(:properties) { { webscales: 6 } }
+
+          before do
+            server.properties.webscales = 5
+          end
+
+          it 'does keeps the existing properties' do
+            expect(server.properties.webscales).to eq 5
+          end
+        end
+      end
+
     end
   end
 end

spec/lib/capistrano/configuration/servers_spec.rb

@@ -41,10 +41,11 @@ module Capistrano
       end
 
       describe 'finding the primary server' do
-        it 'takes the first server for if none have the primary property' do
+        it 'takes the first server if none have the primary property' do
           servers.add_role(:app, %w{1 2})
           servers.fetch_primary(:app).hostname.should == "1"
         end
+
         it 'takes the first server with the primary have the primary flag' do
           servers.add_role(:app, %w{1 2})
           servers.add_host('2', primary: true)