From af2f56f8f742e00ddb298fadea763fd0fe7054f0 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Thu, 2 Jun 2016 12:39:15 +0200
Subject: [PATCH 01/17] Refactor ci commit pipeline to prevent implicit saves

---
 app/models/ci/commit.rb                       | 16 +++--------
 app/services/ci/create_pipeline_service.rb    | 28 +++++++++----------
 app/services/create_commit_builds_service.rb  |  2 +-
 .../create_commit_builds_service_spec.rb      |  2 +-
 4 files changed, 19 insertions(+), 29 deletions(-)

diff --git a/app/models/ci/commit.rb b/app/models/ci/commit.rb
index f22b573a94c..c682f3e570e 100644
--- a/app/models/ci/commit.rb
+++ b/app/models/ci/commit.rb
@@ -13,7 +13,7 @@ module Ci
     validate :valid_commit_sha
 
     # Invalidate object and save if when touched
-    after_touch :update_state
+    after_touch :update_state!
 
     def self.truncate_sha(sha)
       sha[0...8]
@@ -135,10 +135,10 @@ module Ci
       @config_processor ||= begin
         Ci::GitlabCiYamlProcessor.new(ci_yaml_file, project.path_with_namespace)
       rescue Ci::GitlabCiYamlProcessor::ValidationError, Psych::SyntaxError => e
-        save_yaml_error(e.message)
+        self.yaml_errors = (e.message)
         nil
       rescue
-        save_yaml_error("Undefined error")
+        self.yaml_errors = 'Undefined error'
         nil
       end
     end
@@ -159,9 +159,7 @@ module Ci
       git_commit_message =~ /(\[ci skip\])/ if git_commit_message
     end
 
-    private
-
-    def update_state
+    def update_state!
       statuses.reload
       self.status = if yaml_errors.blank?
                       statuses.latest.status || 'skipped'
@@ -173,11 +171,5 @@ module Ci
       self.duration = statuses.latest.duration
       save
     end
-
-    def save_yaml_error(error)
-      return if self.yaml_errors?
-      self.yaml_errors = error
-      update_state
-    end
   end
 end
diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index 5bc0c31cb42..0336b767de5 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -8,7 +8,9 @@ module Ci
         return pipeline
       end
 
-      unless commit
+      if commit
+        pipeline.sha = commit.id
+      else
         pipeline.errors.add(:base, 'Commit not found')
         return pipeline
       end
@@ -18,22 +20,18 @@ module Ci
         return pipeline
       end
 
-      begin
-        Ci::Commit.transaction do
-          pipeline.sha = commit.id
-
-          unless pipeline.config_processor
-            pipeline.errors.add(:base, pipeline.yaml_errors || 'Missing .gitlab-ci.yml file')
-            raise ActiveRecord::Rollback
-          end
-
-          pipeline.save!
-          pipeline.create_builds(current_user)
-        end
-      rescue
-        pipeline.errors.add(:base, 'The pipeline could not be created. Please try again.')
+      unless pipeline.config_processor
+        pipeline.errors.add(:base, pipeline.yaml_errors || 'Missing .gitlab-ci.yml file')
+        return pipeline
       end
 
+      pipeline.save!
+
+      unless pipeline.create_builds(current_user)
+        pipeline.errors.add(:base, 'No builds for this pipeline.')
+      end
+
+      pipeline.update_state!
       pipeline
     end
 
diff --git a/app/services/create_commit_builds_service.rb b/app/services/create_commit_builds_service.rb
index 5b6fefe669e..ee84023e514 100644
--- a/app/services/create_commit_builds_service.rb
+++ b/app/services/create_commit_builds_service.rb
@@ -34,7 +34,7 @@ class CreateCommitBuildsService
       commit.create_builds(user)
     end
 
-    commit.touch
+    commit.update_state!
     commit
   end
 end
diff --git a/spec/services/create_commit_builds_service_spec.rb b/spec/services/create_commit_builds_service_spec.rb
index 9ae8f31b372..e643991e0b9 100644
--- a/spec/services/create_commit_builds_service_spec.rb
+++ b/spec/services/create_commit_builds_service_spec.rb
@@ -81,7 +81,7 @@ describe CreateCommitBuildsService, services: true do
       expect(commit.yaml_errors).not_to be_nil
     end
 
-    describe :ci_skip? do
+    context 'when commit contains a [ci skip] directive' do
       let(:message) { "some message[ci skip]" }
 
       before do

From 1bcb61dd2076995b5fed786133f94def1fd637a5 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Thu, 2 Jun 2016 13:38:30 +0200
Subject: [PATCH 02/17] Add specs covering case when there are no builds

---
 .../create_commit_builds_service_spec.rb       | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/spec/services/create_commit_builds_service_spec.rb b/spec/services/create_commit_builds_service_spec.rb
index e643991e0b9..8c6b602ac83 100644
--- a/spec/services/create_commit_builds_service_spec.rb
+++ b/spec/services/create_commit_builds_service_spec.rb
@@ -171,5 +171,23 @@ describe CreateCommitBuildsService, services: true do
       expect(commit.status).to eq("failed")
       expect(commit.builds.any?).to be false
     end
+
+    context 'when there are no jobs for this pipeline' do
+      before do
+        config = YAML.dump({ test: { deploy: 'ls', only: ['feature'] } })
+        stub_ci_commit_yaml_file(config)
+      end
+
+      it 'does not create a new pipeline' do
+        result = service.execute(project, user,
+                                 ref: 'refs/heads/master',
+                                 before: '00000000',
+                                 after: '31das312',
+                                 commits: [{ message: 'some msg'}])
+
+        expect(result).to be false
+        expect(Ci::Build.all).to be_empty
+      end
+    end
   end
 end

From 0d19abf450d26fa76a23aaae38d392ecdef4e1e0 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Thu, 2 Jun 2016 13:52:19 +0200
Subject: [PATCH 03/17] Add minor improvements in create builds service

---
 app/models/ci/commit.rb                            | 2 +-
 app/services/ci/create_builds_service.rb           | 9 ++-------
 lib/ci/gitlab_ci_yaml_processor.rb                 | 5 ++++-
 spec/services/create_commit_builds_service_spec.rb | 2 +-
 4 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/app/models/ci/commit.rb b/app/models/ci/commit.rb
index c682f3e570e..ccd26959ad1 100644
--- a/app/models/ci/commit.rb
+++ b/app/models/ci/commit.rb
@@ -135,7 +135,7 @@ module Ci
       @config_processor ||= begin
         Ci::GitlabCiYamlProcessor.new(ci_yaml_file, project.path_with_namespace)
       rescue Ci::GitlabCiYamlProcessor::ValidationError, Psych::SyntaxError => e
-        self.yaml_errors = (e.message)
+        self.yaml_errors = e.message
         nil
       rescue
         self.yaml_errors = 'Undefined error'
diff --git a/app/services/ci/create_builds_service.rb b/app/services/ci/create_builds_service.rb
index 18274ce24e2..7fb2ad7e061 100644
--- a/app/services/ci/create_builds_service.rb
+++ b/app/services/ci/create_builds_service.rb
@@ -2,10 +2,11 @@ module Ci
   class CreateBuildsService
     def initialize(commit)
       @commit = commit
