From 2a4a8ac859ebc4c47706f03fdd9a54260ebe2de1 Mon Sep 17 00:00:00 2001 From: Donal McBreen Date: Thu, 6 Jun 2024 09:43:58 +0100 Subject: [PATCH] Combine multiarch and native/cache builders Combine the two builders, as they are almost identical. The only difference was whether the platforms were set. The native cached builder wasn't using the context it created, so now we do. We'll set the driver to `docker-container` - it seems to be the default but the Docker docs claim it is `docker`. --- lib/kamal/commands/builder.rb | 33 +++++++------------ lib/kamal/commands/builder/base.rb | 5 ++- .../builder/{multiarch.rb => local.rb} | 20 ++++++----- .../commands/builder/multiarch/remote.rb | 27 +++++++++++++-- lib/kamal/commands/builder/native/cached.rb | 25 -------------- test/cli/build_test.rb | 20 +++++------ test/cli/main_test.rb | 4 +-- test/commands/builder_test.rb | 24 +++++++------- 8 files changed, 76 insertions(+), 82 deletions(-) rename lib/kamal/commands/builder/{multiarch.rb => local.rb} (53%) delete mode 100644 lib/kamal/commands/builder/native/cached.rb diff --git a/lib/kamal/commands/builder.rb b/lib/kamal/commands/builder.rb index 13c3d829..c7be883a 100644 --- a/lib/kamal/commands/builder.rb +++ b/lib/kamal/commands/builder.rb @@ -11,22 +11,17 @@ class Kamal::Commands::Builder < Kamal::Commands::Base end def target - if config.builder.multiarch? - if config.builder.remote? - if config.builder.local? - multiarch_remote - else - native_remote - end - else - multiarch - end + case + when !config.builder.multiarch? && !config.builder.cached? + native + when !config.builder.multiarch? && config.builder.cached? + local + when config.builder.local? && config.builder.remote? + multiarch_remote + when config.builder.remote? + native_remote else - if config.builder.cached? - native_cached - else - native - end + local end end @@ -34,16 +29,12 @@ class Kamal::Commands::Builder < Kamal::Commands::Base @native ||= Kamal::Commands::Builder::Native.new(config) end - def native_cached - @native ||= Kamal::Commands::Builder::Native::Cached.new(config) - end - def native_remote @native ||= Kamal::Commands::Builder::Native::Remote.new(config) end - def multiarch - @multiarch ||= Kamal::Commands::Builder::Multiarch.new(config) + def local + @local ||= Kamal::Commands::Builder::Local.new(config) end def multiarch_remote diff --git a/lib/kamal/commands/builder/base.rb b/lib/kamal/commands/builder/base.rb index cedfeadf..a59c5561 100644 --- a/lib/kamal/commands/builder/base.rb +++ b/lib/kamal/commands/builder/base.rb @@ -5,7 +5,10 @@ class Kamal::Commands::Builder::Base < Kamal::Commands::Base ENDPOINT_DOCKER_HOST_INSPECT = "'{{.Endpoints.docker.Host}}'" delegate :argumentize, to: Kamal::Utils - delegate :args, :secrets, :dockerfile, :target, :local_arch, :local_host, :remote_arch, :remote_host, :cache_from, :cache_to, :ssh, to: :builder_config + delegate \ + :args, :secrets, :dockerfile, :target, :local_arch, :local_host, :remote_arch, :remote_host, + :cache_from, :cache_to, :multiarch?, :ssh, + to: :builder_config def clean docker :image, :rm, "--force", config.absolute_image diff --git a/lib/kamal/commands/builder/multiarch.rb b/lib/kamal/commands/builder/local.rb similarity index 53% rename from lib/kamal/commands/builder/multiarch.rb rename to lib/kamal/commands/builder/local.rb index ae1d423f..7af8bc06 100644 --- a/lib/kamal/commands/builder/multiarch.rb +++ b/lib/kamal/commands/builder/local.rb @@ -1,6 +1,6 @@ -class Kamal::Commands::Builder::Multiarch < Kamal::Commands::Builder::Base +class Kamal::Commands::Builder::Local < Kamal::Commands::Builder::Base def create - docker :buildx, :create, "--use", "--name", builder_name + docker :buildx, :create, "--name", builder_name, "--driver=docker-container" end def remove @@ -16,7 +16,7 @@ class Kamal::Commands::Builder::Multiarch < Kamal::Commands::Builder::Base def push docker :buildx, :build, "--push", - "--platform", platform_names, + *platform_options, "--builder", builder_name, *build_options, build_context @@ -28,14 +28,16 @@ class Kamal::Commands::Builder::Multiarch < Kamal::Commands::Builder::Base private def builder_name - "kamal-#{config.service}-multiarch" + "kamal-local" end - def platform_names - if local_arch - "linux/#{local_arch}" - else - "linux/amd64,linux/arm64" + def platform_options + if multiarch? + if local_arch + [ "--platform", "linux/#{local_arch}" ] + else + [ "--platform", "linux/amd64,linux/arm64" ] + end end end end diff --git a/lib/kamal/commands/builder/multiarch/remote.rb b/lib/kamal/commands/builder/multiarch/remote.rb index d60bee77..01114bce 100644 --- a/lib/kamal/commands/builder/multiarch/remote.rb +++ b/lib/kamal/commands/builder/multiarch/remote.rb @@ -1,4 +1,4 @@ -class Kamal::Commands::Builder::Multiarch::Remote < Kamal::Commands::Builder::Multiarch +class Kamal::Commands::Builder::Multiarch::Remote < Kamal::Commands::Builder::Base def create combine \ create_contexts, @@ -12,6 +12,21 @@ class Kamal::Commands::Builder::Multiarch::Remote < Kamal::Commands::Builder::Mu super end + def info + combine \ + docker(:context, :ls), + docker(:buildx, :ls) + end + + def push + docker :buildx, :build, + "--push", + *platform_options, + "--builder", builder_name, + *build_options, + build_context + end + def context_hosts chain \ context_host(builder_name_with_arch(local_arch)), @@ -24,7 +39,7 @@ class Kamal::Commands::Builder::Multiarch::Remote < Kamal::Commands::Builder::Mu private def builder_name - super + "-remote" + "kamal-#{config.service}-multiarch-remote" end def builder_name_with_arch(arch) @@ -58,4 +73,12 @@ class Kamal::Commands::Builder::Multiarch::Remote < Kamal::Commands::Builder::Mu def remove_context(arch) docker :context, :rm, builder_name_with_arch(arch) end + + def platform_options + if local_arch + [ "--platform", "linux/#{local_arch}" ] + else + [ "--platform", "linux/amd64,linux/arm64" ] + end + end end diff --git a/lib/kamal/commands/builder/native/cached.rb b/lib/kamal/commands/builder/native/cached.rb deleted file mode 100644 index 8f65d5f3..00000000 --- a/lib/kamal/commands/builder/native/cached.rb +++ /dev/null @@ -1,25 +0,0 @@ -class Kamal::Commands::Builder::Native::Cached < Kamal::Commands::Builder::Native - def create - docker :buildx, :create, "--name", builder_name, "--use", "--driver=docker-container" - end - - def remove - docker :buildx, :rm, builder_name - end - - def push - docker :buildx, :build, - "--push", - *build_options, - build_context - end - - def context_hosts - docker :buildx, :inspect, builder_name, "> /dev/null" - end - - private - def builder_name - "kamal-#{config.service}-native-cached" - end -end diff --git a/test/cli/build_test.rb b/test/cli/build_test.rb index 18ff254b..dc349c7c 100644 --- a/test/cli/build_test.rb +++ b/test/cli/build_test.rb @@ -22,7 +22,7 @@ class CliBuildTest < CliTestCase .returns("") SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info) - .with(:docker, :buildx, :inspect, "kamal-app-multiarch", "> /dev/null") + .with(:docker, :buildx, :inspect, "kamal-local", "> /dev/null") .returns("") run_command("push", "--verbose").tap do |output| @@ -30,7 +30,7 @@ class CliBuildTest < CliTestCase assert_match /Cloning repo into build directory/, output assert_match /git -C #{Dir.tmpdir}\/kamal-clones\/app-#{pwd_sha} clone #{Dir.pwd}/, output assert_match /docker --version && docker buildx version/, output - assert_match /docker buildx build --push --platform linux\/amd64,linux\/arm64 --builder kamal-app-multiarch -t dhh\/app:999 -t dhh\/app:latest --label service="app" --file Dockerfile \. as .*@localhost/, output + assert_match /docker buildx build --push --platform linux\/amd64,linux\/arm64 --builder kamal-local -t dhh\/app:999 -t dhh\/app:latest --label service="app" --file Dockerfile \. as .*@localhost/, output end end end @@ -52,7 +52,7 @@ class CliBuildTest < CliTestCase SSHKit::Backend::Abstract.any_instance.expects(:execute).with(:git, "-C", build_directory, :clean, "-fdx") SSHKit::Backend::Abstract.any_instance.expects(:execute) - .with(:docker, :buildx, :build, "--push", "--platform", "linux/amd64,linux/arm64", "--builder", "kamal-app-multiarch", "-t", "dhh/app:999", "-t", "dhh/app:latest", "--label", "service=\"app\"", "--file", "Dockerfile", ".") + .with(:docker, :buildx, :build, "--push", "--platform", "linux/amd64,linux/arm64", "--builder", "kamal-local", "-t", "dhh/app:999", "-t", "dhh/app:latest", "--label", "service=\"app\"", "--file", "Dockerfile", ".") SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info) .with(:git, "-C", anything, :"rev-parse", :HEAD) @@ -77,7 +77,7 @@ class CliBuildTest < CliTestCase assert_no_match /Cloning repo into build directory/, output assert_hook_ran "pre-build", output, **hook_variables assert_match /docker --version && docker buildx version/, output - assert_match /docker buildx build --push --platform linux\/amd64,linux\/arm64 --builder kamal-app-multiarch -t dhh\/app:999 -t dhh\/app:latest --label service="app" --file Dockerfile . as .*@localhost/, output + assert_match /docker buildx build --push --platform linux\/amd64,linux\/arm64 --builder kamal-local -t dhh\/app:999 -t dhh\/app:latest --label service="app" --file Dockerfile . as .*@localhost/, output end end @@ -123,10 +123,10 @@ class CliBuildTest < CliTestCase .with(:docker, "--version", "&&", :docker, :buildx, "version") SSHKit::Backend::Abstract.any_instance.expects(:execute) - .with(:docker, :buildx, :create, "--use", "--name", "kamal-app-multiarch") + .with(:docker, :buildx, :create, "--name", "kamal-local", "--driver=docker-container") SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info) - .with(:docker, :buildx, :inspect, "kamal-app-multiarch", "> /dev/null") + .with(:docker, :buildx, :inspect, "kamal-local", "> /dev/null") .raises(SSHKit::Command::Failed.new("no builder")) SSHKit::Backend::Abstract.any_instance.expects(:execute).with { |*args| args.first.start_with?("git") } @@ -140,7 +140,7 @@ class CliBuildTest < CliTestCase .returns("") SSHKit::Backend::Abstract.any_instance.expects(:execute) - .with(:docker, :buildx, :build, "--push", "--platform", "linux/amd64,linux/arm64", "--builder", "kamal-app-multiarch", "-t", "dhh/app:999", "-t", "dhh/app:latest", "--label", "service=\"app\"", "--file", "Dockerfile", ".") + .with(:docker, :buildx, :build, "--push", "--platform", "linux/amd64,linux/arm64", "--builder", "kamal-local", "-t", "dhh/app:999", "-t", "dhh/app:latest", "--label", "service=\"app\"", "--file", "Dockerfile", ".") run_command("push").tap do |output| assert_match /WARN Missing compatible builder, so creating a new one first/, output @@ -206,7 +206,7 @@ class CliBuildTest < CliTestCase test "create" do run_command("create").tap do |output| - assert_match /docker buildx create --use --name kamal-app-multiarch/, output + assert_match /docker buildx create --name kamal-local --driver=docker-container/, output end end @@ -239,7 +239,7 @@ class CliBuildTest < CliTestCase test "remove" do run_command("remove").tap do |output| - assert_match /docker buildx rm kamal-app-multiarch/, output + assert_match /docker buildx rm kamal-local/, output end end @@ -249,7 +249,7 @@ class CliBuildTest < CliTestCase .returns("docker builder info") run_command("details").tap do |output| - assert_match /Builder: multiarch/, output + assert_match /Builder: local/, output assert_match /docker builder info/, output end end diff --git a/test/cli/main_test.rb b/test/cli/main_test.rb index e562499b..a8413e59 100644 --- a/test/cli/main_test.rb +++ b/test/cli/main_test.rb @@ -122,7 +122,7 @@ class CliMainTest < CliTestCase .returns("") SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info) - .with(:docker, :buildx, :inspect, "kamal-app-multiarch", "> /dev/null") + .with(:docker, :buildx, :inspect, "kamal-local", "> /dev/null") .returns("") SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info) @@ -160,7 +160,7 @@ class CliMainTest < CliTestCase .returns("") SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info) - .with(:docker, :buildx, :inspect, "kamal-app-multiarch", "> /dev/null") + .with(:docker, :buildx, :inspect, "kamal-local", "> /dev/null") .returns("") SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info) diff --git a/test/commands/builder_test.rb b/test/commands/builder_test.rb index f3faa5f6..6e282be7 100644 --- a/test/commands/builder_test.rb +++ b/test/commands/builder_test.rb @@ -7,9 +7,9 @@ class CommandsBuilderTest < ActiveSupport::TestCase test "target multiarch by default" do builder = new_builder_command(builder: { "cache" => { "type" => "gha" } }) - assert_equal "multiarch", builder.name + assert_equal "local", builder.name assert_equal \ - "docker buildx build --push --platform linux/amd64,linux/arm64 --builder kamal-app-multiarch -t dhh/app:123 -t dhh/app:latest --cache-to type=gha --cache-from type=gha --label service=\"app\" --file Dockerfile .", + "docker buildx build --push --platform linux/amd64,linux/arm64 --builder kamal-local -t dhh/app:123 -t dhh/app:latest --cache-to type=gha --cache-from type=gha --label service=\"app\" --file Dockerfile .", builder.push.join(" ") end @@ -23,9 +23,9 @@ class CommandsBuilderTest < ActiveSupport::TestCase test "target native cached when multiarch is off and cache is set" do builder = new_builder_command(builder: { "multiarch" => false, "cache" => { "type" => "gha" } }) - assert_equal "native/cached", builder.name + assert_equal "local", builder.name assert_equal \ - "docker buildx build --push -t dhh/app:123 -t dhh/app:latest --cache-to type=gha --cache-from type=gha --label service=\"app\" --file Dockerfile .", + "docker buildx build --push --builder kamal-local -t dhh/app:123 -t dhh/app:latest --cache-to type=gha --cache-from type=gha --label service=\"app\" --file Dockerfile .", builder.push.join(" ") end @@ -39,9 +39,9 @@ class CommandsBuilderTest < ActiveSupport::TestCase test "target multiarch local when arch is set" do builder = new_builder_command(builder: { "local" => { "arch" => "amd64" } }) - assert_equal "multiarch", builder.name + assert_equal "local", builder.name assert_equal \ - "docker buildx build --push --platform linux/amd64 --builder kamal-app-multiarch -t dhh/app:123 -t dhh/app:latest --label service=\"app\" --file Dockerfile .", + "docker buildx build --push --platform linux/amd64 --builder kamal-local -t dhh/app:123 -t dhh/app:latest --label service=\"app\" --file Dockerfile .", builder.push.join(" ") end @@ -93,7 +93,7 @@ class CommandsBuilderTest < ActiveSupport::TestCase test "build context" do builder = new_builder_command(builder: { "context" => ".." }) assert_equal \ - "docker buildx build --push --platform linux/amd64,linux/arm64 --builder kamal-app-multiarch -t dhh/app:123 -t dhh/app:latest --label service=\"app\" --file Dockerfile ..", + "docker buildx build --push --platform linux/amd64,linux/arm64 --builder kamal-local -t dhh/app:123 -t dhh/app:latest --label service=\"app\" --file Dockerfile ..", builder.push.join(" ") end @@ -107,7 +107,7 @@ class CommandsBuilderTest < ActiveSupport::TestCase test "multiarch push with build args" do builder = new_builder_command(builder: { "args" => { "a" => 1, "b" => 2 } }) assert_equal \ - "docker buildx build --push --platform linux/amd64,linux/arm64 --builder kamal-app-multiarch -t dhh/app:123 -t dhh/app:latest --label service=\"app\" --build-arg a=\"1\" --build-arg b=\"2\" --file Dockerfile .", + "docker buildx build --push --platform linux/amd64,linux/arm64 --builder kamal-local -t dhh/app:123 -t dhh/app:latest --label service=\"app\" --build-arg a=\"1\" --build-arg b=\"2\" --file Dockerfile .", builder.push.join(" ") end @@ -133,7 +133,7 @@ class CommandsBuilderTest < ActiveSupport::TestCase test "multiarch context build" do builder = new_builder_command(builder: { "context" => "./foo" }) assert_equal \ - "docker buildx build --push --platform linux/amd64,linux/arm64 --builder kamal-app-multiarch -t dhh/app:123 -t dhh/app:latest --label service=\"app\" --file Dockerfile ./foo", + "docker buildx build --push --platform linux/amd64,linux/arm64 --builder kamal-local -t dhh/app:123 -t dhh/app:latest --label service=\"app\" --file Dockerfile ./foo", builder.push.join(" ") end @@ -147,7 +147,7 @@ class CommandsBuilderTest < ActiveSupport::TestCase test "cached context build" do builder = new_builder_command(builder: { "multiarch" => false, "context" => "./foo", "cache" => { "type" => "gha" } }) assert_equal \ - "docker buildx build --push -t dhh/app:123 -t dhh/app:latest --cache-to type=gha --cache-from type=gha --label service=\"app\" --file Dockerfile ./foo", + "docker buildx build --push --builder kamal-local -t dhh/app:123 -t dhh/app:latest --cache-to type=gha --cache-from type=gha --label service=\"app\" --file Dockerfile ./foo", builder.push.join(" ") end @@ -160,7 +160,7 @@ class CommandsBuilderTest < ActiveSupport::TestCase test "multiarch context hosts" do command = new_builder_command - assert_equal "docker buildx inspect kamal-app-multiarch > /dev/null", command.context_hosts.join(" ") + assert_equal "docker buildx inspect kamal-local > /dev/null", command.context_hosts.join(" ") assert_equal "", command.config_context_hosts.join(" ") end @@ -172,7 +172,7 @@ class CommandsBuilderTest < ActiveSupport::TestCase test "native cached context hosts" do command = new_builder_command(builder: { "multiarch" => false, "cache" => { "type" => "registry" } }) - assert_equal "docker buildx inspect kamal-app-native-cached > /dev/null", command.context_hosts.join(" ") + assert_equal "docker buildx inspect kamal-local > /dev/null", command.context_hosts.join(" ") assert_equal "", command.config_context_hosts.join(" ") end