From 4966d529193707e138e98291fa09cf635d89b815 Mon Sep 17 00:00:00 2001
From: Donal McBreen <donal@basecamp.com>
Date: Fri, 8 Mar 2024 08:44:35 +0000
Subject: [PATCH] Pass around Roles instead of Strings

Avoid looking up roles by names everywhere. This avoids the awkward
role/role_config naming as well.
---
 lib/kamal/cli/app.rb                | 10 ++++------
 lib/kamal/cli/env.rb                |  4 +---
 lib/kamal/commander.rb              |  4 ++--
 lib/kamal/commands/app.rb           | 25 ++++++++++++------------
 lib/kamal/commands/app/assets.rb    | 16 +++++++--------
 lib/kamal/commands/app/cord.rb      |  6 +++---
 lib/kamal/commands/app/execution.rb |  4 ++--
 lib/kamal/configuration.rb          | 30 ++++++++++++++++-------------
 lib/kamal/configuration/role.rb     |  5 +++--
 test/commander_test.rb              | 10 +++++-----
 test/commands/app_test.rb           |  3 ++-
 test/configuration_test.rb          |  4 ++--
 12 files changed, 61 insertions(+), 60 deletions(-)

diff --git a/lib/kamal/cli/app.rb b/lib/kamal/cli/app.rb
index 390b2ae93..788229ba1 100644
--- a/lib/kamal/cli/app.rb
+++ b/lib/kamal/cli/app.rb
@@ -13,9 +13,8 @@ def boot
 
             KAMAL.roles_on(host).each do |role|
               app = KAMAL.app(role: role)
-              role_config = KAMAL.config.role(role)
 
-              if role_config.assets?
+              if role.assets?
                 execute *app.extract_assets
                 old_version = capture_with_info(*app.current_running_version, raise_on_non_zero_exit: false).strip
                 execute *app.sync_asset_volumes(old_version: old_version)
@@ -27,7 +26,6 @@ def boot
             KAMAL.roles_on(host).each do |role|
               app = KAMAL.app(role: role)
               auditor = KAMAL.auditor(role: role)
-              role_config = KAMAL.config.role(role)
 
               if capture_with_info(*app.container_id_for_version(version), raise_on_non_zero_exit: false).present?
                 tmp_version = "#{version}_replaced_#{SecureRandom.hex(8)}"
@@ -38,7 +36,7 @@ def boot
 
               old_version = capture_with_info(*app.current_running_version, raise_on_non_zero_exit: false).strip
 
-              execute *app.tie_cord(role_config.cord_host_file) if role_config.uses_cord?
+              execute *app.tie_cord(role.cord_host_file) if role.uses_cord?
 
               execute *auditor.record("Booted app version #{version}"), verbosity: :debug
 
@@ -47,7 +45,7 @@ def boot
               Kamal::Cli::Healthcheck::Poller.wait_for_healthy(pause_after_ready: true) { capture_with_info(*app.status(version: version)) }
 
               if old_version.present?
-                if role_config.uses_cord?
+                if role.uses_cord?
                   cord = capture_with_info(*app.cord(version: old_version), raise_on_non_zero_exit: false).strip
                   if cord.present?
                     execute *app.cut_cord(cord)
@@ -57,7 +55,7 @@ def boot
 
                 execute *app.stop(version: old_version), raise_on_non_zero_exit: false
 
-                execute *app.clean_up_assets if role_config.assets?
+                execute *app.clean_up_assets if role.assets?
               end
             end
           end
diff --git a/lib/kamal/cli/env.rb b/lib/kamal/cli/env.rb
index 44763554a..1a02aeb1e 100644
--- a/lib/kamal/cli/env.rb
+++ b/lib/kamal/cli/env.rb
@@ -8,9 +8,8 @@ def push
         execute *KAMAL.auditor.record("Pushed env files"), verbosity: :debug
 
         KAMAL.roles_on(host).each do |role|
-          role_config = KAMAL.config.role(role)
           execute *KAMAL.app(role: role).make_env_directory
-          upload! StringIO.new(role_config.env_file), role_config.host_env_file_path, mode: 400
+          upload! StringIO.new(role.env_file), role.host_env_file_path, mode: 400
         end
       end
 
@@ -36,7 +35,6 @@ def delete
         execute *KAMAL.auditor.record("Deleted env files"), verbosity: :debug
 
         KAMAL.roles_on(host).each do |role|
-          role_config = KAMAL.config.role(role)
           execute *KAMAL.app(role: role).remove_env_file
         end
       end
