move image pushing into image-builder to get parity with Gcloud image…

… builder

app/models/build.rb

@@ -32,8 +32,6 @@ class Build < ActiveRecord::Base
 
   before_create :assign_number
 
-  attr_accessor :docker_image_id
-
   def nice_name
     name.presence || "Build #{id}"
   end

app/models/docker_builder_service.rb

@@ -2,50 +2,38 @@
 require 'docker'
 
 class DockerBuilderService
-  DIGEST_SHA_REGEX = /Digest:.*(sha256:[0-9a-f]{64})/i
-  DOCKER_REPO_REGEX = /^BUILD DIGEST: (.*@sha256:[0-9a-f]{64})/i
   include ::NewRelic::Agent::MethodTracer
 
-  attr_reader :build, :execution, :output
-
   def initialize(build)
     @build = build
     @output = OutputBuffer.new
   end
 
   def run(tag_as_latest: false)
-    return unless Rails.cache.write("build-service-#{build.id}", true, unless_exist: true, expires_in: 10.seconds)
-    build.docker_build_job&.destroy # if there's an old build job, delete it
-    build.docker_tag = build.name&.parameterize.presence || 'latest'
-    build.started_at = Time.now
-    build.docker_repo_digest = nil
-
-    job = build.create_docker_job
-    build.save!
-
-    @execution = JobExecution.new(build.git_sha, job, output: @output) do |_, tmp_dir|
-      if build_image(tmp_dir, tag_as_latest: tag_as_latest)
-        ret = true
-        unless build.docker_repo_digest
-          ret = push_image(tag_as_latest: tag_as_latest)
-          unless ENV["DOCKER_KEEP_BUILT_IMGS"] == "1"
-            @output.puts("### Deleting local docker image")
-            Docker::Image.get(build.docker_image_id).remove(force: true)
-          end
-        end
-        build.save!
-        ret
+    return unless Rails.cache.write("build-service-#{@build.id}", true, unless_exist: true, expires_in: 10.seconds)
+    @build.docker_build_job&.destroy # if there's an old build job, delete it
+    @build.docker_tag = @build.name&.parameterize.presence || 'latest'
+    @build.started_at = Time.now
+    @build.docker_repo_digest = nil
+
+    job = @build.create_docker_job
+    @build.save!
+
+    @execution = JobExecution.new(@build.git_sha, job, output: @output) do |_, tmp_dir|
+      if @build.docker_repo_digest = build_image(tmp_dir, tag_as_latest: tag_as_latest)
+        @build.save!
+        true
       else
-        output.puts("Docker build failed (image id not found in response)")
+        @output.puts("Docker build failed (image id not found in response)")
         false
       end
     end
 
-    project.repository.executor = @execution.executor
+    @build.project.repository.executor = @execution.executor
 
     @execution.on_finish do
-      build.update_column(:finished_at, Time.now)
-      Samson::Hooks.fire(:after_docker_build, build)
+      @build.update_column(:finished_at, Time.now)
+      Samson::Hooks.fire(:after_docker_build, @build)
     end
 
     JobQueue.perform_later(@execution)
@@ -62,91 +50,31 @@ def execute_build_command(tmp_dir, command)
   end
 
   def before_docker_build(tmp_dir)
-    Samson::Hooks.fire(:before_docker_repository_usage, build)
-    Samson::Hooks.fire(:before_docker_build, tmp_dir, build, @output)
-    execute_build_command(tmp_dir, build.project.build_command)
+    Samson::Hooks.fire(:before_docker_repository_usage, @build)
+    Samson::Hooks.fire(:before_docker_build, tmp_dir, @build, @output)
+    execute_build_command(tmp_dir, @build.project.build_command)
   end
   add_method_tracer :before_docker_build
 
   def build_image(tmp_dir, tag_as_latest:)
-    File.write("#{tmp_dir}/REVISION", build.git_sha)
+    File.write("#{tmp_dir}/REVISION", @build.git_sha)
 
     before_docker_build(tmp_dir)
 
-    cache = build.project.builds.
+    cache = @build.project.builds.
       where.not(docker_repo_digest: nil).
-      where(dockerfile: build.dockerfile).
+      where(dockerfile: @build.dockerfile).
       last&.docker_repo_digest
 