+      @config = commit.config_processor
     end
 
     def execute(stage, user, status, trigger_request = nil)
-      builds_attrs = config_processor.builds_for_stage_and_ref(stage, @commit.ref, @commit.tag, trigger_request)
+      builds_attrs = @config.builds_for_stage_and_ref(stage, @commit.ref, @commit.tag, trigger_request)
 
       # check when to create next build
       builds_attrs = builds_attrs.select do |build_attrs|
@@ -41,11 +42,5 @@ module Ci
         end
       end
     end
-
-    private
-
-    def config_processor
-      @config_processor ||= @commit.config_processor
-    end
   end
 end
diff --git a/lib/ci/gitlab_ci_yaml_processor.rb b/lib/ci/gitlab_ci_yaml_processor.rb
index 026a5ac97ca..fcc8af16488 100644
--- a/lib/ci/gitlab_ci_yaml_processor.rb
+++ b/lib/ci/gitlab_ci_yaml_processor.rb
@@ -27,7 +27,10 @@ module Ci
     end
 
     def builds_for_stage_and_ref(stage, ref, tag = false, trigger_request = nil)
-      builds.select{|build| build[:stage] == stage && process?(build[:only], build[:except], ref, tag, trigger_request)}
+      builds.select do |build|
+        build[:stage] == stage &&
+          process?(build[:only], build[:except], ref, tag, trigger_request)
+      end
     end
 
     def builds
diff --git a/spec/services/create_commit_builds_service_spec.rb b/spec/services/create_commit_builds_service_spec.rb
index 8c6b602ac83..dc915e9dd77 100644
--- a/spec/services/create_commit_builds_service_spec.rb
+++ b/spec/services/create_commit_builds_service_spec.rb
@@ -183,7 +183,7 @@ describe CreateCommitBuildsService, services: true do
                                  ref: 'refs/heads/master',
                                  before: '00000000',
                                  after: '31das312',
-                                 commits: [{ message: 'some msg'}])
+                                 commits: [{ message: 'some msg'} ])
 
         expect(result).to be false
         expect(Ci::Build.all).to be_empty

From 53fe06efde46acd2df62f818c421ecf3a0b971c9 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Thu, 2 Jun 2016 14:41:48 +0200
Subject: [PATCH 04/17] Update ci commit pipeline specs according to changes

---
 spec/models/ci/commit_spec.rb                    | 16 ++++++++--------
 .../create_commit_builds_service_spec.rb         |  2 +-
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/spec/models/ci/commit_spec.rb b/spec/models/ci/commit_spec.rb
index 22f8639e5ab..0939eb946ac 100644
--- a/spec/models/ci/commit_spec.rb
+++ b/spec/models/ci/commit_spec.rb
@@ -346,9 +346,9 @@ describe Ci::Commit, models: true do
     end
   end
 
-  describe '#update_state' do
-    it 'execute update_state after touching object' do
-      expect(commit).to receive(:update_state).and_return(true)
+  describe '#update_state!' do
+    it 'execute update_state! after touching object' do
+      expect(commit).to receive(:update_state!).and_return(true)
       commit.touch
     end
 
@@ -356,17 +356,17 @@ describe Ci::Commit, models: true do
       let(:commit_status) { build :commit_status, commit: commit }
 
       it 'execute update_state after saving dependent object' do
-        expect(commit).to receive(:update_state).and_return(true)
+        expect(commit).to receive(:update_state!).and_return(true)
         commit_status.save
       end
     end
 
     context 'update state' do
       let(:current) { Time.now.change(usec: 0) }
-      let(:build) { FactoryGirl.create :ci_build, :success, commit: commit, started_at: current - 120, finished_at: current - 60 }
-
-      before do
-        build
+      let!(:build) do
+        create :ci_build, :success, commit: commit,
+                                    started_at: current - 120,
+                                    finished_at: current - 60
       end
 
       [:status, :started_at, :finished_at, :duration].each do |param|
diff --git a/spec/services/create_commit_builds_service_spec.rb b/spec/services/create_commit_builds_service_spec.rb
index dc915e9dd77..b116e3e8fb4 100644
--- a/spec/services/create_commit_builds_service_spec.rb
+++ b/spec/services/create_commit_builds_service_spec.rb
@@ -183,7 +183,7 @@ describe CreateCommitBuildsService, services: true do
                                  ref: 'refs/heads/master',
                                  before: '00000000',
                                  after: '31das312',
-                                 commits: [{ message: 'some msg'} ])
+                                 commits: [{ message: 'some msg' }])
 
         expect(result).to be false
         expect(Ci::Build.all).to be_empty

From 07af37a243ea0d6b5741754ea116044ee46614b3 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Thu, 2 Jun 2016 15:55:38 +0200
Subject: [PATCH 05/17] Do not create pipeline objects when no builds

---
 app/services/ci/create_builds_service.rb      | 41 +++++++++++--------
 app/services/create_commit_builds_service.rb  | 10 ++---
 .../services/ci/create_builds_service_spec.rb |  6 ++-
 3 files changed, 34 insertions(+), 23 deletions(-)

diff --git a/app/services/ci/create_builds_service.rb b/app/services/ci/create_builds_service.rb
index 7fb2ad7e061..a02b0b8f9b3 100644
--- a/app/services/ci/create_builds_service.rb
+++ b/app/services/ci/create_builds_service.rb
@@ -20,26 +20,33 @@ module Ci
         end
       end
 
+      # don't create the same build twice
+      builds_attrs.reject! do |build_attrs|
+        @commit.builds.find_by(ref: @commit.ref, tag: @commit.tag,
+                               trigger_request: trigger_request,
+                               name: build_attrs[:name])
+      end
+
       builds_attrs.map do |build_attrs|
-        # don't create the same build twice
-        unless @commit.builds.find_by(ref: @commit.ref, tag: @commit.tag,
-                                      trigger_request: trigger_request, name: build_attrs[:name])
-          build_attrs.slice!(:name,
-                             :commands,
-                             :tag_list,
-                             :options,
-                             :allow_failure,
-                             :stage,
-                             :stage_idx)
+        build_attrs.slice!(:name,
+                           :commands,
+                           :tag_list,
+                           :options,
+                           :allow_failure,
+                           :stage,
+                           :stage_idx)
 
-          build_attrs.merge!(ref: @commit.ref,
-                             tag: @commit.tag,
-                             trigger_request: trigger_request,
-                             user: user,
-                             project: @commit.project)
+        build_attrs.merge!(ref: @commit.ref,
+                           tag: @commit.tag,
+                           trigger_request: trigger_request,
+                           user: user,
+                           project: @commit.project)
 
-          @commit.builds.create!(build_attrs)
-        end
+        ##
+        # We do not persist new builds here.
+        # Those will be persisted when @commit is saved.
+        #
+        @commit.builds.new(build_attrs)
       end
     end
   end
