Refactor code reponsible for creating builds

This removes duplications and extracts method that builds build-jobs
without persisting those objects, to a separate method.

7 files changed, 20 insertions, 25 deletions

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index fd6fba42a34..64723ab6b4b 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -66,10 +66,7 @@ module Ci
       # We use around_transition to create builds for next stage as soon as possible, before the `after_*` is executed
       around_transition any => [:success, :failed, :canceled] do |build, block|
         block.call
-        if build.commit
-          build.commit.create_next_builds(build)
-          build.commit.save
-        end
+        build.commit.create_next_builds(build) if build.commit
       end
 
       after_transition any => [:success, :failed, :canceled] do |build|
diff --git a/app/models/ci/commit.rb b/app/models/ci/commit.rb
index 7e6bb4f8c1b..cdab6d5f316 100644
--- a/app/models/ci/commit.rb
+++ b/app/models/ci/commit.rb
@@ -89,13 +89,22 @@ module Ci
       trigger_requests.any?
     end
 
-    def create_builds(user, trigger_request = nil)
+    def build_builds_for_stage(stage, user, status, trigger_request)
+      CreateBuildsService.new(self).execute(stage, user, status, trigger_request)
+    end
+
+    def build_builds(user, status = 'success', trigger_request = nil)
       return unless config_processor
       config_processor.stages.any? do |stage|
-        CreateBuildsService.new(self).execute(stage, user, 'success', trigger_request).present?
+        build_builds_for_stage(stage, user, status, trigger_request).present?
       end
     end
 
+    def create_builds(user, trigger_request = nil)
+      build_builds(user, 'success', trigger_request)
+      save!
+    end
+
     def create_next_builds(build)
       return unless config_processor
 
@@ -112,9 +121,11 @@ module Ci
       prior_status = prior_builds.status
 
       # create builds for next stages based
-      next_stages.any? do |stage|
-        CreateBuildsService.new(self).execute(stage, build.user, prior_status, build.trigger_request).present?
+      have_builds = next_stages.any? do |stage|
+        build_builds_for_stage(stage, build.user, prior_status, build.trigger_request).present?
       end
+
+      save! if have_builds
     end
 
     def retried
diff --git a/app/services/ci/create_trigger_request_service.rb b/app/services/ci/create_trigger_request_service.rb
index c611a963112..993acf11db9 100644
--- a/app/services/ci/create_trigger_request_service.rb
+++ b/app/services/ci/create_trigger_request_service.rb
@@ -15,7 +15,6 @@ module Ci
       )
 
       if ci_commit.create_builds(nil, trigger_request)
-        ci_commit.save
         trigger_request
       end
     end
diff --git a/app/services/create_commit_builds_service.rb b/app/services/create_commit_builds_service.rb
index aa0ec45be8c..dbfc93ff5bc 100644
--- a/app/services/create_commit_builds_service.rb
+++ b/app/services/create_commit_builds_service.rb
@@ -25,7 +25,7 @@ class CreateCommitBuildsService
 
     # Create builds for commit and
     #   skip saving pipeline when there are no builds
-    unless commit.create_builds(user)
+    unless commit.build_builds(user)
       # Save object when there are yaml errors
       unless commit.yaml_errors.present?
         commit.errors.add(:base, 'No builds created')
diff --git a/spec/features/merge_requests/created_from_fork_spec.rb b/spec/features/merge_requests/created_from_fork_spec.rb
index 16c572b3197..edc0bdec3db 100644
--- a/spec/features/merge_requests/created_from_fork_spec.rb
+++ b/spec/features/merge_requests/created_from_fork_spec.rb
@@ -34,10 +34,7 @@ feature 'Merge request created from fork' do
                                        ref: merge_request.source_branch)
     end
 
-    background do
-      pipeline.create_builds(user)
-      pipeline.save
-    end
+    background { pipeline.create_builds(user) }
 
     scenario 'user visits a pipelines page', js: true do
       visit_merge_request(merge_request)
diff --git a/spec/models/ci/commit_spec.rb b/spec/models/ci/commit_spec.rb
index a4549d40461..01d931b087e 100644
--- a/spec/models/ci/commit_spec.rb
+++ b/spec/models/ci/commit_spec.rb
@@ -55,15 +55,11 @@ describe Ci::Commit, models: true do
     let!(:commit) { FactoryGirl.create :ci_commit, project: project, ref: 'master', tag: false }
 
     def create_builds(trigger_request = nil)
-      if commit.create_builds(nil, trigger_request)
-        commit.save
-      end
+      commit.create_builds(nil, trigger_request)
     end
 
     def create_next_builds
-      if commit.create_next_builds(commit.builds.order(:id).last)
-        commit.save
-      end
+      commit.create_next_builds(commit.builds.order(:id).last)
     end
 
     it 'creates builds' do
diff --git a/spec/requests/ci/api/builds_spec.rb b/spec/requests/ci/api/builds_spec.rb
index 7eff8048667..e5124ea5ea7 100644
--- a/spec/requests/ci/api/builds_spec.rb
+++ b/spec/requests/ci/api/builds_spec.rb
@@ -22,7 +22,6 @@ describe Ci::API::API do
       it "should start a build" do
         commit = FactoryGirl.create(:ci_commit, project: project, ref: 'master')
         commit.create_builds(nil)
-        commit.save
         build = commit.builds.first
 
         post ci_api("/builds/register"), token: runner.token, info: { platform: :darwin }
@@ -59,7 +58,6 @@ describe Ci::API::API do
       it "returns options" do
         commit = FactoryGirl.create(:ci_commit, project: project, ref: 'master')
         commit.create_builds(nil)
-        commit.save
 
         post ci_api("/builds/register"), token: runner.token, info: { platform: :darwin }
 
@@ -70,7 +68,6 @@ describe Ci::API::API do
       it "returns variables" do
         commit = FactoryGirl.create(:ci_commit, project: project, ref: 'master')
         commit.create_builds(nil)
-        commit.save
         project.variables << Ci::Variable.new(key: "SECRET_KEY", value: "secret_value")
 
         post ci_api("/builds/register"), token: runner.token, info: { platform: :darwin }
@@ -90,7 +87,6 @@ describe Ci::API::API do
 
         trigger_request = FactoryGirl.create(:ci_trigger_request_with_variables, commit: commit, trigger: trigger)
         commit.create_builds(nil, trigger_request)
-        commit.save
         project.variables << Ci::Variable.new(key: "SECRET_KEY", value: "secret_value")
 
         post ci_api("/builds/register"), token: runner.token, info: { platform: :darwin }
@@ -109,7 +105,6 @@ describe Ci::API::API do
       it "returns dependent builds" do
         commit = FactoryGirl.create(:ci_commit, project: project, ref: 'master')
         commit.create_builds(nil, nil)
-        commit.save
         commit.builds.where(stage: 'test').each(&:success)
 
         post ci_api("/builds/register"), token: runner.token, info: { platform: :darwin }