-    if defined?(SamsonGcloud::ImageBuilder) && build.project.build_with_gcb
-      # we do not push after this since GCR handles that
-      build.docker_repo_digest = SamsonGcloud::ImageBuilder.build_image(
-        build, tmp_dir, @execution.executor, tag_as_latest: tag_as_latest, cache_from: cache
-      )
+    builder = if defined?(SamsonGcloud::ImageBuilder) && @build.project.build_with_gcb
+      SamsonGcloud::ImageBuilder
     else
-      build.docker_image_id = ImageBuilder.build_image(
-        tmp_dir, @execution.executor, dockerfile: build.dockerfile, cache_from: cache
-      )
-    end
-  end
-  add_method_tracer :build_image
-
-  def push_image(tag_as_latest: false)
-    tag = build.docker_tag
-    tag_is_latest = (tag == 'latest')
-
-    unless build.docker_repo_digest = push_image_to_registries(tag: tag, override_tag: tag_is_latest)
-      raise Docker::Error::DockerError, "Unable to get repo digest"
-    end
-
-    if tag_as_latest && !tag_is_latest
-      push_image_to_registries tag: 'latest', override_tag: true
-    end
-    true
-  rescue Docker::Error::DockerError => e
-    @output.puts("Docker push failed: #{e.message}\n")
-    nil
-  end
-  add_method_tracer :push_image
-
-  # TODO: move to the image_builder so both have the same interface
-  def push_image_to_registries(tag:, override_tag: false)
-    digest = nil
-
-    DockerRegistry.all.each_with_index do |registry, i|
-      primary = i.zero?
-      repo = project.docker_repo(registry, build.dockerfile)
-
-      if override_tag
-        @output.puts("### Tagging and pushing Docker image to #{repo}:#{tag}")
-      else
-        @output.puts("### Pushing Docker image to #{repo} without tag")
-      end
-
-      ImageBuilder.local_docker_login do |login_commands|
-        full_tag = "#{repo}:#{tag}"
-
-        @execution.executor.quiet do
-          return nil unless @execution.executor.execute(
-            *login_commands,
-            @execution.executor.verbose_command(["docker", "tag", build.docker_image_id, full_tag].shelljoin),
-            @execution.executor.verbose_command(["docker", "push", full_tag].shelljoin)
-          )
-        end
-
-        if primary
-          # cache-from also produced digest lines, so we need to be careful
-          last = @execution.executor.output.to_s.split("\n").last.to_s
-          return nil unless sha = last[DIGEST_SHA_REGEX, 1]
-          digest = "#{repo}@#{sha}"
-        end
-      end
+      ImageBuilder
     end
 
-    digest
-  end
-
-  def project
-    @build.project
+    builder.build_image(
+      tmp_dir, @build, @execution.executor, tag_as_latest: tag_as_latest, cache_from: cache
+    )
   end
+  add_method_tracer :build_image
 end

app/models/image_builder.rb

@@ -4,7 +4,24 @@
 
 class ImageBuilder
   class << self
-    def build_image(dir, executor, dockerfile:, tag: nil, cache_from: nil)
+    DIGEST_SHA_REGEX = /Digest:.*(sha256:[0-9a-f]{64})/i
+
+    include ::NewRelic::Agent::MethodTracer
+
+    def build_image(dir, build, executor, tag_as_latest:, **args)
+      return unless image_id = build_image_locally(
+        dir, executor,
+        tag: build.docker_tag, dockerfile: build.dockerfile, **args
+      )
+      push_image(image_id, build, executor, tag_as_latest: tag_as_latest)
+    ensure
+      if image_id && ENV["DOCKER_KEEP_BUILT_IMGS"] != "1"
+        executor.output.puts("### Deleting local docker image")
+        Docker::Image.get(image_id).remove(force: true)
+      end
+    end
+
+    def build_image_locally(dir, executor, dockerfile:, tag:, cache_from:)
       local_docker_login do |login_commands|
         tag = " -t #{tag.shellescape}" if tag
         file = " -f #{dockerfile.shellescape}"
