From 098c937bab84ea9e4b62fc13e0fd9b181bf036a8 Mon Sep 17 00:00:00 2001 From: Donal McBreen Date: Fri, 18 Apr 2025 09:52:22 +0100 Subject: [PATCH] Move docker login into build command We only need to run the docker login commands for pushing and pulling images. So let's move the logins into those commands. This ensures we are logged in when calling `kamal build` commands directly. Fixes: https://github.com/basecamp/kamal/issues/919 --- lib/kamal/cli/build.rb | 16 ++++++++++++++++ lib/kamal/cli/main.rb | 5 +---- test/cli/build_test.rb | 5 +++++ test/cli/main_test.rb | 18 +++++------------- 4 files changed, 27 insertions(+), 17 deletions(-) diff --git a/lib/kamal/cli/build.rb b/lib/kamal/cli/build.rb index 0187e686..310c2689 100644 --- a/lib/kamal/cli/build.rb +++ b/lib/kamal/cli/build.rb @@ -15,6 +15,8 @@ class Kamal::Cli::Build < Kamal::Cli::Base cli = self ensure_docker_installed + login_to_registry_locally + run_hook "pre-build" uncommitted_changes = Kamal::Git.uncommitted_changes @@ -61,6 +63,8 @@ class Kamal::Cli::Build < Kamal::Cli::Base desc "pull", "Pull app image from registry onto servers" def pull + login_to_registry_remotely + if (first_hosts = mirror_hosts).any? #  Pull on a single host per mirror first to seed them say "Pulling image on #{first_hosts.join(", ")} to seed the #{"mirror".pluralize(first_hosts.count)}...", :magenta @@ -181,4 +185,16 @@ class Kamal::Cli::Build < Kamal::Cli::Base execute *KAMAL.builder.validate_image end end + + def login_to_registry_locally + run_locally do + execute *KAMAL.registry.login + end + end + + def login_to_registry_remotely + on(KAMAL.hosts) do + execute *KAMAL.registry.login + end + end end diff --git a/lib/kamal/cli/main.rb b/lib/kamal/cli/main.rb index 2fae36e8..cd28ab35 100644 --- a/lib/kamal/cli/main.rb +++ b/lib/kamal/cli/main.rb @@ -20,9 +20,6 @@ class Kamal::Cli::Main < Kamal::Cli::Base runtime = print_runtime do invoke_options = deploy_options - say "Log into image registry...", :magenta - invoke "kamal:cli:registry:login", [], invoke_options.merge(skip_local: options[:skip_push]) - if options[:skip_push] say "Pull app image...", :magenta invoke "kamal:cli:build:pull", [], invoke_options @@ -52,7 +49,7 @@ class Kamal::Cli::Main < Kamal::Cli::Base run_hook "post-deploy", secrets: true, runtime: runtime.round.to_s end - desc "redeploy", "Deploy app to servers without bootstrapping servers, starting kamal-proxy, pruning, and registry login" + desc "redeploy", "Deploy app to servers without bootstrapping servers, starting kamal-proxy and pruning" option :skip_push, aliases: "-P", type: :boolean, default: false, desc: "Skip image build and push" def redeploy runtime = print_runtime do diff --git a/test/cli/build_test.rb b/test/cli/build_test.rb index 8690e00b..f2dba886 100644 --- a/test/cli/build_test.rb +++ b/test/cli/build_test.rb @@ -57,6 +57,7 @@ class CliBuildTest < CliTestCase stub_setup SSHKit::Backend::Abstract.any_instance.expects(:execute).with(:docker, "--version", "&&", :docker, :buildx, "version") + SSHKit::Backend::Abstract.any_instance.stubs(:execute).with { |*args| args[0..1] == [ :docker, :login ] } SSHKit::Backend::Abstract.any_instance.expects(:execute) .with(:git, "-C", "#{Dir.tmpdir}/kamal-clones/app-#{pwd_sha}", :clone, Dir.pwd, "--recurse-submodules") @@ -103,6 +104,7 @@ class CliBuildTest < CliTestCase stub_setup SSHKit::Backend::Abstract.any_instance.expects(:execute).with(:docker, "--version", "&&", :docker, :buildx, "version") + SSHKit::Backend::Abstract.any_instance.stubs(:execute).with { |*args| args[0..1] == [ :docker, :login ] } SSHKit::Backend::Abstract.any_instance.expects(:execute) .with(:git, "-C", "#{Dir.tmpdir}/kamal-clones/app-#{pwd_sha}", :clone, Dir.pwd, "--recurse-submodules") @@ -139,6 +141,9 @@ class CliBuildTest < CliTestCase SSHKit::Backend::Abstract.any_instance.expects(:execute) .with(:docker, "--version", "&&", :docker, :buildx, "version") + SSHKit::Backend::Abstract.any_instance.stubs(:execute) + .with { |*args| args[0..1] == [ :docker, :login ] } + SSHKit::Backend::Abstract.any_instance.expects(:execute) .with(:docker, :buildx, :rm, "kamal-local-docker-container") diff --git a/test/cli/main_test.rb b/test/cli/main_test.rb index becace43..d37c9352 100644 --- a/test/cli/main_test.rb +++ b/test/cli/main_test.rb @@ -21,7 +21,6 @@ class CliMainTest < CliTestCase Kamal::Cli::Main.any_instance.expects(:invoke).with("kamal:cli:server:bootstrap", [], invoke_options) Kamal::Cli::Main.any_instance.expects(:invoke).with("kamal:cli:accessory:boot", [ "all" ], invoke_options) # deploy - Kamal::Cli::Main.any_instance.expects(:invoke).with("kamal:cli:registry:login", [], invoke_options.merge(skip_local: true)) Kamal::Cli::Main.any_instance.expects(:invoke).with("kamal:cli:build:pull", [], invoke_options) Kamal::Cli::Main.any_instance.expects(:invoke).with("kamal:cli:proxy:boot", [], invoke_options) Kamal::Cli::Main.any_instance.expects(:invoke).with("kamal:cli:app:stale_containers", [], invoke_options.merge(stop: true)) @@ -32,7 +31,6 @@ class CliMainTest < CliTestCase assert_match /Ensure Docker is installed.../, output # deploy assert_match /Acquiring the deploy lock/, output - assert_match /Log into image registry/, output assert_match /Pull app image/, output assert_match /Ensure kamal-proxy is running/, output assert_match /Detect stale containers/, output @@ -45,8 +43,7 @@ class CliMainTest < CliTestCase with_test_secrets("secrets" => "DB_PASSWORD=secret") do invoke_options = { "config_file" => "test/fixtures/deploy_simple.yml", "version" => "999", "skip_hooks" => false, "verbose" => true } - Kamal::Cli::Main.any_instance.expects(:invoke).with("kamal:cli:registry:login", [], invoke_options.merge(skip_local: false)) - Kamal::Cli::Main.any_instance.expects(:invoke).with("kamal:cli:build:deliver", [], invoke_options) + Kamal::Cli::Main.any_instance.expects(:invoke).with("kamal:cli:build:deliver", [], invoke_options) Kamal::Cli::Main.any_instance.expects(:invoke).with("kamal:cli:proxy:boot", [], invoke_options) Kamal::Cli::Main.any_instance.expects(:invoke).with("kamal:cli:app:stale_containers", [], invoke_options.merge(stop: true)) Kamal::Cli::Main.any_instance.expects(:invoke).with("kamal:cli:app:boot", [], invoke_options) @@ -56,8 +53,7 @@ class CliMainTest < CliTestCase run_command("deploy", "--verbose").tap do |output| assert_hook_ran "pre-connect", output - assert_match /Log into image registry/, output - assert_match /Build and push app image/, output + assert_match /Build and push app image/, output assert_hook_ran "pre-deploy", output assert_match /Ensure kamal-proxy is running/, output assert_match /Detect stale containers/, output @@ -70,7 +66,6 @@ class CliMainTest < CliTestCase test "deploy with skip_push" do invoke_options = { "config_file" => "test/fixtures/deploy_simple.yml", "version" => "999", "skip_hooks" => false } - Kamal::Cli::Main.any_instance.expects(:invoke).with("kamal:cli:registry:login", [], invoke_options.merge(skip_local: true)) Kamal::Cli::Main.any_instance.expects(:invoke).with("kamal:cli:build:pull", [], invoke_options) Kamal::Cli::Main.any_instance.expects(:invoke).with("kamal:cli:proxy:boot", [], invoke_options) Kamal::Cli::Main.any_instance.expects(:invoke).with("kamal:cli:app:stale_containers", [], invoke_options.merge(stop: true)) @@ -79,7 +74,6 @@ class CliMainTest < CliTestCase run_command("deploy", "--skip_push").tap do |output| assert_match /Acquiring the deploy lock/, output - assert_match /Log into image registry/, output assert_match /Pull app image/, output assert_match /Ensure kamal-proxy is running/, output assert_match /Detect stale containers/, output @@ -153,11 +147,11 @@ class CliMainTest < CliTestCase end end - test "deploy errors during outside section leave remove lock" do - invoke_options = { "config_file" => "test/fixtures/deploy_simple.yml", "version" => "999", "skip_hooks" => false, :skip_local => false } + test "deploy errors during outside section leave remote lock" do + invoke_options = { "config_file" => "test/fixtures/deploy_simple.yml", "version" => "999", "skip_hooks" => false } Kamal::Cli::Main.any_instance.expects(:invoke) - .with("kamal:cli:registry:login", [], invoke_options.merge(skip_local: false)) + .with("kamal:cli:build:deliver", [], invoke_options) .raises(RuntimeError) assert_not KAMAL.holding_lock? @@ -170,7 +164,6 @@ class CliMainTest < CliTestCase test "deploy with skipped hooks" do invoke_options = { "config_file" => "test/fixtures/deploy_simple.yml", "version" => "999", "skip_hooks" => true } - Kamal::Cli::Main.any_instance.expects(:invoke).with("kamal:cli:registry:login", [], invoke_options.merge(skip_local: false)) Kamal::Cli::Main.any_instance.expects(:invoke).with("kamal:cli:build:deliver", [], invoke_options) Kamal::Cli::Main.any_instance.expects(:invoke).with("kamal:cli:proxy:boot", [], invoke_options) Kamal::Cli::Main.any_instance.expects(:invoke).with("kamal:cli:app:stale_containers", [], invoke_options.merge(stop: true)) @@ -185,7 +178,6 @@ class CliMainTest < CliTestCase test "deploy with missing secrets" do invoke_options = { "config_file" => "test/fixtures/deploy_with_secrets.yml", "version" => "999", "skip_hooks" => false } - Kamal::Cli::Main.any_instance.expects(:invoke).with("kamal:cli:registry:login", [], invoke_options.merge(skip_local: false)) Kamal::Cli::Main.any_instance.expects(:invoke).with("kamal:cli:build:deliver", [], invoke_options) Kamal::Cli::Main.any_instance.expects(:invoke).with("kamal:cli:proxy:boot", [], invoke_options) Kamal::Cli::Main.any_instance.expects(:invoke).with("kamal:cli:app:stale_containers", [], invoke_options.merge(stop: true))