diff --git a/app/services/create_commit_builds_service.rb b/app/services/create_commit_builds_service.rb
index ee84023e514..3f048157b3f 100644
--- a/app/services/create_commit_builds_service.rb
+++ b/app/services/create_commit_builds_service.rb
@@ -25,16 +25,16 @@ class CreateCommitBuildsService
       return false
     end
 
-    # Create a new ci_commit
-    commit.save!
 
     # Skip creating builds for commits that have [ci skip]
     unless commit.skip_ci?
-      # Create builds for commit
-      commit.create_builds(user)
+      # Create builds for commit and
+      #   skip saving pipeline when there are no builds
+      return false unless commit.create_builds(user)
     end
 
-    commit.update_state!
+    # Create a new ci_commit
+    commit.save!
     commit
   end
 end
diff --git a/spec/services/ci/create_builds_service_spec.rb b/spec/services/ci/create_builds_service_spec.rb
index ecc3a88a262..8e737fd44f9 100644
--- a/spec/services/ci/create_builds_service_spec.rb
+++ b/spec/services/ci/create_builds_service_spec.rb
@@ -9,7 +9,7 @@ describe Ci::CreateBuildsService, services: true do
     #
 
     subject do
-      described_class.new(commit).execute(commit, nil, user, status)
+      described_class.new(commit).execute('test', user, status, nil)
     end
 
     context 'next builds available' do
@@ -17,6 +17,10 @@ describe Ci::CreateBuildsService, services: true do
 
       it { is_expected.to be_an_instance_of Array }
       it { is_expected.to all(be_an_instance_of Ci::Build) }
+
+      it 'does not persist created builds' do
+        expect(subject.first).not_to be_persisted
+      end
     end
 
     context 'builds skipped' do

From c6bce7e63c305d07dbc91d032df9c783e0cf0c9f Mon Sep 17 00:00:00 2001
From: Kamil Trzcinski <ayufan@ayufan.eu>
Date: Thu, 2 Jun 2016 17:17:23 +0200
Subject: [PATCH 06/17] Save Ci::Commit object to persist all created builds

---
 app/models/ci/build.rb                            | 5 ++++-
 app/models/ci/commit.rb                           | 6 ++++--
 app/services/ci/create_pipeline_service.rb        | 2 +-
 app/services/ci/create_trigger_request_service.rb | 1 +
 spec/models/ci/commit_spec.rb                     | 8 ++++++--
 spec/requests/ci/api/builds_spec.rb               | 5 +++++
 6 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 64723ab6b4b..fd6fba42a34 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -66,7 +66,10 @@ 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
-        build.commit.create_next_builds(build) if build.commit
+        if build.commit
+          build.commit.create_next_builds(build)
+          build.commit.save
+        end
       end
 
       after_transition any => [:success, :failed, :canceled] do |build|
diff --git a/app/models/ci/commit.rb b/app/models/ci/commit.rb
index ccd26959ad1..7e6bb4f8c1b 100644
--- a/app/models/ci/commit.rb
+++ b/app/models/ci/commit.rb
@@ -13,7 +13,7 @@ module Ci
     validate :valid_commit_sha
 
     # Invalidate object and save if when touched
-    after_touch :update_state!
+    after_touch :update_state
 
     def self.truncate_sha(sha)
       sha[0...8]
@@ -159,7 +159,9 @@ module Ci
       git_commit_message =~ /(\[ci skip\])/ if git_commit_message
     end
 
-    def update_state!
+    private
+
+    def update_state
       statuses.reload
       self.status = if yaml_errors.blank?
                       statuses.latest.status || 'skipped'
diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index 0336b767de5..864415ef747 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -31,7 +31,7 @@ module Ci
         pipeline.errors.add(:base, 'No builds for this pipeline.')
       end
 
-      pipeline.update_state!
+      pipeline.save
       pipeline
     end
 
diff --git a/app/services/ci/create_trigger_request_service.rb b/app/services/ci/create_trigger_request_service.rb
index 993acf11db9..c611a963112 100644
--- a/app/services/ci/create_trigger_request_service.rb
+++ b/app/services/ci/create_trigger_request_service.rb
@@ -15,6 +15,7 @@ module Ci
       )
 
       if ci_commit.create_builds(nil, trigger_request)
+        ci_commit.save
         trigger_request
       end
     end
diff --git a/spec/models/ci/commit_spec.rb b/spec/models/ci/commit_spec.rb
index 0939eb946ac..07b875e4f88 100644
--- a/spec/models/ci/commit_spec.rb
+++ b/spec/models/ci/commit_spec.rb
@@ -55,11 +55,15 @@ describe Ci::Commit, models: true do
     let!(:commit) { FactoryGirl.create :ci_commit, project: project, ref: 'master', tag: false }
 
     def create_builds(trigger_request = nil)
-      commit.create_builds(nil, trigger_request)
+      if commit.create_builds(nil, trigger_request)
+        commit.save
+      end
     end
 
     def create_next_builds
-      commit.create_next_builds(commit.builds.order(:id).last)
+      if commit.create_next_builds(commit.builds.order(:id).last)
+        commit.save
+      end
     end
 
     it 'creates builds' do
diff --git a/spec/requests/ci/api/builds_spec.rb b/spec/requests/ci/api/builds_spec.rb
index e5124ea5ea7..7eff8048667 100644
--- a/spec/requests/ci/api/builds_spec.rb
+++ b/spec/requests/ci/api/builds_spec.rb
@@ -22,6 +22,7 @@ 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 }
@@ -58,6 +59,7 @@ 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 }
 
@@ -68,6 +70,7 @@ 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 }
@@ -87,6 +90,7 @@ 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 }
@@ -105,6 +109,7 @@ 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 }

From 8e811f2c6c4c74a30789ff5213de5ebc28897753 Mon Sep 17 00:00:00 2001
From: Kamil Trzcinski <ayufan@ayufan.eu>
Date: Thu, 2 Jun 2016 21:02:52 +0200
Subject: [PATCH 07/17] Update CreateCommitBuildsService to pass tests

---
 app/services/ci/create_builds_service.rb      |  3 +-
 app/services/create_commit_builds_service.rb  | 35 ++++++++++---------
 spec/models/ci/commit_spec.rb                 |  6 ++--
 .../create_commit_builds_service_spec.rb      |  8 ++---
 4 files changed, 27 insertions(+), 25 deletions(-)

diff --git a/app/services/ci/create_builds_service.rb b/app/services/ci/create_builds_service.rb
index a02b0b8f9b3..f458dee49a6 100644
--- a/app/services/ci/create_builds_service.rb
+++ b/app/services/ci/create_builds_service.rb
@@ -36,7 +36,8 @@ module Ci
                            :stage,
                            :stage_idx)
 
