Merge pull request #1506 from basecamp/registry-login-on-push-and-pull

Move docker login into build command

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

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

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")
 

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
+  test "deploy errors during outside section leave remote lock" do
-    invoke_options = { "config_file" => "test/fixtures/deploy_simple.yml", "version" => "999", "skip_hooks" => false, :skip_local => false }
+    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))