diff --git a/lib/kamal/commander.rb b/lib/kamal/commander.rb
index 0a4f4b927..19e4c6cd4 100644
--- a/lib/kamal/commander.rb
+++ b/lib/kamal/commander.rb
@@ -53,7 +53,7 @@ def specific_hosts=(hosts)
 
   def primary_host
     # Given a list of specific roles, make an effort to match up with the primary_role
-    specific_hosts&.first || specific_roles&.detect { |role| role.name == config.primary_role }&.primary_host || specific_roles&.first&.primary_host || config.primary_host
+    specific_hosts&.first || specific_roles&.detect { |role| role == config.primary_role }&.primary_host || specific_roles&.first&.primary_host || config.primary_host
   end
 
   def primary_role
@@ -73,7 +73,7 @@ def hosts
   end
 
   def roles_on(host)
-    roles.select { |role| role.hosts.include?(host.to_s) }.map(&:name)
+    roles.select { |role| role.hosts.include?(host.to_s) }
   end
 
   def traefik_hosts
diff --git a/lib/kamal/commands/app.rb b/lib/kamal/commands/app.rb
index 825fe55f6..234abcfc9 100644
--- a/lib/kamal/commands/app.rb
+++ b/lib/kamal/commands/app.rb
@@ -3,12 +3,11 @@ class Kamal::Commands::App < Kamal::Commands::Base
 
   ACTIVE_DOCKER_STATUSES = [ :running, :restarting ]
 
-  attr_reader :role, :role_config
+  attr_reader :role, :role
 
   def initialize(config, role: nil)
     super(config)
     @role = role
-    @role_config = config.role(self.role)
   end
 
   def run(hostname: nil)
@@ -19,15 +18,15 @@ def run(hostname: nil)
       *(["--hostname", hostname] if hostname),
       "-e", "KAMAL_CONTAINER_NAME=\"#{container_name}\"",
       "-e", "KAMAL_VERSION=\"#{config.version}\"",
-      *role_config.env_args,
-      *role_config.health_check_args,
-      *role_config.logging_args,
+      *role.env_args,
+      *role.health_check_args,
+      *role.logging_args,
       *config.volume_args,
-      *role_config.asset_volume_args,
-      *role_config.label_args,
-      *role_config.option_args,
+      *role.asset_volume_args,
+      *role.label_args,
+      *role.option_args,
       config.absolute_image,
-      role_config.cmd
+      role.cmd
   end
 
   def start
@@ -64,22 +63,22 @@ def current_running_version
   def list_versions(*docker_args, statuses: nil)
     pipe \
       docker(:ps, *filter_args(statuses: statuses), *docker_args, "--format", '"{{.Names}}"'),
