From 79baa598fa99afc80cf0dea8317122b1fea1dcfb Mon Sep 17 00:00:00 2001 From: Matthew Kent Date: Fri, 24 Nov 2023 15:35:36 -0800 Subject: [PATCH 1/2] Make an effort to match the primary_role from a list of specific roles. This is less surprising than picking the first role and first host. --- lib/kamal/commander.rb | 3 ++- test/commander_test.rb | 9 +++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/lib/kamal/commander.rb b/lib/kamal/commander.rb index 35939fe7d..ded958eb8 100644 --- a/lib/kamal/commander.rb +++ b/lib/kamal/commander.rb @@ -36,7 +36,8 @@ def specific_hosts=(hosts) end def primary_host - specific_hosts&.first || specific_roles&.first&.primary_host || config.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 end def primary_role diff --git a/test/commander_test.rb b/test/commander_test.rb index 06cc5bbb1..d98026685 100644 --- a/test/commander_test.rb +++ b/test/commander_test.rb @@ -100,6 +100,15 @@ class CommanderTest < ActiveSupport::TestCase assert_equal({ in: :groups, limit: 1, wait: 2 }, @kamal.boot_strategy) end + test "try to match the primary role from a list of specific roles" do + configure_with(:deploy_primary_web_role_override) + + @kamal.specific_roles = [ "web_*" ] + assert_equal [ "web_chicago", "web_tokyo" ], @kamal.roles.map(&:name) + assert_equal "web_tokyo", @kamal.primary_role + assert_equal "1.1.1.3", @kamal.primary_host + end + private def configure_with(variant) @kamal = Kamal::Commander.new.tap do |kamal| From 63babecba745460543d129fc94bedebe7f8e40a4 Mon Sep 17 00:00:00 2001 From: Matthew Kent Date: Fri, 24 Nov 2023 21:18:06 -0800 Subject: [PATCH 2/2] Raise an error when either the filtered hosts or roles are empty. Keeps us confusingly running things on the primary_host when nothing matches. --- lib/kamal/commander.rb | 20 ++++++++++++++++++-- test/commander_test.rb | 17 +++++++++++++---- 2 files changed, 31 insertions(+), 6 deletions(-) diff --git a/lib/kamal/commander.rb b/lib/kamal/commander.rb index ded958eb8..0a4f4b927 100644 --- a/lib/kamal/commander.rb +++ b/lib/kamal/commander.rb @@ -28,11 +28,27 @@ def specific_primary! end def specific_roles=(role_names) - @specific_roles = Kamal::Utils.filter_specific_items(role_names, config.roles) if role_names.present? + if role_names.present? + @specific_roles = Kamal::Utils.filter_specific_items(role_names, config.roles) + + if @specific_roles.empty? + raise ArgumentError, "No --roles match for #{role_names.join(',')}" + end + + @specific_roles + end end def specific_hosts=(hosts) - @specific_hosts = Kamal::Utils.filter_specific_items(hosts, config.all_hosts) if hosts.present? + if hosts.present? + @specific_hosts = Kamal::Utils.filter_specific_items(hosts, config.all_hosts) + + if @specific_hosts.empty? + raise ArgumentError, "No --hosts match for #{hosts.join(',')}" + end + + @specific_hosts + end end def primary_host diff --git a/test/commander_test.rb b/test/commander_test.rb index d98026685..4e848a4b1 100644 --- a/test/commander_test.rb +++ b/test/commander_test.rb @@ -24,8 +24,10 @@ class CommanderTest < ActiveSupport::TestCase @kamal.specific_hosts = [ "*" ] assert_equal [ "1.1.1.1", "1.1.1.2", "1.1.1.3", "1.1.1.4" ], @kamal.hosts - @kamal.specific_hosts = [ "*miss" ] - assert_equal [], @kamal.hosts + exception = assert_raises(ArgumentError) do + @kamal.specific_hosts = [ "*miss" ] + end + assert_match /hosts match for \*miss/, exception.message end test "filtering hosts by filtering roles" do @@ -33,6 +35,11 @@ class CommanderTest < ActiveSupport::TestCase @kamal.specific_roles = [ "web" ] assert_equal [ "1.1.1.1", "1.1.1.2" ], @kamal.hosts + + exception = assert_raises(ArgumentError) do + @kamal.specific_roles = [ "*miss" ] + end + assert_match /roles match for \*miss/, exception.message end test "filtering roles" do @@ -50,8 +57,10 @@ class CommanderTest < ActiveSupport::TestCase @kamal.specific_roles = [ "*" ] assert_equal [ "web", "workers" ], @kamal.roles.map(&:name) - @kamal.specific_roles = [ "*miss" ] - assert_equal [], @kamal.roles.map(&:name) + exception = assert_raises(ArgumentError) do + @kamal.specific_roles = [ "*miss" ] + end + assert_match /roles match for \*miss/, exception.message end test "filtering roles by filtering hosts" do