From 4dd8208290edbbab277b0175c8517dda5180f137 Mon Sep 17 00:00:00 2001
From: Donal McBreen <donal@basecamp.com>
Date: Tue, 8 Aug 2023 14:07:09 +0100
Subject: [PATCH] Extract versions that contains dashes

The version extraction assumed that the version is everything after the
last `-` in the container name. This doesn't work if you deploy a
non-MRSK generated version that contains a `-`.

To fix we'll generate the non version prefix and strip it off. In some
places for this to work we need to make sure to pass the role through.

Fixes: https://github.com/mrsked/mrsk/issues/402
---
 lib/mrsk/cli/app.rb       | 12 +++++++++---
 lib/mrsk/commands/app.rb  |  7 +++++--
 test/cli/app_test.rb      | 12 ++++++------
 test/cli/main_test.rb     |  4 ++--
 test/commands/app_test.rb |  6 +++---
 5 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/lib/mrsk/cli/app.rb b/lib/mrsk/cli/app.rb
index 40119d98b..629fdbcbb 100644
--- a/lib/mrsk/cli/app.rb
+++ b/lib/mrsk/cli/app.rb
@@ -97,7 +97,7 @@ def exec(cmd)
       say "Get most recent version available as an image...", :magenta unless options[:version]
       using_version(version_or_latest) do |version|
         say "Launching interactive command with version #{version} via SSH from new container on #{MRSK.primary_host}...", :magenta
-        run_locally { exec MRSK.app.execute_in_new_container_over_ssh(cmd, host: MRSK.primary_host) }
+        run_locally { exec MRSK.app(role: MRSK.primary_host.roles.first).execute_in_new_container_over_ssh(cmd, host: MRSK.primary_host) }
       end
 
     when options[:reuse]
@@ -249,7 +249,10 @@ def remove_images
 
   desc "version", "Show app version currently running on servers"
   def version
-    on(MRSK.hosts) { |host| puts_by_host host, capture_with_info(*MRSK.app.current_running_version).strip }
+    on(MRSK.hosts) do |host|
+      role = MRSK.roles_on(host).first
+      puts_by_host host, capture_with_info(*MRSK.app(role: role).current_running_version).strip
+    end
   end
 
   private
@@ -269,7 +272,10 @@ def using_version(new_version)
 
     def current_running_version(host: MRSK.primary_host)
       version = nil
-      on(host) { version = capture_with_info(*MRSK.app.current_running_version).strip }
+      on(host) do
+        role = MRSK.roles_on(host).first
+        version = capture_with_info(*MRSK.app(role: role).current_running_version).strip
+      end
       version.presence
     end
 
diff --git a/lib/mrsk/commands/app.rb b/lib/mrsk/commands/app.rb
index c9e287744..5d95a300b 100644
--- a/lib/mrsk/commands/app.rb
+++ b/lib/mrsk/commands/app.rb
@@ -112,8 +112,7 @@ def current_running_version
   def list_versions(*docker_args, statuses: nil)
     pipe \
       docker(:ps, *filter_args(statuses: statuses), *docker_args, "--format", '"{{.Names}}"'),
-      %(grep -oE "\\-[^-]+$"), # Extract SHA from "service-role-dest-SHA"
-      %(cut -c 2-)
+      %(while read line; do echo ${line##{service_role_dest}-}; done) # Extract SHA from "service-role-dest-SHA"
   end
 
   def list_containers
@@ -160,6 +159,10 @@ def filter_args(statuses: nil)
       argumentize "--filter", filters(statuses: statuses)
     end
 
+    def service_role_dest
+      [config.service, role, config.destination].compact.join("-")
+    end
+
     def filters(statuses: nil)
       [ "label=service=#{config.service}" ].tap do |filters|
         filters << "label=destination=#{config.destination}" if config.destination
diff --git a/test/cli/app_test.rb b/test/cli/app_test.rb
index ecc96d49c..828d7d489 100644
--- a/test/cli/app_test.rb
+++ b/test/cli/app_test.rb
@@ -22,7 +22,7 @@ class CliAppTest < CliTestCase
       .returns("running") # health check
 
     SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info)
-      .with(:docker, :ps, "--filter", "label=service=app", "--filter", "label=role=web", "--filter", "status=running", "--filter", "status=restarting", "--latest", "--format", "\"{{.Names}}\"", "|", "grep -oE \"\\-[^-]+$\"", "|", "cut -c 2-", raise_on_non_zero_exit: false)
+      .with(:docker, :ps, "--filter", "label=service=app", "--filter", "label=role=web", "--filter", "status=running", "--filter", "status=restarting", "--latest", "--format", "\"{{.Names}}\"", "|", "while read line; do echo ${line#app-web-}; done", raise_on_non_zero_exit: false)
       .returns("123") # old version
 
     run_command("boot").tap do |output|
@@ -71,7 +71,7 @@ class CliAppTest < CliTestCase
 
   test "stale_containers" do
     SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info)