-        build_attrs.merge!(ref: @commit.ref,
+        build_attrs.merge!(commit: @commit,
+                           ref: @commit.ref,
                            tag: @commit.tag,
                            trigger_request: trigger_request,
                            user: user,
diff --git a/app/services/create_commit_builds_service.rb b/app/services/create_commit_builds_service.rb
index 3f048157b3f..aa0ec45be8c 100644
--- a/app/services/create_commit_builds_service.rb
+++ b/app/services/create_commit_builds_service.rb
@@ -1,40 +1,41 @@
 class CreateCommitBuildsService
   def execute(project, user, params)
-    return false unless project.builds_enabled?
+    return unless project.builds_enabled?
 
     before_sha = params[:checkout_sha] || params[:before]
     sha = params[:checkout_sha] || params[:after]
     origin_ref = params[:ref]
 
-    unless origin_ref && sha.present?
-      return false
-    end
-
     ref = Gitlab::Git.ref_name(origin_ref)
     tag = Gitlab::Git.tag_ref?(origin_ref)
 
-    # Skip branch removal
-    if sha == Gitlab::Git::BLANK_SHA
-      return false
-    end
-
     commit = Ci::Commit.new(project: project, sha: sha, ref: ref, before_sha: before_sha, tag: tag)
 
-    # Skip creating ci_commit when no gitlab-ci.yml is found
     unless commit.ci_yaml_file
-      return false
+      commit.errors.add(:base, 'No .gitlab-ci.yml file found')
+      return commit
     end
 
+    # Make object as skipped
+    if commit.skip_ci?
+      commit.status = 'skipped'
+      commit.save
+      return commit
+    end
 
-    # Skip creating builds for commits that have [ci skip]
-    unless commit.skip_ci?
-      # Create builds for commit and
-      #   skip saving pipeline when there are no builds
-      return false unless commit.create_builds(user)
+    # Create builds for commit and
+    #   skip saving pipeline when there are no builds
+    unless commit.create_builds(user)
+      # Save object when there are yaml errors
+      unless commit.yaml_errors.present?
+        commit.errors.add(:base, 'No builds created')
+        return commit
+      end
     end
 
     # Create a new ci_commit
     commit.save!
+    commit.touch
     commit
   end
 end
diff --git a/spec/models/ci/commit_spec.rb b/spec/models/ci/commit_spec.rb
index 07b875e4f88..d36aca113a1 100644
--- a/spec/models/ci/commit_spec.rb
+++ b/spec/models/ci/commit_spec.rb
@@ -351,8 +351,8 @@ describe Ci::Commit, models: true do
   end
 
   describe '#update_state!' do
-    it 'execute update_state! after touching object' do
-      expect(commit).to receive(:update_state!).and_return(true)
+    it 'execute update_state after touching object' do
+      expect(commit).to receive(:update_state).and_return(true)
       commit.touch
     end
 
@@ -360,7 +360,7 @@ describe Ci::Commit, models: true do
       let(:commit_status) { build :commit_status, commit: commit }
 
       it 'execute update_state after saving dependent object' do
-        expect(commit).to receive(:update_state!).and_return(true)
+        expect(commit).to receive(:update_state).and_return(true)
         commit_status.save
       end
     end
diff --git a/spec/services/create_commit_builds_service_spec.rb b/spec/services/create_commit_builds_service_spec.rb
index b116e3e8fb4..e3202c959d9 100644
--- a/spec/services/create_commit_builds_service_spec.rb
+++ b/spec/services/create_commit_builds_service_spec.rb
@@ -60,7 +60,7 @@ describe CreateCommitBuildsService, services: true do
                                after: '31das312',
                                commits: [{ message: 'Message' }]
                               )
-      expect(result).to be_falsey
+      expect(result).not_to be_persisted
       expect(Ci::Commit.count).to eq(0)
     end
 
@@ -174,7 +174,7 @@ describe CreateCommitBuildsService, services: true do
 
     context 'when there are no jobs for this pipeline' do
       before do
-        config = YAML.dump({ test: { deploy: 'ls', only: ['feature'] } })
+        config = YAML.dump({ test: { script: 'ls', only: ['feature'] } })
         stub_ci_commit_yaml_file(config)
       end
 
@@ -184,9 +184,9 @@ describe CreateCommitBuildsService, services: true do
                                  before: '00000000',
                                  after: '31das312',
                                  commits: [{ message: 'some msg' }])
-
-        expect(result).to be false
+        expect(result).not_to be_persisted
         expect(Ci::Build.all).to be_empty
+        expect(Ci::Commit.count).to eq(0)
       end
     end
   end

From 681b472c36d5079b620b93957d62dbacc473bf6f Mon Sep 17 00:00:00 2001
From: Kamil Trzcinski <ayufan@ayufan.eu>
Date: Thu, 2 Jun 2016 21:04:45 +0200
Subject: [PATCH 08/17] Update specs describe

---
 spec/models/ci/commit_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/models/ci/commit_spec.rb b/spec/models/ci/commit_spec.rb
index d36aca113a1..a4549d40461 100644
--- a/spec/models/ci/commit_spec.rb
+++ b/spec/models/ci/commit_spec.rb
@@ -350,7 +350,7 @@ describe Ci::Commit, models: true do
     end
   end
 
-  describe '#update_state!' do
+  describe '#update_state' do
     it 'execute update_state after touching object' do
       expect(commit).to receive(:update_state).and_return(true)
       commit.touch

From a21d084ded6cdf5b83163d4d72bb5c636218d091 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Fri, 3 Jun 2016 10:01:00 +0200
Subject: [PATCH 09/17] Fix specs for pipeline create for merge requests

---
 spec/features/merge_requests/created_from_fork_spec.rb | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/spec/features/merge_requests/created_from_fork_spec.rb b/spec/features/merge_requests/created_from_fork_spec.rb
index edc0bdec3db..16c572b3197 100644
--- a/spec/features/merge_requests/created_from_fork_spec.rb
+++ b/spec/features/merge_requests/created_from_fork_spec.rb
@@ -34,7 +34,10 @@ feature 'Merge request created from fork' do
                                        ref: merge_request.source_branch)
     end
 
-    background { pipeline.create_builds(user) }
+    background do
+      pipeline.create_builds(user)
+      pipeline.save
+    end
 
     scenario 'user visits a pipelines page', js: true do
       visit_merge_request(merge_request)

From d8c4556d3c8623bf48e689f3734c9c35cda34c2f Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Fri, 3 Jun 2016 11:58:08 +0200
Subject: [PATCH 10/17] Refactor code reponsible for creating builds

This removes duplications and extracts method that builds build-jobs
without persisting those objects, to a separate method.
---
 app/models/ci/build.rb                        |  5 +----
 app/models/ci/commit.rb                       | 19 +++++++++++++++----
 .../ci/create_trigger_request_service.rb      |  1 -
 app/services/create_commit_builds_service.rb  |  2 +-
 .../merge_requests/created_from_fork_spec.rb  |  5 +----
 spec/models/ci/commit_spec.rb                 |  8 ++------
 spec/requests/ci/api/builds_spec.rb           |  5 -----
 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 }

From e45fd5a1e4efb805d3f7f5ed9cb708105c4e9d60 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 14 Jun 2016 14:21:24 +0200
Subject: [PATCH 11/17] Remove ci commit specs that remain after bad merge

---
 spec/models/ci/commit_spec.rb | 403 ----------------------------------
 1 file changed, 403 deletions(-)
 delete mode 100644 spec/models/ci/commit_spec.rb

