From ede09c93e80155716ebf2ef0f2e7084a1521b55e Mon Sep 17 00:00:00 2001 From: Nick Townsend Date: Wed, 24 Sep 2014 20:12:14 -0700 Subject: [PATCH] Treat all host/role filtering options the same MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Command line —host and —role options Environment variables ROLES and HOSTS Filter variable hash keys :role and :host --- CHANGELOG.md | 25 ++++-- README.md | 78 ++++++------------ lib/capistrano/all.rb | 7 -- lib/capistrano/application.rb | 13 +-- lib/capistrano/configuration.rb | 24 ++++-- lib/capistrano/configuration/servers.rb | 6 +- lib/capistrano/dsl.rb | 9 ++- spec/integration/dsl_spec.rb | 80 ++++++++++++++++--- .../capistrano/configuration/servers_spec.rb | 4 +- 9 files changed, 146 insertions(+), 100 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 669dd718..6c5295ed 100644 --- a/CHANGELOG.md +++ b/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()` - and `primary()` methods. - * Host and Role filtering now supports Regular expressions - * See the README.md file for a comprehensive discussion of these changes + * This applies to filters defined via the command line, the environment + and the :filter variable. + * Filtering now supports Regular expressions + * 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) diff --git a/README.md b/README.md index 7c204c00..99c3db3d 100644 --- a/README.md +++ b/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()` method (and relatives) outside the scope of `on()` +* To determine _configurations_: typically by using the `roles()`, `release_roles()` and + `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 -automate the setup of an F5 pool. +* To _interact_ with remote hosts using the `on()` method + +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 -files deployed will not be the same as those on the hosts excluded by the filters. On the -other hand if the filter applies only to the _command_ aspect, then any configuration -files deployed will be identical across the stage. +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. This is +almost certainly not what is wanted, the filters should apply only to the _interactions_ +ensuring that any configuration files deployed will be identical across the stage. -Consider also the different ways in which filters may be specified. Externally: -* Via environment variables HOSTS and ROLES -* Via command line options `--hosts` and `--roles` -And internally: -* Via the `:filter` variable -* Via options passed to the `roles()` method (and implicitly in methods like `release_roles()`) +Another type of filtering is done by defining properties on servers and selecting on that +basis. An example of that is the 'no_release' property and it's use in the +`release_roles()` method. To distinguish these two types of filtering we name them: -Currently, when a filter is applied via __any__ of the current methods, it -affects __both__ the _command_ and the _configuration_ aspects. +* On-Filtering + 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 -_union_ of all of the components, so an implicit OR is between each one. However when -multiple filters are declared, they are evaluated in the order declared and so are ANDed -together. The order of processing is: - -* Environment variables, -* Command line options, -* The `:filter` variable value in effect at the time of the `on()` call +When filters are specified using comma separated lists, the final filter is the _union_ of +all of the components. However when multiple filters are declared the result is the +_intersection_. ## SSHKit diff --git a/lib/capistrano/all.rb b/lib/capistrano/all.rb index f9db2fb9..334d2a79 100644 --- a/lib/capistrano/all.rb +++ b/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' diff --git a/lib/capistrano/application.rb b/lib/capistrano/application.rb index af04a253..acf7a059 100644 --- a/lib/capistrano/application.rb +++ b/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 diff --git a/lib/capistrano/configuration.rb b/lib/capistrano/configuration.rb index da76371a..ab439e9b 100644 --- a/lib/capistrano/configuration.rb +++ b/lib/capistrano/configuration.rb @@ -87,16 +87,30 @@ module Capistrano @timestamp ||= Time.now.utc end - def add_external_filter(type, values) - external_filters << Filter.new(type, values) + def setup_filters + @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) diff --git a/lib/capistrano/configuration/servers.rb b/lib/capistrano/configuration/servers.rb index 43907539..e2935a37 100644 --- a/lib/capistrano/configuration/servers.rb +++ b/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)) - 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 = Filter.new(:role, names).filter(servers) s.select { |server| server.select?(options) } end diff --git a/lib/capistrano/dsl.rb b/lib/capistrano/dsl.rb index f84ebe62..6b1acb30 100644 --- a/lib/capistrano/dsl.rb +++ b/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 diff --git a/spec/integration/dsl_spec.rb b/spec/integration/dsl_spec.rb index a376311c..5ff772d4 100644 --- a/spec/integration/dsl_spec.rb +++ b/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 diff --git a/spec/lib/capistrano/configuration/servers_spec.rb b/spec/lib/capistrano/configuration/servers_spec.rb index 99170b90..09f8caa2 100644 --- a/spec/lib/capistrano/configuration/servers_spec.rb +++ b/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