Treat all host/role filtering options the same

Command line —host and —role options
Environment variables ROLES and HOSTS
Filter variable hash keys :role and :host

CHANGELOG.md

@@ -7,12 +7,27 @@ Reverse Chronological Order:
 https://github.com/capistrano/capistrano/compare/v3.2.1...HEAD
 
 * Enhancements (@townsen)
-  * _External_ Host and Role filtering now affects only `on()` commands
+  * Previously filtering would affect any generated configuration files so that
+    files newly deployed would not be the same as those on the hosts previously
+    deployed (and now excluded by filters). This is almost certainly not what is
+    wanted: the filters should apply only to the on() method and thus any
+    configuration files deployed will be identical across the set of servers
+    making up the stage.
+  * Host and Role filtering now affects only `on()` commands
     and not the `roles()`, `release_roles()` and `primary()` methods.
-  * _Internal_ Host and Role filtering affects the `roles()`, `release_roles()`
+  * This applies to filters defined via the command line, the environment
-    and `primary()` methods.
+    and the :filter variable.
-  * Host and Role filtering now supports Regular expressions
+  * Filtering now supports Regular expressions
-  * See the README.md file for a comprehensive discussion of these changes
+  * This change _could_ cause existing scripts that use filtering and depend on
+    the old behaviour to fail, though it is unlikely. Users who rely on
+    filtering should check that generated configuration files are correct, and
+    where not introduce server properties to do the filtering. For example, if a
+    filter was used to specify an active subset of servers (by hostname), it should
+    be removed and replaced with an 'active' property (set to true or false) on the
+    server definitions. This keeps the stage file as the canonical model of the
+    deployment environment.
+
+  * See the documentation in the README.md file
 
 * Pushing again to trigger another build (I have a seemingly random build fail) (@townsen)
 * Enhancements (@townsen)

README.md

@@ -272,58 +272,37 @@ __Support removed__ for following variables:
 Capistrano enables the declaration of servers and roles, each of which may have properties
 associated with them.  Tasks are then able to use these definitions in two distinct ways:
 
-* To execute commands on remote hosts: using the `on()` method (provided by SSHKit), and
+* To determine _configurations_: typically by using the `roles()`, `release_roles()` and
-* To determine configurations: typically by using the `roles()` method (and relatives) outside the scope of `on()`
+  `primary()` methods. Typically these are used outside the scope of the `on()` method.
 