diff --git a/spec/models/ci/commit_spec.rb b/spec/models/ci/commit_spec.rb
deleted file mode 100644
index 01d931b087e..00000000000
--- a/spec/models/ci/commit_spec.rb
+++ /dev/null
@@ -1,403 +0,0 @@
-require 'spec_helper'
-
-describe Ci::Commit, models: true do
-  let(:project) { FactoryGirl.create :empty_project }
-  let(:commit) { FactoryGirl.create :ci_commit, project: project }
-
-  it { is_expected.to belong_to(:project) }
-  it { is_expected.to have_many(:statuses) }
-  it { is_expected.to have_many(:trigger_requests) }
-  it { is_expected.to have_many(:builds) }
-  it { is_expected.to validate_presence_of :sha }
-  it { is_expected.to validate_presence_of :status }
-
-  it { is_expected.to respond_to :git_author_name }
-  it { is_expected.to respond_to :git_author_email }
-  it { is_expected.to respond_to :short_sha }
-
-  describe :valid_commit_sha do
-    context 'commit.sha can not start with 00000000' do
-      before do
-        commit.sha = '0' * 40
-        commit.valid_commit_sha
-      end
-
-      it('commit errors should not be empty') { expect(commit.errors).not_to be_empty }
-    end
-  end
-
-  describe :short_sha do
-    subject { commit.short_sha }
-
-    it 'has 8 items' do
-      expect(subject.size).to eq(8)
-    end
-    it { expect(commit.sha).to start_with(subject) }
-  end
-
-  describe :create_next_builds do
-  end
-
-  describe :retried do
-    subject { commit.retried }
-
-    before do
-      @commit1 = FactoryGirl.create :ci_build, commit: commit, name: 'deploy'
-      @commit2 = FactoryGirl.create :ci_build, commit: commit, name: 'deploy'
-    end
-
-    it 'returns old builds' do
-      is_expected.to contain_exactly(@commit1)
-    end
-  end
-
-  describe :create_builds do
-    let!(:commit) { FactoryGirl.create :ci_commit, project: project, ref: 'master', tag: false }
-
-    def create_builds(trigger_request = nil)
-      commit.create_builds(nil, trigger_request)
-    end
-
-    def create_next_builds
-      commit.create_next_builds(commit.builds.order(:id).last)
-    end
-
-    it 'creates builds' do
-      expect(create_builds).to be_truthy
-      commit.builds.update_all(status: "success")
-      expect(commit.builds.count(:all)).to eq(2)
-
-      expect(create_next_builds).to be_truthy
-      commit.builds.update_all(status: "success")
-      expect(commit.builds.count(:all)).to eq(4)
-
-      expect(create_next_builds).to be_truthy
-      commit.builds.update_all(status: "success")
-      expect(commit.builds.count(:all)).to eq(5)
-
-      expect(create_next_builds).to be_falsey
-    end
-
-    context 'custom stage with first job allowed to fail' do
-      let(:yaml) do
-        {
-          stages: ['clean', 'test'],
-          clean_job: {
-            stage: 'clean',
-            allow_failure: true,
-            script: 'BUILD',
-          },
-          test_job: {
-            stage: 'test',
-            script: 'TEST',
-          },
-        }
-      end
-
-      before do
-        stub_ci_commit_yaml_file(YAML.dump(yaml))
-        create_builds
-      end
-
-      it 'properly schedules builds' do
-        expect(commit.builds.pluck(:status)).to contain_exactly('pending')
-        commit.builds.running_or_pending.each(&:drop)
-        expect(commit.builds.pluck(:status)).to contain_exactly('pending', 'failed')
-      end
-    end
-
-    context 'properly creates builds when "when" is defined' do
-      let(:yaml) do
-        {
-          stages: ["build", "test", "test_failure", "deploy", "cleanup"],
-          build: {
-            stage: "build",
-            script: "BUILD",
-          },
-          test: {
-            stage: "test",
-            script: "TEST",
-          },
-          test_failure: {
-            stage: "test_failure",
-            script: "ON test failure",
-            when: "on_failure",
-          },
-          deploy: {
-            stage: "deploy",
-            script: "PUBLISH",
-          },
-          cleanup: {
-            stage: "cleanup",
-            script: "TIDY UP",
-            when: "always",
-          }
-        }
-      end
-
-      before do
-        stub_ci_commit_yaml_file(YAML.dump(yaml))
-      end
-
-      context 'when builds are successful' do
-        it 'properly creates builds' do
-          expect(create_builds).to be_truthy
-          expect(commit.builds.pluck(:name)).to contain_exactly('build')
-          expect(commit.builds.pluck(:status)).to contain_exactly('pending')
-          commit.builds.running_or_pending.each(&:success)
-
-          expect(commit.builds.pluck(:name)).to contain_exactly('build', 'test')
-          expect(commit.builds.pluck(:status)).to contain_exactly('success', 'pending')
-          commit.builds.running_or_pending.each(&:success)
-
-          expect(commit.builds.pluck(:name)).to contain_exactly('build', 'test', 'deploy')
-          expect(commit.builds.pluck(:status)).to contain_exactly('success', 'success', 'pending')
-          commit.builds.running_or_pending.each(&:success)
-
-          expect(commit.builds.pluck(:name)).to contain_exactly('build', 'test', 'deploy', 'cleanup')
-          expect(commit.builds.pluck(:status)).to contain_exactly('success', 'success', 'success', 'pending')
-          commit.builds.running_or_pending.each(&:success)
-
-          expect(commit.builds.pluck(:status)).to contain_exactly('success', 'success', 'success', 'success')
-          commit.reload
-          expect(commit.status).to eq('success')
-        end
-      end
-
-      context 'when test job fails' do
-        it 'properly creates builds' do
-          expect(create_builds).to be_truthy
-          expect(commit.builds.pluck(:name)).to contain_exactly('build')
-          expect(commit.builds.pluck(:status)).to contain_exactly('pending')
-          commit.builds.running_or_pending.each(&:success)
-
-          expect(commit.builds.pluck(:name)).to contain_exactly('build', 'test')
-          expect(commit.builds.pluck(:status)).to contain_exactly('success', 'pending')
-          commit.builds.running_or_pending.each(&:drop)
-
-          expect(commit.builds.pluck(:name)).to contain_exactly('build', 'test', 'test_failure')
-          expect(commit.builds.pluck(:status)).to contain_exactly('success', 'failed', 'pending')
-          commit.builds.running_or_pending.each(&:success)
-
-          expect(commit.builds.pluck(:name)).to contain_exactly('build', 'test', 'test_failure', 'cleanup')
-          expect(commit.builds.pluck(:status)).to contain_exactly('success', 'failed', 'success', 'pending')
-          commit.builds.running_or_pending.each(&:success)
-
-          expect(commit.builds.pluck(:status)).to contain_exactly('success', 'failed', 'success', 'success')
-          commit.reload
-          expect(commit.status).to eq('failed')
-        end
-      end
-
-      context 'when test and test_failure jobs fail' do
-        it 'properly creates builds' do
-          expect(create_builds).to be_truthy
-          expect(commit.builds.pluck(:name)).to contain_exactly('build')
-          expect(commit.builds.pluck(:status)).to contain_exactly('pending')
-          commit.builds.running_or_pending.each(&:success)
-
-          expect(commit.builds.pluck(:name)).to contain_exactly('build', 'test')
-          expect(commit.builds.pluck(:status)).to contain_exactly('success', 'pending')
-          commit.builds.running_or_pending.each(&:drop)
-
-          expect(commit.builds.pluck(:name)).to contain_exactly('build', 'test', 'test_failure')
-          expect(commit.builds.pluck(:status)).to contain_exactly('success', 'failed', 'pending')
-          commit.builds.running_or_pending.each(&:drop)
-
-          expect(commit.builds.pluck(:name)).to contain_exactly('build', 'test', 'test_failure', 'cleanup')
-          expect(commit.builds.pluck(:status)).to contain_exactly('success', 'failed', 'failed', 'pending')
-          commit.builds.running_or_pending.each(&:success)
-
-          expect(commit.builds.pluck(:name)).to contain_exactly('build', 'test', 'test_failure', 'cleanup')
-          expect(commit.builds.pluck(:status)).to contain_exactly('success', 'failed', 'failed', 'success')
-          commit.reload
-          expect(commit.status).to eq('failed')
-        end
-      end
-
-      context 'when deploy job fails' do
-        it 'properly creates builds' do
-          expect(create_builds).to be_truthy
-          expect(commit.builds.pluck(:name)).to contain_exactly('build')
-          expect(commit.builds.pluck(:status)).to contain_exactly('pending')
-          commit.builds.running_or_pending.each(&:success)
-
-          expect(commit.builds.pluck(:name)).to contain_exactly('build', 'test')
-          expect(commit.builds.pluck(:status)).to contain_exactly('success', 'pending')
-          commit.builds.running_or_pending.each(&:success)
-
-          expect(commit.builds.pluck(:name)).to contain_exactly('build', 'test', 'deploy')
-          expect(commit.builds.pluck(:status)).to contain_exactly('success', 'success', 'pending')
-          commit.builds.running_or_pending.each(&:drop)
-
-          expect(commit.builds.pluck(:name)).to contain_exactly('build', 'test', 'deploy', 'cleanup')
-          expect(commit.builds.pluck(:status)).to contain_exactly('success', 'success', 'failed', 'pending')
-          commit.builds.running_or_pending.each(&:success)
-
-          expect(commit.builds.pluck(:status)).to contain_exactly('success', 'success', 'failed', 'success')
-          commit.reload
-          expect(commit.status).to eq('failed')
-        end
-      end
-
-      context 'when build is canceled in the second stage' do
-        it 'does not schedule builds after build has been canceled' do
-          expect(create_builds).to be_truthy
-          expect(commit.builds.pluck(:name)).to contain_exactly('build')
-          expect(commit.builds.pluck(:status)).to contain_exactly('pending')
-          commit.builds.running_or_pending.each(&:success)
-
-          expect(commit.builds.running_or_pending).not_to be_empty
-
-          expect(commit.builds.pluck(:name)).to contain_exactly('build', 'test')
-          expect(commit.builds.pluck(:status)).to contain_exactly('success', 'pending')
-          commit.builds.running_or_pending.each(&:cancel)
-
-          expect(commit.builds.running_or_pending).to be_empty
-          expect(commit.reload.status).to eq('canceled')
-        end
-      end
-    end
-  end
-
-  describe "#finished_at" do
-    let(:commit) { FactoryGirl.create :ci_commit }
-
-    it "returns finished_at of latest build" do
-      build = FactoryGirl.create :ci_build, commit: commit, finished_at: Time.now - 60
-      FactoryGirl.create :ci_build, commit: commit, finished_at: Time.now - 120
-
-      expect(commit.finished_at.to_i).to eq(build.finished_at.to_i)
-    end
-
-    it "returns nil if there is no finished build" do
-      FactoryGirl.create :ci_not_started_build, commit: commit
-
-      expect(commit.finished_at).to be_nil
-    end
-  end
-
-  describe "coverage" do
-    let(:project) { FactoryGirl.create :empty_project, build_coverage_regex: "/.*/" }
-    let(:commit) { FactoryGirl.create :ci_commit, project: project }
-
-    it "calculates average when there are two builds with coverage" do
-      FactoryGirl.create :ci_build, name: "rspec", coverage: 30, commit: commit
-      FactoryGirl.create :ci_build, name: "rubocop", coverage: 40, commit: commit
-      expect(commit.coverage).to eq("35.00")
-    end
-
-    it "calculates average when there are two builds with coverage and one with nil" do
-      FactoryGirl.create :ci_build, name: "rspec", coverage: 30, commit: commit
-      FactoryGirl.create :ci_build, name: "rubocop", coverage: 40, commit: commit
-      FactoryGirl.create :ci_build, commit: commit
-      expect(commit.coverage).to eq("35.00")
-    end
-
-    it "calculates average when there are two builds with coverage and one is retried" do
-      FactoryGirl.create :ci_build, name: "rspec", coverage: 30, commit: commit
-      FactoryGirl.create :ci_build, name: "rubocop", coverage: 30, commit: commit
-      FactoryGirl.create :ci_build, name: "rubocop", coverage: 40, commit: commit
-      expect(commit.coverage).to eq("35.00")
-    end
-
-    it "calculates average when there is one build without coverage" do
-      FactoryGirl.create :ci_build, commit: commit
-      expect(commit.coverage).to be_nil
-    end
-  end
-
-  describe '#retryable?' do
-    subject { commit.retryable? }
-
-    context 'no failed builds' do
-      before do
-        FactoryGirl.create :ci_build, name: "rspec", commit: commit, status: 'success'
-      end
-
-      it 'be not retryable' do
-        is_expected.to be_falsey
-      end
-    end
-
-    context 'with failed builds' do
-      before do
-        FactoryGirl.create :ci_build, name: "rspec", commit: commit, status: 'running'
-        FactoryGirl.create :ci_build, name: "rubocop", commit: commit, status: 'failed'
-      end
-
-      it 'be retryable' do
-        is_expected.to be_truthy
-      end
-    end
-  end
-
-  describe '#stages' do
-    let(:commit2) { FactoryGirl.create :ci_commit, project: project }
-    subject { CommitStatus.where(commit: [commit, commit2]).stages }
-
-    before do
-      FactoryGirl.create :ci_build, commit: commit2, stage: 'test', stage_idx: 1
-      FactoryGirl.create :ci_build, commit: commit, stage: 'build', stage_idx: 0
-    end
-
-    it 'return all stages' do
-      is_expected.to eq(%w(build test))
-    end
-  end
-
-  describe '#update_state' do
-    it 'execute update_state after touching object' do
-      expect(commit).to receive(:update_state).and_return(true)
-      commit.touch
-    end
-
-    context 'dependent objects' do
-      let(:commit_status) { build :commit_status, commit: commit }
-
-      it 'execute update_state after saving dependent object' do
-        expect(commit).to receive(:update_state).and_return(true)
-        commit_status.save
-      end
-    end
-
-    context 'update state' do
-      let(:current) { Time.now.change(usec: 0) }
-      let!(:build) do
-        create :ci_build, :success, commit: commit,
-                                    started_at: current - 120,
-                                    finished_at: current - 60
-      end
-
-      [:status, :started_at, :finished_at, :duration].each do |param|
-        it "update #{param}" do
-          expect(commit.send(param)).to eq(build.send(param))
-        end
-      end
-    end
-  end
-
-  describe '#branch?' do
-    subject { commit.branch? }
-
-    context 'is not a tag' do
-      before do
-        commit.tag = false
-      end
-
-      it 'return true when tag is set to false' do
-        is_expected.to be_truthy
-      end
-    end
-
-    context 'is not a tag' do
-      before do
-        commit.tag = true
-      end
-
-      it 'return false when tag is set to true' do
-        is_expected.to be_falsey
-      end
-    end
-  end
-end