-      %(while read line; do echo ${line##{role_config.container_prefix}-}; done) # Extract SHA from "service-role-dest-SHA"
+      %(while read line; do echo ${line##{role.container_prefix}-}; done) # Extract SHA from "service-role-dest-SHA"
   end
 
 
   def make_env_directory
-    make_directory role_config.host_env_directory
+    make_directory role.host_env_directory
   end
 
   def remove_env_file
-    [ :rm, "-f", role_config.host_env_file_path ]
+    [ :rm, "-f", role.host_env_file_path ]
   end
 
 
   private
     def container_name(version = nil)
-      [ role_config.container_prefix, version || config.version ].compact.join("-")
+      [ role.container_prefix, version || config.version ].compact.join("-")
     end
 
     def filter_args(statuses: nil)
diff --git a/lib/kamal/commands/app/assets.rb b/lib/kamal/commands/app/assets.rb
index cc65627aa..ee7ea939c 100644
--- a/lib/kamal/commands/app/assets.rb
+++ b/lib/kamal/commands/app/assets.rb
@@ -1,20 +1,20 @@
 module Kamal::Commands::App::Assets
   def extract_assets
-    asset_container = "#{role_config.container_prefix}-assets"
+    asset_container = "#{role.container_prefix}-assets"
 
     combine \
-      make_directory(role_config.asset_extracted_path),
+      make_directory(role.asset_extracted_path),
       [*docker(:stop, "-t 1", asset_container, "2> /dev/null"), "|| true"],
       docker(:run, "--name", asset_container, "--detach", "--rm", config.latest_image, "sleep 1000000"),
-      docker(:cp, "-L", "#{asset_container}:#{role_config.asset_path}/.", role_config.asset_extracted_path),
+      docker(:cp, "-L", "#{asset_container}:#{role.asset_path}/.", role.asset_extracted_path),
       docker(:stop, "-t 1", asset_container),
       by: "&&"
   end
 
   def sync_asset_volumes(old_version: nil)
-    new_extracted_path, new_volume_path = role_config.asset_extracted_path(config.version), role_config.asset_volume.host_path
+    new_extracted_path, new_volume_path = role.asset_extracted_path(config.version), role.asset_volume.host_path
     if old_version.present?
-      old_extracted_path, old_volume_path = role_config.asset_extracted_path(old_version), role_config.asset_volume(old_version).host_path
+      old_extracted_path, old_volume_path = role.asset_extracted_path(old_version), role.asset_volume(old_version).host_path
     end
 
     commands = [make_directory(new_volume_path), copy_contents(new_extracted_path, new_volume_path)]
@@ -29,8 +29,8 @@ def sync_asset_volumes(old_version: nil)
 
   def clean_up_assets
     chain \
-      find_and_remove_older_siblings(role_config.asset_extracted_path),
-      find_and_remove_older_siblings(role_config.asset_volume_path)
+      find_and_remove_older_siblings(role.asset_extracted_path),
+      find_and_remove_older_siblings(role.asset_volume_path)
   end
 
   private
@@ -39,7 +39,7 @@ def find_and_remove_older_siblings(path)
         :find,
         Pathname.new(path).dirname.to_s,
         "-maxdepth 1",
-        "-name", "'#{role_config.container_prefix}-*'",
+        "-name", "'#{role.container_prefix}-*'",
         "!", "-name", Pathname.new(path).basename.to_s,
         "-exec rm -rf \"{}\" +"
       ]
diff --git a/lib/kamal/commands/app/cord.rb b/lib/kamal/commands/app/cord.rb
index ef722d66a..3cc2415a5 100644
--- a/lib/kamal/commands/app/cord.rb
+++ b/lib/kamal/commands/app/cord.rb
@@ -2,7 +2,7 @@ module Kamal::Commands::App::Cord
   def cord(version:)
     pipe \
       docker(:inspect, "-f '{{ range .Mounts }}{{printf \"%s %s\\n\" .Source .Destination}}{{ end }}'", container_name(version)),
-      [:awk, "'$2 == \"#{role_config.cord_volume.container_path}\" {print $1}'"]
+      [:awk, "'$2 == \"#{role.cord_volume.container_path}\" {print $1}'"]
   end
 
   def tie_cord(cord)
@@ -12,8 +12,8 @@ def tie_cord(cord)
   def cut_cord(cord)
     remove_directory(cord)
   end
-  
-  private    
+
+  private
     def create_empty_file(file)
       chain \
         make_directory_for(file),
diff --git a/lib/kamal/commands/app/execution.rb b/lib/kamal/commands/app/execution.rb
index ff3877269..cb07075be 100644
--- a/lib/kamal/commands/app/execution.rb
+++ b/lib/kamal/commands/app/execution.rb
@@ -10,9 +10,9 @@ def execute_in_new_container(*command, interactive: false)
     docker :run,
       ("-it" if interactive),
       "--rm",
-      *role_config&.env_args,
+      *role&.env_args,
       *config.volume_args,
-      *role_config&.option_args,
+      *role&.option_args,
       config.absolute_image,
       *command
   end
diff --git a/lib/kamal/configuration.rb b/lib/kamal/configuration.rb
index a57d8b3b8..7ac5dc1be 100644
--- a/lib/kamal/configuration.rb
+++ b/lib/kamal/configuration.rb
@@ -92,7 +92,19 @@ def all_hosts
   end
 
   def primary_host
-    role(primary_role)&.primary_host
+    primary_role&.primary_host
+  end
+
+  def primary_role_name
+    raw_config.primary_role || "web"
+  end
+
+  def primary_role
+    role(primary_role_name)
+  end
+
+  def allow_empty_roles?
+    raw_config.allow_empty_roles
   end
 
   def traefik_roles
@@ -212,14 +224,6 @@ def asset_path
     raw_config.asset_path
   end
 
-  def primary_role
-    raw_config.primary_role || "web"
-  end
-
-  def allow_empty_roles?
-    raw_config.allow_empty_roles
-  end
-
 
   def valid?
     ensure_destination_if_required && ensure_required_keys_present && ensure_valid_kamal_version && ensure_retain_containers_valid && ensure_valid_service_name
@@ -268,12 +272,12 @@ def ensure_required_keys_present
         raise ArgumentError, "You must specify a password for the registry in config/deploy.yml (or set the ENV variable if that's used)"
       end
 
-      unless role_names.include?(primary_role)
-        raise ArgumentError, "The primary_role #{primary_role} isn't defined"
+      unless role_names.include?(primary_role_name)
+        raise ArgumentError, "The primary_role #{primary_role_name} isn't defined"
       end
 
-      if role(primary_role).hosts.empty?
-        raise ArgumentError, "No servers specified for the #{primary_role} primary_role"
+      if primary_role.hosts.empty?
+        raise ArgumentError, "No servers specified for the #{primary_role.name} primary_role"
       end
 
       unless allow_empty_roles?
diff --git a/lib/kamal/configuration/role.rb b/lib/kamal/configuration/role.rb
index 82c281815..9a30ee9f4 100644
--- a/lib/kamal/configuration/role.rb
+++ b/lib/kamal/configuration/role.rb
@@ -3,9 +3,10 @@ class Kamal::Configuration::Role
   delegate :argumentize, :optionize, to: Kamal::Utils
 
   attr_accessor :name
+  alias to_s name
 
   def initialize(name, config:)
-   @name, @config = name.inquiry, config
+    @name, @config = name.inquiry, config
   end
 
   def primary_host
@@ -113,7 +114,7 @@ def running_traefik?
   end
 
   def primary?
-    @config.primary_role == name
+    self == @config.primary_role
   end
 
 
diff --git a/test/commander_test.rb b/test/commander_test.rb
index 598fc56bc..55d6cb1d4 100644
--- a/test/commander_test.rb
+++ b/test/commander_test.rb
@@ -83,14 +83,14 @@ class CommanderTest < ActiveSupport::TestCase
   end
 
   test "primary_role" do
-    assert_equal "web", @kamal.primary_role
+    assert_equal "web", @kamal.primary_role.name
     @kamal.specific_roles = "workers"
-    assert_equal "workers", @kamal.primary_role
+    assert_equal "workers", @kamal.primary_role.name
   end
 
   test "roles_on" do
-    assert_equal [ "web" ], @kamal.roles_on("1.1.1.1")
-    assert_equal [ "workers" ], @kamal.roles_on("1.1.1.3")
+    assert_equal [ "web" ], @kamal.roles_on("1.1.1.1").map(&:name)
+    assert_equal [ "workers" ], @kamal.roles_on("1.1.1.3").map(&:name)
   end
 
   test "default group strategy" do
@@ -120,7 +120,7 @@ class CommanderTest < ActiveSupport::TestCase
 
     @kamal.specific_roles = [ "web_*" ]
     assert_equal [ "web_chicago", "web_tokyo" ], @kamal.roles.map(&:name)
-    assert_equal "web_tokyo", @kamal.primary_role
+    assert_equal "web_tokyo", @kamal.primary_role.name
     assert_equal "1.1.1.3", @kamal.primary_host
   end
 
diff --git a/test/commands/app_test.rb b/test/commands/app_test.rb
index 3ec377d79..d31a42d1c 100644
--- a/test/commands/app_test.rb
+++ b/test/commands/app_test.rb
@@ -400,6 +400,7 @@ class CommandsAppTest < ActiveSupport::TestCase
 
   private
     def new_command(role: "web", **additional_config)
-      Kamal::Commands::App.new(Kamal::Configuration.new(@config.merge(additional_config), destination: @destination, version: "999"), role: role)
+      config = Kamal::Configuration.new(@config.merge(additional_config), destination: @destination, version: "999")
+      Kamal::Commands::App.new(config, role: config.role(role))
     end
 end
diff --git a/test/configuration_test.rb b/test/configuration_test.rb
index c538a82ab..5432c98de 100644
--- a/test/configuration_test.rb
+++ b/test/configuration_test.rb
@@ -300,14 +300,14 @@ class ConfigurationTest < ActiveSupport::TestCase
   end
 
   test "primary role" do
-    assert_equal "web", @config.primary_role
+    assert_equal "web", @config.primary_role.name
 
     config = Kamal::Configuration.new(@deploy_with_roles.deep_merge({
       servers: { "alternate_web" => { "hosts" => [ "1.1.1.4", "1.1.1.5" ] } },
       primary_role: "alternate_web" } ))
 
 
-    assert_equal "alternate_web", config.primary_role
+    assert_equal "alternate_web", config.primary_role.name
     assert_equal "1.1.1.4", config.primary_host
     assert config.role(:alternate_web).primary?
     assert config.role(:alternate_web).running_traefik?