-An example of the latter would be to create a list of available web servers in order to
+* To _interact_ with remote hosts using the `on()` method
-automate the setup of an F5 pool.
+
+An example of the two would be to create a `/etc/krb5.conf' file containing the list of
+available KDC's by using the list of servers returned by `roles(:kdc)` and then uploading
+it to all client machines using `on(roles(:all)) do upload!(file) end`
 
 A problem with this arises when _filters_ are used. Filters are designed to limit the
 actual set of hosts that are used to a subset of those in the overall stage, but how
-should that apply to the above?
+should that apply in the above case?
 
-If the filter applies to both the _command_ and _configuration_ aspects, any configuration
+If the filter applies to both the _interaction_ and _configuration_ aspects, any configuration
-files deployed will not be the same as those on the hosts excluded by the filters. On the
+files deployed will not be the same as those on the hosts excluded by the filters. This is
-other hand if the filter applies only to the _command_ aspect, then any configuration
+almost certainly not what is wanted, the filters should apply only to the _interactions_
-files deployed will be identical across the stage.
+ensuring that any configuration files deployed will be identical across the stage.
 
-Consider also the different ways in which filters may be specified. Externally:
+Another type of filtering is done by defining properties on servers and selecting on that
-* Via environment variables HOSTS and ROLES
+basis. An example of that is the 'no_release' property and it's use in the
-* Via command line options `--hosts` and `--roles`
+`release_roles()` method. To distinguish these two types of filtering we name them:
-And internally:
-* Via the `:filter` variable
-* Via options passed to the `roles()` method (and implicitly in methods like `release_roles()`)
 
-Currently, when a filter is applied via __any__ of the current methods, it
+* On-Filtering
-affects __both__ the _command_ and the _configuration_ aspects.
+    Specified in the following ways:
+    * Via environment variables HOSTS and ROLES
+    * Via command line options `--hosts` and `--roles`
+    * Via the `:filter` variable set in a stage file
+* Property-Filtering
+    These are specified by options passed to the `roles()` method (and implicitly in methods
+    like `release_roles()` and `primary()`)
 
-For practical uses this behaviour needs refining.  A core principle of Capistrano is that
-the stage file is the complete embodiment of the configuration (possibly including
-settings from deploy.rb and Capfile), and therefore any filtering of the configuration
-should be declared there. Put the other way, an external filter, done for the purposes of
-limiting which hosts commands are executed on, should not affect the overall
-configuration.
-
-So this fix makes the external filters only apply to commands issued: ie. they restrict
-the hosts that an `on()` method will use, the will not affect the `roles()` method. On the
-other hand internal filters will always apply
-
-By making this distinction two distinct usage models can be catered for. When a subset of
-servers and/or roles are deployed to:
-
-* With a configuration that reflects only that subset. This can be achieved by
-  using the `set :filter, hosts: [a,b], roles: [:web,:app]` approach.
-
-* With a configuration that reflects the entire stage. This is useful when deploying a new
-  server to an existing configuration. This can be achieved by using either of the
-  external filtering methods.
-
-The former corresponds to the existing behaviour, although done slightly differently.
-The latter is new behaviour which is a more common use case.
-
-We also change external filters so that they can use regular expressions. If either
-a host or role name in a filter doesn't match `/^[-\w.]*$/` then it's assumed to be
-a regular expression.
 To increase the utility of On-Filters they can use regular expressions:
 * If the host name in a filter doesn't match `/^[-A-Za-z0-9.]+$/` (the set of valid characters
     for a DNS name) then it's assumed to be a regular expression.
@@ -331,14 +310,9 @@ To increase the utility of On-Filters they can use regular expressions:
     of them to be specified on one line we use the comma. To use a regexp for a role filter begin
     and end the string with '/'. These may not contain a comma.
 
-When multiple filters are specified in the same declaration, the final filter is the
+When filters are specified using comma separated lists, the final filter is the _union_ of
-_union_ of all of the components, so an implicit OR is between each one. However when
+all of the components. However when multiple filters are declared the result is the
-multiple filters are declared, they are evaluated in the order declared and so are ANDed
+_intersection_. 
-together. The order of processing is:
-
-* Environment variables,
-* Command line options,
-* The `:filter` variable value in effect at the time of the `on()` call
 
 ## SSHKit
 

lib/capistrano/all.rb

@@ -1,12 +1,5 @@
 require 'rake'
 require 'sshkit'
-require 'sshkit/dsl'
-
-module SSHKit
-  module DSL
-    alias_method :sshkit_on, :on
-  end
-end
 
 require 'io/console'
 

lib/capistrano/application.rb

@@ -40,11 +40,6 @@ module Capistrano
         opts.separator "Invoke (or simulate invoking) a task:"
         opts.separator "    bundle exec cap [--dry-run] STAGE TASK"
         opts.separator ""
-        opts.separator "Host and Role Filters:"
-        opts.separator "    Host and role patterns may be specified as a comma separated list"
-        opts.separator "    each item of which is treated either as a literal match (if it contains"
-        opts.separator "    just the characters A-Za-z0-9-_.) or a regular expression (otherwise)."
-        opts.separator ""
         opts.separator "Advanced options:"
 
         opts.on_tail("-h", "--help", "-H", "Display this help message.") do
@@ -112,18 +107,18 @@ module Capistrano
 
     def roles
       ['--roles ROLES', '-r',
-       "Run SSH commands only on hosts matching these roles (see syntax above)",
+       "Run SSH commands only on hosts matching these roles",
        lambda { |value|
-         Configuration.env.add_external_filter(:role, value.split(","))
+         Configuration.env.add_cmdline_filter(:role, value)
        }
       ]
     end
 
     def hostfilter
       ['--hosts HOSTS', '-z',
-       "Run SSH commands only on matching hosts (see syntax above)",
+       "Run SSH commands only on matching hosts",
        lambda { |value|
-         Configuration.env.add_external_filter(:host, value.split(","))
+         Configuration.env.add_cmdline_filter(:host, value)
        }
       ]
     end

lib/capistrano/configuration.rb

@@ -87,16 +87,30 @@ module Capistrano
       @timestamp ||= Time.now.utc
     end
 
-    def add_external_filter(type, values)
+    def setup_filters
-      external_filters << Filter.new(type, values)
+      @filters = cmdline_filters.clone
+      @filters << Filter.new(:role, ENV['ROLES']) if ENV['ROLES']
+      @filters << Filter.new(:host, ENV['HOSTS']) if ENV['HOSTS']
+      fh = fetch_for(:filter,{})
+      @filters << Filter.new(:host, fh[:host]) if fh[:host]
+      @filters << Filter.new(:role, fh[:role]) if fh[:role]
+    end
+
+    def add_cmdline_filter(type, values)
+      cmdline_filters << Filter.new(type, values)
     end
 
     def filter list
-      external_filters.reduce(list){|l,f| f.filter list}
+      setup_filters if @filters.nil?
+      @filters.reduce(list) { |l,f| f.filter l }
     end
 
     private
 
+    def cmdline_filters
+      @cmdline_filters ||= []
+    end
+
     def servers
       @servers ||= Servers.new
     end
@@ -105,10 +119,6 @@ module Capistrano
       @config ||= Hash.new
     end
 
-    def external_filters
-      @external_filters ||= []
-    end
-
     def fetch_for(key, default, &block)
       if block_given?
         config.fetch(key, &block)

lib/capistrano/configuration/servers.rb

@@ -18,11 +18,7 @@ module Capistrano
 
       def roles_for(names)
         options = extract_options(names)
-        fia = Array(Filter.new(:role, names))
+        s = Filter.new(:role, names).filter(servers)
-        fs = Configuration.env.fetch(:filter,{})
-        fia << Filter.new(:host, fs[:host]) if fs[:host]
-        fia << Filter.new(:role, fs[:role]) if fs[:role]
-        s = fia.reduce(servers){|m,o| o.filter m}
         s.select { |server| server.select?(options) }
       end
 

lib/capistrano/dsl.rb

@@ -3,6 +3,7 @@ require 'capistrano/dsl/task_enhancements'
 require 'capistrano/dsl/paths'
 require 'capistrano/dsl/stages'
 require 'capistrano/dsl/env'
+require 'capistrano/configuration/filter'
 
 module Capistrano
   module DSL
@@ -49,9 +50,13 @@ module Capistrano
       VersionValidator.new(locked_version).verify
     end
 
-    # Having intercepted the SSHKit on() method we can filter externally
     def on(hosts, options={}, &block)
-      sshkit_on(Configuration.env.filter(hosts), options, &block)
+      subset = Configuration.env.filter hosts
+      SSHKit::Coordinator.new(subset).each(options, &block)
+    end
+
+    def run_locally(&block)
+      SSHKit::Backend::Local.new(&block).run
     end
 
   end

spec/integration/dsl_spec.rb

@@ -36,10 +36,10 @@ describe Capistrano::DSL do
           end
         end
 
-        context 'with filter options' do
+        context 'with property filter options' do
           subject { dsl.release_roles(:all, filter: :active) }
 
-          it 'returns all release servers that match the filter' do
+          it 'returns all release servers that match the property filter' do
             expect(subject.map(&:hostname)).to eq %w{example1.com example3.com}
           end
         end
@@ -92,7 +92,7 @@ describe Capistrano::DSL do
           end
         end
 
-        context 'when the attribute `primary` is explicity set' do
+        context 'when the attribute `primary` is explicitly set' do
           subject { dsl.primary(:app) }
           it 'returns the servers' do
             expect(subject.hostname).to eq 'example4.com'
@@ -102,35 +102,36 @@ describe Capistrano::DSL do
 
       describe 'setting an internal host filter' do
         subject { dsl.roles(:app) }
-        it 'returns one' do
+        it 'is ignored' do
           dsl.set :filter, { host: 'example3.com' }
-          expect(subject.map(&:hostname)).to eq(['example3.com'])
+          expect(subject.map(&:hostname)).to eq(['example3.com', 'example4.com'])
         end
       end
 
       describe 'setting an internal role filter' do
         subject { dsl.roles(:app) }
-        it 'returns one' do
+        it 'ignores it' do
           dsl.set :filter, { role: :web }
-          expect(subject.map(&:hostname)).to eq(['example3.com'])
+          expect(subject.map(&:hostname)).to eq(['example3.com','example4.com'])
         end
       end
 
       describe 'setting an internal host and role filter' do
         subject { dsl.roles(:app) }
-        it 'returns one' do
+        it 'ignores it' do
           dsl.set :filter, { role: :web, host: 'example1.com' }
-          expect(subject.map(&:hostname)).to be_empty
+          expect(subject.map(&:hostname)).to eq(['example3.com','example4.com'])
         end
       end
 
       describe 'setting an internal regexp host filter' do
         subject { dsl.roles(:all) }
-        it 'works' do
+        it 'is ignored' do
           dsl.set :filter, { host: /1/ }
-          expect(subject.map(&:hostname)).to eq(['example1.com'])
+          expect(subject.map(&:hostname)).to eq(%w{example1.com example2.com example3.com example4.com example5.com})
         end
       end
+
     end
 
     describe 'when defining role with reserved name' do
@@ -549,4 +550,61 @@ describe Capistrano::DSL do
       end
     end
   end
+
+  describe 'on()' do
+
+    before do
+      dsl.server 'example1.com', roles: %w{web}, active: true
+      dsl.server 'example2.com', roles: %w{web}
+      dsl.server 'example3.com', roles: %w{app web}, active: true
+      dsl.server 'example4.com', roles: %w{app}, primary: true
+      dsl.server 'example5.com', roles: %w{db}, no_release: true
+      @coordinator = mock('coordinator')
+      @coordinator.expects(:each).returns(nil)
+      ENV.delete 'ROLES'
+      ENV.delete 'HOSTS'
+
+    end
+
+    it 'filters by role from the :filter variable' do
+      hosts = dsl.roles(:web)
+      all = dsl.roles(:all)
+      SSHKit::Coordinator.expects(:new).with(hosts).returns(@coordinator)
+      dsl.set :filter, { role: 'web' }
+      dsl.on(all)
+    end
+
+    it 'filters by host and role from the :filter variable' do
+      all = dsl.roles(:all)
+      SSHKit::Coordinator.expects(:new).with([]).returns(@coordinator)
+      dsl.set :filter, { role: 'db', host: 'example3.com' }
+      dsl.on(all)
+    end
+
+    it 'filters from ENV[ROLES]' do
+      hosts = dsl.roles(:db)
+      all = dsl.roles(:all)
+      SSHKit::Coordinator.expects(:new).with(hosts).returns(@coordinator)
+      ENV['ROLES'] = 'db'
+      dsl.on(all)
+    end
+
+    it 'filters from ENV[HOSTS]' do
+      hosts = dsl.roles(:db)
+      all = dsl.roles(:all)
+      SSHKit::Coordinator.expects(:new).with(hosts).returns(@coordinator)
+      ENV['HOSTS'] = 'example5.com'
+      dsl.on(all)
+    end
+
+    it 'filters by ENV[HOSTS] && ENV[ROLES]' do
+      all = dsl.roles(:all)
+      SSHKit::Coordinator.expects(:new).with([]).returns(@coordinator)
+      ENV['HOSTS'] = 'example5.com'
+      ENV['ROLES'] = 'web'
+      dsl.on(all)
+    end
+
+  end
+
 end

spec/lib/capistrano/configuration/servers_spec.rb

@@ -82,11 +82,11 @@ module Capistrano
           expect(servers.fetch_primary(:app).hostname).to eq('2')
         end
 
-        it 'honours any internal filters' do
+        it 'ignores any on_filters' do
           Configuration.env.set :filter, { host: '1'}
           servers.add_role(:app, %w{1 2})
           servers.add_host('2', primary: true)
-          expect(servers.fetch_primary(:app).hostname).to eq('1')
+          expect(servers.fetch_primary(:app).hostname).to eq('2')
         end
       end