From 953e590b18289005c69b72575ae6f38161ffa11b Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Thu, 16 Feb 2017 13:13:10 +0100
Subject: [PATCH] Make build clone/retry implementation more robust

---
 app/services/ci/retry_build_service.rb       | 38 ++++++++++----------
 features/steps/shared/builds.rb              |  2 +-
 spec/factories/ci/builds.rb                  | 13 ++++++-
 spec/services/ci/retry_build_service_spec.rb | 38 +++++++++++---------
 4 files changed, 54 insertions(+), 37 deletions(-)

diff --git a/app/services/ci/retry_build_service.rb b/app/services/ci/retry_build_service.rb
index 4c4518d8029..4b47ee489cf 100644
--- a/app/services/ci/retry_build_service.rb
+++ b/app/services/ci/retry_build_service.rb
@@ -1,5 +1,18 @@
 module Ci
   class RetryBuildService < ::BaseService
+    CLONE_ATTRIBUTES = %i[pipeline ref tag options commands tag_list name
+                          allow_failure stage stage_idx trigger_request
+                          yaml_variables when environment coverage_regex]
+                            .freeze
+
+    REJECT_ATTRIBUTES = %i[id status user token coverage trace runner
+                           artifacts_file artifacts_metadata artifacts_size
+                           created_at updated_at started_at finished_at
+                           queued_at erased_by erased_at].freeze
+
+    IGNORE_ATTRIBUTES = %i[trace type lock_version project target_url
+                           deploy job_id description].freeze
+
     def execute(build)
       reprocess(build).tap do |new_build|
         build.pipeline.mark_as_processable_after_stage(build.stage_idx)
@@ -17,24 +30,13 @@ module Ci
         raise Gitlab::Access::AccessDeniedError
       end
 
-      Ci::Build.create(
-        ref: build.ref,
-        tag: build.tag,
-        options: build.options,
-        commands: build.commands,
-        tag_list: build.tag_list,
-        project: build.project,
-        pipeline: build.pipeline,
-        name: build.name,
-        allow_failure: build.allow_failure,
-        stage: build.stage,
-        stage_idx: build.stage_idx,
-        trigger_request: build.trigger_request,
-        yaml_variables: build.yaml_variables,
-        when: build.when,
-        environment: build.environment,
-        coverage_regex: build.coverage_regex,
-        user: current_user)
+      attributes = CLONE_ATTRIBUTES.map do |attribute|
+        [attribute, build.send(attribute)]
+      end
+
+      attributes.push([:user, current_user])
+
+      project.builds.create(Hash[attributes])
     end
   end
 end
diff --git a/features/steps/shared/builds.rb b/features/steps/shared/builds.rb
index d008a8a26af..5bc3a1f5ac4 100644
--- a/features/steps/shared/builds.rb
+++ b/features/steps/shared/builds.rb
@@ -11,7 +11,7 @@ module SharedBuilds
 
   step 'project has a recent build' do
     @pipeline = create(:ci_empty_pipeline, project: @project, sha: @project.commit.sha, ref: 'master')
-    @build = create(:ci_build_with_coverage, pipeline: @pipeline)
+    @build = create(:ci_build, :coverage, pipeline: @pipeline)
   end
 
   step 'recent build is successful' do
diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb
index 0397d5d4001..4f671091afc 100644
--- a/spec/factories/ci/builds.rb
+++ b/spec/factories/ci/builds.rb
@@ -89,8 +89,9 @@ FactoryGirl.define do
       tag true
     end
 
-    factory :ci_build_with_coverage do
+    trait :coverage do
       coverage 99.9
+      coverage_regex '/(d+)/'
     end
 
     trait :trace do
@@ -99,6 +100,16 @@ FactoryGirl.define do
       end
     end
 
+    trait :erased do
+      erased_at Time.now
+      erased_by factory: :user
+    end
+
+    trait :queued do
+      queued_at Time.now
+      runner factory: :ci_runner
+    end
+
     trait :artifacts do
       after(:create) do |build, _|
         build.artifacts_file =
diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb
index d3cc9fea4df..93147870afe 100644
--- a/spec/services/ci/retry_build_service_spec.rb
+++ b/spec/services/ci/retry_build_service_spec.rb
@@ -12,30 +12,34 @@ describe Ci::RetryBuildService, :services do
 
   shared_examples 'build duplication' do
     let(:build) do
-      create(:ci_build, :failed, :artifacts,
-               pipeline: pipeline,
-               coverage: 90.0,
-               coverage_regex: '/(d+)/')
+      create(:ci_build, :failed, :artifacts, :erased, :trace,
+             :queued, :coverage, pipeline: pipeline)
     end
 
-    it 'clones expected attributes' do
-      clone_attributes = %w[ref tag project pipeline options commands tag_list
-                            name allow_failure stage stage_idx trigger_request
-                            yaml_variables when environment coverage_regex]
-
-      clone_attributes.each do |attribute|
-        expect(new_build.send(attribute)).to eq build.send(attribute)
+    describe 'clone attributes' do
+      described_class::CLONE_ATTRIBUTES.each do |attribute|
+        it "clones #{attribute} build attribute" do
+          expect(new_build.send(attribute)).to eq build.send(attribute)
+        end
       end
     end
 
-    it 'does not clone forbidden attributes' do
-      forbidden_attributes = %w[id status token user artifacts_file
-                                artifacts_metadata coverage]
-
-      forbidden_attributes.each do |attribute|
-        expect(new_build.send(attribute)).not_to eq build.send(attribute)
+    describe 'reject attributes' do
+      described_class::REJECT_ATTRIBUTES.each do |attribute|
+        it "does not clone #{attribute} build attribute" do
+          expect(new_build.send(attribute)).not_to eq build.send(attribute)
+        end
       end
     end
+
+    it 'has correct number of known attributes' do
+      attributes =
+        described_class::CLONE_ATTRIBUTES +
+        described_class::IGNORE_ATTRIBUTES +
+        described_class::REJECT_ATTRIBUTES
+
+      expect(attributes.size).to eq build.attributes.size
+    end
   end
 
   describe '#execute' do