deploy:cleanup should skip only the invalid releases and continue the rotation of releases (#1899)

* deploy:cleanup should skip only the invalid releases and continue the rotation
of releases

* Add cucumber tests for deploy:cleanup

* PR feedback

* rebase on master

* add ruby 2.4.1 to specs

* vagrant works with bundler 1.15.3

* remove unnecessary command

.travis.yml

@@ -1,5 +1,6 @@
 language: ruby
 rvm:
+  - 2.4.1
   - 2.3.1
   - 2.2
   - 2.1

CHANGELOG.md

@@ -20,6 +20,8 @@ https://github.com/capistrano/capistrano/compare/v3.8.1...HEAD
 
 * Your contribution here!
 
+* Updated `deploy:cleanup` to continue rotating the releases and skip the invalid directory names instead of skipping the whole rotation of releases. The warning message has changed slightly due to the change of behavior.
+
 ## `3.8.2` (2017-06-16)
 
 https://github.com/capistrano/capistrano/compare/v3.8.1...v3.8.2

DEVELOPMENT.md

@@ -28,11 +28,10 @@ Capistrano is a Ruby project, so we expect you to have a functioning Ruby enviro
 
 Make sure to install:
 
-* [Bundler](https://bundler.io/) 1.10.5 (see note)
+* [Bundler](https://bundler.io/)
 * [Vagrant](https://www.vagrantup.com/)
 * [VirtualBox](https://www.virtualbox.org/wiki/Downloads) (or another [Vagrant-supported](https://docs.vagrantup.com/v2/getting-started/providers.html) VM host)
 
-*Note: As of this writing (December 2015), Vagrant does not work with Bundler > 1.10.5. If you have multiple versions of Bundler installed, use the special underscore command-line argument to force a compatible version, like this: `bundle _1.10.5_ exec rake features`.*
 
 ### Running tests
 
@@ -45,7 +44,7 @@ $ bundle install
 # Run the RSpec suite
 $ bundle exec rake spec
 
-# Run the Cucumber suite (requires Bundler 1.10.5)
+# Run the Cucumber suite
 $ bundle exec rake features
 
 # Run the Cucumber suite and leave the VM running (faster for subsequent runs)

features/deploy.feature

@@ -52,6 +52,14 @@ Feature: Deploy
     When I run cap "deploy:symlink:release"
     Then the current directory will be a symlink to the release
 
+  Scenario: Cleanup
+    Given config stage file has line "set :keep_releases, 3"
+    And 5 valid existing releases
+    And an invalid release named "new"
+    When I run cap "deploy:cleanup"
+    Then 3 valid releases are kept
+    And the invalid "new" release is ignored
+
   Scenario: Rolling Back
     Given I make 2 deployments
     When I run cap "deploy:rollback"
@@ -61,3 +69,4 @@ Feature: Deploy
     Given I make 3 deployments
     When I rollback to a specific release
     Then the current symlink points to that specific release
+

features/step_definitions/assertions.rb

@@ -19,6 +19,18 @@ Then(/^the releases path is created$/) do
   run_vagrant_command(test_dir_exists(TestApp.releases_path))
 end
 
+Then(/^(\d+) valid releases are kept/) do |num|
+  test = %Q([ $(ls -g #{TestApp.releases_path} | grep -E '[0-9]{14}' | wc -l) == "#{num}" ])
+  _, _, status = vagrant_cli_command("ssh -c #{test.shellescape}")
+  expect(status).to be_success
+end
+
+Then(/^the invalid (.+) release is ignored$/) do |filename|
+  test = "ls -g #{TestApp.releases_path} | grep #{filename}"
+  _, _, status = vagrant_cli_command("ssh -c #{test.shellescape}")
+  expect(status).to be_success
+end
+
 Then(/^directories in :linked_dirs are created in shared$/) do
   TestApp.linked_dirs.each do |dir|
     run_vagrant_command(test_dir_exists(TestApp.shared_path.join(dir)))

features/step_definitions/setup.rb

@@ -77,3 +77,16 @@ Given(/^I make (\d+) deployments$/) do |count|
     stdout.strip
   end
 end
+
+Given(/^(\d+) valid existing releases$/) do |num|
+  a_day = 86_400 # in seconds
+  offset = -(a_day * num.to_i)
+  num.to_i.times do
+    run_vagrant_command("mkdir -p #{TestApp.release_path(TestApp.timestamp(offset))}")
+    offset += a_day
+  end
+end
+
+Given(/^an invalid release named "(.+)"$/) do |filename|
+  run_vagrant_command("mkdir -p #{TestApp.release_path(filename)}")
+end

lib/capistrano/i18n.rb

@@ -13,7 +13,7 @@ en = {
   question: "Please enter %{key}: ",
   question_default: "Please enter %{key} (%{default_value}): ",
   keeping_releases: "Keeping %{keep_releases} of %{releases} deployed releases on %{host}",
-  skip_cleanup: "Skipping cleanup of old releases on %{host}; unexpected foldername found (should be timestamp)",
+  skip_cleanup: "Skipping cleanup of invalid releases on %{host}; unexpected foldername found (should be timestamp)",
   no_old_releases: "No old releases (keeping newest %{keep_releases}) on %{host}",
   linked_file_does_not_exist: "linked file %{file} does not exist on %{host}",
   cannot_rollback: "There are no older releases to rollback to",

lib/capistrano/tasks/deploy.rake

@@ -149,11 +149,13 @@ namespace :deploy do
   task :cleanup do
     on release_roles :all do |host|
       releases = capture(:ls, "-x", releases_path).split
-      if !(releases.all? { |e| /^\d{14}$/ =~ e })
-        warn t(:skip_cleanup, host: host.to_s)
-      elsif releases.count >= fetch(:keep_releases)
-        info t(:keeping_releases, host: host.to_s, keep_releases: fetch(:keep_releases), releases: releases.count)
-        directories = (releases - releases.last(fetch(:keep_releases)))
+      valid, invalid = releases.partition { |e| /^\d{14}$/ =~ e }
+
+      warn t(:skip_cleanup, host: host.to_s) if invalid.any?
+
+      if valid.count >= fetch(:keep_releases)
+        info t(:keeping_releases, host: host.to_s, keep_releases: fetch(:keep_releases), releases: valid.count)
+        directories = (valid - valid.last(fetch(:keep_releases)))
         if directories.any?
           directories_str = directories.map do |release|
             releases_path.join(release)

spec/support/test_app.rb

@@ -132,12 +132,12 @@ module TestApp
     deploy_to.join("releases")
   end
 
-  def release_path
-    releases_path.join(timestamp)
+  def release_path(t=timestamp)
+    releases_path.join(t)
   end
 
-  def timestamp
-    Time.now.utc.strftime("%Y%m%d%H%M%S")
+  def timestamp(offset=0)
+    (Time.now.utc + offset).strftime("%Y%m%d%H%M%S")
   end
 
   def repo_path