@@ -15,13 +32,13 @@ def build_image(dir, executor, dockerfile:, tag: nil, cache_from: nil)
             cache_option = " --cache-from #{cache_from.shellescape}"
           end
 
-          build = "docker build#{file}#{tag} .#{cache_option}"
+          build_command = "docker build#{file}#{tag} .#{cache_option}"
 
           return unless executor.execute(
             "cd #{dir.shellescape}",
             *login_commands,
             *pull_cache,
-            executor.verbose_command(build)
+            executor.verbose_command(build_command)
           )
         end
         executor.output.to_s.scan(/Successfully built ([a-f\d]{12,})/).last&.first
@@ -50,6 +67,60 @@ def local_docker_login
 
     private
 
+    def push_image(image_id, build, executor, tag_as_latest:)
+      tag = build.docker_tag
+      tag_is_latest = (tag == 'latest')
+
+      unless repo_digest = push_image_to_registries(image_id, build, executor, tag: tag, override_tag: tag_is_latest)
+        raise Docker::Error::DockerError, "Unable to get repo digest"
+      end
+
+      if tag_as_latest && !tag_is_latest
+        push_image_to_registries image_id, build, executor, tag: 'latest', override_tag: true
+      end
+      repo_digest
+    rescue Docker::Error::DockerError => e
+      executor.output.puts("Docker push failed: #{e.message}\n")
+      nil
+    end
+    add_method_tracer :push_image
+
+    def push_image_to_registries(image_id, build, executor, tag:, override_tag:)
+      digest = nil
+
+      DockerRegistry.all.each_with_index do |registry, i|
+        primary = i.zero?
+        repo = build.project.docker_repo(registry, build.dockerfile)
+
+        if override_tag
+          executor.output.puts("### Tagging and pushing Docker image to #{repo}:#{tag}")
+        else
+          executor.output.puts("### Pushing Docker image to #{repo} without tag")
+        end
+
+        local_docker_login do |login_commands|
+          full_tag = "#{repo}:#{tag}"
+
+          executor.quiet do
+            return nil unless executor.execute(
+              *login_commands,
+              executor.verbose_command(["docker", "tag", image_id, full_tag].shelljoin),
+              executor.verbose_command(["docker", "push", full_tag].shelljoin)
+            )
+          end
+
+          if primary
+            # cache-from also produced digest lines, so we need to be careful
+            last = executor.output.to_s.split("\n").last.to_s
+            return nil unless sha = last[DIGEST_SHA_REGEX, 1]
+            digest = "#{repo}@#{sha}"
+          end
+        end
+      end
+
+      digest
+    end
+
     # TODO: same as in config/initializers/docker.rb ... dry it up
     def docker_major_version
       @@docker_major_version ||= begin

plugins/docker_binary_builder/app/models/binary_builder.rb

@@ -141,7 +141,7 @@ def image_name
   end
 
   def create_build_image
-    ImageBuilder.build_image(@dir, @executor, dockerfile: DOCKER_BUILD_FILE, tag: image_name)
+    ImageBuilder.build_image_locally(@dir, @executor, dockerfile: DOCKER_BUILD_FILE, tag: image_name, cache_from: nil)
   end
 
   def docker_api_version

plugins/gcloud/lib/samson_gcloud/image_builder.rb

@@ -2,7 +2,7 @@
 module SamsonGcloud
   class ImageBuilder
     class << self
-      def build_image(build, dir, executor, tag_as_latest:, cache_from:)
+      def build_image(dir, build, executor, tag_as_latest:, cache_from:)
         prefix = "gcr.io/#{SamsonGcloud.project}/samson"
         fake_registry = OpenStruct.new(base: prefix)
         base = build.project.docker_repo(fake_registry, build.dockerfile)

plugins/gcloud/test/samson_gcloud/image_builder_test.rb

@@ -8,7 +8,7 @@
 
   describe ".build_image" do
     def build_image(tag_as_latest: false, cache_from: nil)
-      SamsonGcloud::ImageBuilder.build_image(build, dir, executor, tag_as_latest: tag_as_latest, cache_from: cache_from)
+      SamsonGcloud::ImageBuilder.build_image(dir, build, executor, tag_as_latest: tag_as_latest, cache_from: cache_from)
     end
 
     let(:dir) { "some-dir" }