From cf292a3f1d3cc17d55131a15d91a53ff31017f5d Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 14 Jun 2016 14:09:07 +0200
Subject: [PATCH 12/17] Improve code clarity in pipeline create service

---
 app/models/ci/pipeline.rb                     |  2 +-
 app/services/create_commit_builds_service.rb  | 52 ++++++++++++++-----
 .../create_commit_builds_service_spec.rb      |  4 +-
 3 files changed, 41 insertions(+), 17 deletions(-)

diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index 83d683b63e4..63639ff2c1f 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -100,7 +100,7 @@ module Ci
 
     def create_builds(user, trigger_request = nil)
       build_builds(user, 'success', trigger_request)
-      save!
+      save
     end
 
     def create_next_builds(build)
diff --git a/app/services/create_commit_builds_service.rb b/app/services/create_commit_builds_service.rb
index f2a537c595e..668d0a86549 100644
--- a/app/services/create_commit_builds_service.rb
+++ b/app/services/create_commit_builds_service.rb
@@ -14,26 +14,50 @@ class CreateCommitBuildsService
       return false
     end
 
-    pipeline = Ci::Pipeline.new(project: project, sha: sha, ref: ref, before_sha: before_sha, tag: tag)
+    @pipeline = Ci::Pipeline.new(project: project, sha: sha, ref: ref, before_sha: before_sha, tag: tag)
 