-      .with(:docker, :ps, "--filter", "label=service=app", "--filter", "label=role=web", "--format", "\"{{.Names}}\"", "|", "grep -oE \"\\-[^-]+$\"", "|", "cut -c 2-", raise_on_non_zero_exit: false)
+      .with(:docker, :ps, "--filter", "label=service=app", "--filter", "label=role=web", "--format", "\"{{.Names}}\"", "|", "while read line; do echo ${line#app-web-}; done", raise_on_non_zero_exit: false)
       .returns("12345678\n87654321")
 
     run_command("stale_containers").tap do |output|
@@ -81,7 +81,7 @@ class CliAppTest < CliTestCase
 
   test "stop stale_containers" do
     SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info)
-      .with(:docker, :ps, "--filter", "label=service=app", "--filter", "label=role=web", "--format", "\"{{.Names}}\"", "|", "grep -oE \"\\-[^-]+$\"", "|", "cut -c 2-", raise_on_non_zero_exit: false)
+      .with(:docker, :ps, "--filter", "label=service=app", "--filter", "label=role=web", "--format", "\"{{.Names}}\"", "|", "while read line; do echo ${line#app-web-}; done", raise_on_non_zero_exit: false)
       .returns("12345678\n87654321")
 
     run_command("stale_containers", "--stop").tap do |output|
@@ -130,7 +130,7 @@ class CliAppTest < CliTestCase
 
   test "exec with reuse" do
     run_command("exec", "--reuse", "ruby -v").tap do |output|
-      assert_match "docker ps --filter label=service=app --filter status=running --filter status=restarting --latest --format \"{{.Names}}\" | grep -oE \"\\-[^-]+$\" | cut -c 2-", output # Get current version
+      assert_match "docker ps --filter label=service=app --filter label=role=web --filter status=running --filter status=restarting --latest --format \"{{.Names}}\" | while read line; do echo ${line#app-web-}; done", output # Get current version
       assert_match "docker exec app-web-999 ruby -v", output
     end
   end
@@ -163,14 +163,14 @@ class CliAppTest < CliTestCase
 
   test "version" do
     run_command("version").tap do |output|
-      assert_match "docker ps --filter label=service=app --filter status=running --filter status=restarting --latest --format \"{{.Names}}\" | grep -oE \"\\-[^-]+$\" | cut -c 2-", output
+      assert_match "docker ps --filter label=service=app --filter label=role=web --filter status=running --filter status=restarting --latest --format \"{{.Names}}\" | while read line; do echo ${line#app-web-}; done", output
     end
   end
 
 
   test "version through main" do
     stdouted { Mrsk::Cli::Main.start(["app", "version", "-c", "test/fixtures/deploy_with_accessories.yml", "--hosts", "1.1.1.1"]) }.tap do |output|
-      assert_match "docker ps --filter label=service=app --filter status=running --filter status=restarting --latest --format \"{{.Names}}\" | grep -oE \"\\-[^-]+$\" | cut -c 2-", output
+      assert_match "docker ps --filter label=service=app --filter label=role=web --filter status=running --filter status=restarting --latest --format \"{{.Names}}\" | while read line; do echo ${line#app-web-}; done", output
     end
   end
 