-    # Skip creating pipeline when no gitlab-ci.yml is found
-    unless pipeline.ci_yaml_file
-      return pipeline
+    ##
+    # Skip creating pipeline if no gitlab-ci.yml is found
+    #
+    unless @pipeline.ci_yaml_file
+      return false
     end
 
+    ##
     # Skip creating builds for commits that have [ci skip]
-   if !pipeline.skip_ci? && pipeline.config_processor
-      # Create builds for commit
-      unless pipeline.build_builds(user)
-        pipeline.errors.add(:base, 'No builds created')
-        return pipeline
-      end
+    # but save pipeline object
+    #
+    if @pipeline.skip_ci?
+      return save_pipeline!
     end
 
-    # Create a new pipeline
-    pipeline.save!
+    ##
+    # Skip creating builds when CI config is invalid
+    # but save pipeline object
+    #
+    unless @pipeline.config_processor
+      return save_pipeline!
+    end
 
-    pipeline.touch
-    pipeline
+    ##
+    # Skip creating pipeline object if there are no builds for it.
+    #
+    unless @pipeline.build_builds(user)
+      @pipeline.errors.add(:base, 'No builds created')
+      return false
+    end
+
+    save_pipeline!
+  end
+
+  private
+
+  ##
+  # Create a new pipeline and touch object to calculate status
+  #
+  def save_pipeline!
+    @pipeline.save!
+    @pipeline.touch
+    @pipeline
   end
 end
diff --git a/spec/services/create_commit_builds_service_spec.rb b/spec/services/create_commit_builds_service_spec.rb
index 08cbc9beb5c..50ce9659c10 100644
--- a/spec/services/create_commit_builds_service_spec.rb
+++ b/spec/services/create_commit_builds_service_spec.rb
@@ -60,7 +60,7 @@ describe CreateCommitBuildsService, services: true do
                                after: '31das312',
                                commits: [{ message: 'Message' }]
                               )
-      expect(result).not_to be_persisted
+      expect(result).to be_falsey
       expect(Ci::Pipeline.count).to eq(0)
     end
 
@@ -184,7 +184,7 @@ describe CreateCommitBuildsService, services: true do
                                  before: '00000000',
                                  after: '31das312',
                                  commits: [{ message: 'some msg' }])
-        expect(result).not_to be_persisted
+        expect(result).to be_falsey
         expect(Ci::Build.all).to be_empty
         expect(Ci::Pipeline.count).to eq(0)
       end

From bf990fcda4c5b728272d3775cdefadce6f80cf01 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Wed, 15 Jun 2016 09:35:11 +0200
Subject: [PATCH 13/17] Return false in create_builds if not builds created

This fixes compatibility with trigger request create service.
---
 app/models/ci/pipeline.rb       |  5 ++---
 spec/models/ci/pipeline_spec.rb | 10 ++++++++++
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index 63639ff2c1f..e90924af312 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -91,7 +91,7 @@ module Ci
       trigger_requests.any?
     end
 
-    def build_builds(user, status = 'success', trigger_request = nil)
+    def build_builds(user, trigger_request = nil, status = 'success')
       return unless config_processor
       config_processor.stages.any? do |stage|
         build_builds_for_stage(stage, user, status, trigger_request).present?
@@ -99,8 +99,7 @@ module Ci
     end
 
     def create_builds(user, trigger_request = nil)
-      build_builds(user, 'success', trigger_request)
-      save
+      build_builds(user, trigger_request) && save
     end
 
     def create_next_builds(build)
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index 0d769ed7324..458013ad9f2 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -258,6 +258,16 @@ describe Ci::Pipeline, models: true do
         end
       end
     end
+
+    context 'when no builds created' do
+      before do
+        stub_ci_pipeline_yaml_file(YAML.dump(before_script: ['ls']))
+      end
+
+      it 'returns false' do
+        expect(pipeline.create_builds(nil)).to be_falsey
+      end
+    end
   end
 
   describe "#finished_at" do

From a76cbe5292d20cd6fdac4e519b65df1ee3544371 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Wed, 15 Jun 2016 10:05:36 +0200
Subject: [PATCH 14/17] Add note for short circuit eval when building builds

---
 app/models/ci/pipeline.rb | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index e90924af312..fde03f21f9b 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -93,6 +93,11 @@ module Ci
 
     def build_builds(user, trigger_request = nil, status = 'success')
       return unless config_processor
+
+      ##
+      # Note that `Array#any?` implements a short circuit evaluation, so we
+      # build builds only for the first stage that has builds available.
+      #
       config_processor.stages.any? do |stage|
         build_builds_for_stage(stage, user, status, trigger_request).present?
       end
@@ -117,9 +122,14 @@ module Ci
       prior_builds = latest_builds.where.not(stage: next_stages)
       prior_status = prior_builds.status
 
-      # create builds for next stages based
+      ##
+      # Create builds for next stages based.
+      #
+      # Note that there is a short circult evaluation here.
+      #
       have_builds = next_stages.any? do |stage|