diff --git a/test/cli/main_test.rb b/test/cli/main_test.rb
index 730212ec5..f72ef34e7 100644
--- a/test/cli/main_test.rb
+++ b/test/cli/main_test.rb
@@ -178,7 +178,7 @@ class CliMainTest < CliTestCase
         .with(:docker, :container, :ls, "--all", "--filter", "name=^app-#{role}-123$", "--quiet")
         .returns("version-to-rollback\n").at_least_once
       SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info)
-        .with(:docker, :ps, "--filter", "label=service=app", "--filter", "label=role=#{role}", "--filter", "status=running", "--filter", "status=restarting", "--latest", "--format", "\"{{.Names}}\"", "|", "grep -oE \"\\-[^-]+$\"", "|", "cut -c 2-", raise_on_non_zero_exit: false)
+        .with(:docker, :ps, "--filter", "label=service=app", "--filter", "label=role=#{role}", "--filter", "status=running", "--filter", "status=restarting", "--latest", "--format", "\"{{.Names}}\"", "|", "while read line; do echo ${line#app-#{role}-}; done", raise_on_non_zero_exit: false)
         .returns("version-to-rollback\n").at_least_once
       SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info)
         .with(:docker, :container, :ls, "--all", "--filter", "name=^app-#{role}-123$", "--quiet", "|", :xargs, :docker, :inspect, "--format", "'{{if .State.Health}}{{.State.Health.Status}}{{else}}{{.State.Status}}{{end}}'")
@@ -207,7 +207,7 @@ class CliMainTest < CliTestCase
       .with(:docker, :container, :ls, "--filter", "name=^app-web-123$", "--quiet", raise_on_non_zero_exit: false)
       .returns("").at_least_once
     SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info)
-      .with(:docker, :ps, "--filter", "label=service=app", "--filter", "label=role=web", "--filter", "status=running", "--filter", "status=restarting", "--latest", "--format", "\"{{.Names}}\"", "|", "grep -oE \"\\-[^-]+$\"", "|", "cut -c 2-", raise_on_non_zero_exit: false)
+      .with(:docker, :ps, "--filter", "label=service=app", "--filter", "label=role=web", "--filter", "status=running", "--filter", "status=restarting", "--latest", "--format", "\"{{.Names}}\"", "|", "while read line; do echo ${line#app-web-}; done", raise_on_non_zero_exit: false)
       .returns("").at_least_once
     SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info)
       .with(:docker, :container, :ls, "--all", "--filter", "name=^app-web-123$", "--quiet", "|", :xargs, :docker, :inspect, "--format", "'{{if .State.Health}}{{.State.Health.Status}}{{else}}{{.State.Status}}{{end}}'")
diff --git a/test/commands/app_test.rb b/test/commands/app_test.rb
index 57db2eaa5..0451f53b7 100644
--- a/test/commands/app_test.rb
+++ b/test/commands/app_test.rb
@@ -250,17 +250,17 @@ class CommandsAppTest < ActiveSupport::TestCase
 
   test "current_running_version" do
     assert_equal \
-      "docker ps --filter label=service=app --filter label=role=web --filter status=running --filter status=restarting --latest --format \"{{.Names}}\" | grep -oE \"\\-[^-]+$\" | cut -c 2-",
+      "docker ps --filter label=service=app --filter label=role=web --filter status=running --filter status=restarting --latest --format \"{{.Names}}\" | while read line; do echo ${line#app-web-}; done",
       new_command.current_running_version.join(" ")
   end
 
   test "list_versions" do
     assert_equal \
-      "docker ps --filter label=service=app --filter label=role=web --format \"{{.Names}}\" | grep -oE \"\\-[^-]+$\" | cut -c 2-",
+      "docker ps --filter label=service=app --filter label=role=web --format \"{{.Names}}\" | while read line; do echo ${line#app-web-}; done",
       new_command.list_versions.join(" ")
 
     assert_equal \
-      "docker ps --filter label=service=app --filter label=role=web --filter status=running --filter status=restarting --latest --format \"{{.Names}}\" | grep -oE \"\\-[^-]+$\" | cut -c 2-",
+      "docker ps --filter label=service=app --filter label=role=web --filter status=running --filter status=restarting --latest --format \"{{.Names}}\" | while read line; do echo ${line#app-web-}; done",
       new_command.list_versions("--latest", statuses: [ :running, :restarting ]).join(" ")
   end