-        build_builds_for_stage(stage, build.user, prior_status, build.trigger_request).present?
+        build_builds_for_stage(stage, build.user, prior_status,
+                               build.trigger_request).present?
       end
 
       save! if have_builds

From 6ff146340fea6d0df1b711933b0399fbf324e861 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Wed, 15 Jun 2016 10:22:20 +0200
Subject: [PATCH 15/17] Improve creating builds by combining two loops

---
 app/models/ci/pipeline.rb | 36 ++++++++++++++++--------------------
 1 file changed, 16 insertions(+), 20 deletions(-)

diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index fde03f21f9b..58c69251824 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -91,16 +91,11 @@ module Ci
       trigger_requests.any?
     end
 
-    def build_builds(user, trigger_request = nil, status = 'success')
+    def build_builds(user, trigger_request = nil)
       return unless config_processor
 
-      ##
-      # Note that `Array#any?` implements a short circuit evaluation, so we
-      # build builds only for the first stage that has builds available.
-      #
-      config_processor.stages.any? do |stage|
-        build_builds_for_stage(stage, user, status, trigger_request).present?
-      end
+      build_builds_for_stages(config_processor.stages, user,
+                              'success', trigger_request)
     end
 
     def create_builds(user, trigger_request = nil)
@@ -122,17 +117,11 @@ module Ci
       prior_builds = latest_builds.where.not(stage: next_stages)
       prior_status = prior_builds.status
 
-      ##
-      # Create builds for next stages based.
-      #
-      # Note that there is a short circult evaluation here.
-      #
-      have_builds = next_stages.any? do |stage|
-        build_builds_for_stage(stage, build.user, prior_status,
-                               build.trigger_request).present?
-      end
+      # build builds for next stage that has builds available
+      # and save pipeline if we have builds
+      build_builds_for_stages(next_stages, build.user, prior_status,
+                              build.trigger_request) && save
 
-      save! if have_builds
     end
 
     def retried
@@ -179,8 +168,15 @@ module Ci
 
     private
 
-    def build_builds_for_stage(stage, user, status, trigger_request)
-      CreateBuildsService.new(self).execute(stage, user, status, trigger_request)
+    def build_builds_for_stages(stages, user, status, trigger_request)
+      ##
+      # Note that `Array#any?` implements a short circuit evaluation, so we
+      # build builds only for the first stage that has builds available.
+      #
+      stages.any? do |stage|
+        CreateBuildsService.new(self)
+          .execute(stage, user, status, trigger_request).present?
+      end
     end
 
     def update_state

From 69112072ca915e8d051f39bb8642f1c4fee4b692 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Wed, 15 Jun 2016 10:45:08 +0200
Subject: [PATCH 16/17] Add Changelog entry for pipeline status fix

---
 CHANGELOG | 1 +
 1 file changed, 1 insertion(+)

diff --git a/CHANGELOG b/CHANGELOG
index 2aed8eb322b..4defd85ef10 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,6 +1,7 @@
 Please view this file on the master branch, on stable branches it's out of date.
 
 v 8.9.0 (unreleased)
+  - Fix pipeline status when there are no builds in pipeline
   - Fix Error 500 when using closes_issues API with an external issue tracker
   - Add more information into RSS feed for issues (Alexander Matyushentsev)
   - Bulk assign/unassign labels to issues.

From 2d495fce529cc3ac15f7096ddf9962db0fbd1e23 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Wed, 15 Jun 2016 14:03:43 +0200
Subject: [PATCH 17/17] Remove reduntant method for building pipeline builds

---
 app/models/ci/pipeline.rb                          | 12 +++++-------
 app/services/ci/create_builds_service.rb           |  3 ++-
 app/services/create_commit_builds_service.rb       |  2 +-
 spec/models/ci/pipeline_spec.rb                    |  3 +++
 spec/services/create_commit_builds_service_spec.rb |  1 +
 5 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index 58c69251824..a26cb7dd7ee 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -91,15 +91,14 @@ module Ci
       trigger_requests.any?
     end
 
-    def build_builds(user, trigger_request = nil)
+    def create_builds(user, trigger_request = nil)
+      ##
+      # We persist pipeline only if there are builds available
+      #
       return unless config_processor
 
       build_builds_for_stages(config_processor.stages, user,
-                              'success', trigger_request)
-    end
-
-    def create_builds(user, trigger_request = nil)
-      build_builds(user, trigger_request) && save
+                              'success', trigger_request) && save
     end
 
     def create_next_builds(build)
@@ -121,7 +120,6 @@ module Ci
       # and save pipeline if we have builds
       build_builds_for_stages(next_stages, build.user, prior_status,
                               build.trigger_request) && save
-
     end
 
     def retried
diff --git a/app/services/ci/create_builds_service.rb b/app/services/ci/create_builds_service.rb
index f7f73aff989..b2882b23d31 100644
--- a/app/services/ci/create_builds_service.rb
+++ b/app/services/ci/create_builds_service.rb
@@ -22,7 +22,8 @@ module Ci
 
       # don't create the same build twice
       builds_attrs.reject! do |build_attrs|
-        @pipeline.builds.find_by(ref: @pipeline.ref, tag: @pipeline.tag,
+        @pipeline.builds.find_by(ref: @pipeline.ref,
+                                 tag: @pipeline.tag,
                                  trigger_request: trigger_request,
                                  name: build_attrs[:name])
       end
diff --git a/app/services/create_commit_builds_service.rb b/app/services/create_commit_builds_service.rb
index 668d0a86549..f947e8f452e 100644
--- a/app/services/create_commit_builds_service.rb
+++ b/app/services/create_commit_builds_service.rb
@@ -42,7 +42,7 @@ class CreateCommitBuildsService
     ##
     # Skip creating pipeline object if there are no builds for it.
     #
-    unless @pipeline.build_builds(user)
+    unless @pipeline.create_builds(user)
       @pipeline.errors.add(:base, 'No builds created')
       return false
     end
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index 458013ad9f2..34507cf5083 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -260,12 +260,15 @@ describe Ci::Pipeline, models: true do
     end
 
     context 'when no builds created' do
+      let(:pipeline) { build(:ci_pipeline) }
+
       before do
         stub_ci_pipeline_yaml_file(YAML.dump(before_script: ['ls']))
       end
 
       it 'returns false' do
         expect(pipeline.create_builds(nil)).to be_falsey
+        expect(pipeline).not_to be_persisted
       end
     end
   end
diff --git a/spec/services/create_commit_builds_service_spec.rb b/spec/services/create_commit_builds_service_spec.rb
index 50ce9659c10..deab242f45a 100644
--- a/spec/services/create_commit_builds_service_spec.rb
+++ b/spec/services/create_commit_builds_service_spec.rb
@@ -184,6 +184,7 @@ describe CreateCommitBuildsService, services: true do
                                  before: '00000000',
                                  after: '31das312',
                                  commits: [{ message: 'some msg' }])
+
         expect(result).to be_falsey
         expect(Ci::Build.all).to be_empty
         expect(Ci::Pipeline.count).to eq(0)