Merge branch 'refactor-builds-creation-service' into pipeline-hooks-without-touchpipeline-hooks-without-touch

14 files changed, 72 insertions, 29 deletions

diff --git a/app/controllers/projects/pipelines_controller.rb b/app/controllers/projects/pipelines_controller.rb
index a433634dd58..b0c72cfe4b4 100644
--- a/app/controllers/projects/pipelines_controller.rb
+++ b/app/controllers/projects/pipelines_controller.rb
@@ -19,7 +19,7 @@ class Projects::PipelinesController < Projects::ApplicationController
   end
 
   def create
-    @pipeline = Ci::CreatePipelineService.new(project, current_user, create_params).execute(skip_ci: false, save_on_errors: false)
+    @pipeline = Ci::CreatePipelineService.new(project, current_user, create_params).execute(ignore_skip_ci: true, save_on_errors: false)
     unless @pipeline.persisted?
       render 'new'
       return
diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 48a49ef203e..05b11f3b115 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -70,14 +70,6 @@ module Ci
         build.execute_hooks
       end
 
-      # 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.pipeline
-          build.pipeline.process!
-        end
-      end
-
       after_transition any => [:success, :failed, :canceled] do |build|
         build.update_coverage
         build.execute_hooks
diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index 5c85578bdf3..c1417903590 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -90,12 +90,14 @@ module Ci
 
     def cancel_running
       builds.running_or_pending.each(&:cancel)
+      reload_status!
     end
 
     def retry_failed(user)
       builds.latest.failed.select(&:retryable?).each do |build|
         Ci::Build.retry(build, user)
       end
+      reload_status!
     end
 
     def latest?
@@ -182,7 +184,6 @@ module Ci
 
     def process!
       Ci::ProcessPipelineService.new(project, user).execute(self)
-      reload_status!
     end
 
     def predefined_variables
diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb
index 79e038b6a78..9c730904ba3 100644
--- a/app/models/commit_status.rb
+++ b/app/models/commit_status.rb
@@ -70,9 +70,12 @@ class CommitStatus < ActiveRecord::Base
       MergeRequests::AddTodoWhenBuildFailsService.new(commit_status.pipeline.project, nil).execute(commit_status)
     end
 
-    after_transition any => [:success, :failed, :canceled] do |commit_status|
+    # We use around_transition to process pipeline on next stages as soon as possible, before the `after_*` is executed
+    around_transition any => [:success, :failed, :canceled] do |commit_status, block|
+      block.call
       if commit_status.pipeline
         commit_status.pipeline.process!
+        commit_status.pipeline.reload_status!
       end
     end
   end
diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index 1acc411af49..25160cb72c4 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -2,7 +2,7 @@ module Ci
   class CreatePipelineService < BaseService
     attr_reader :pipeline
 
-    def execute(skip_ci: true, save_on_errors: true, trigger_request: nil)
+    def execute(ignore_skip_ci: false, save_on_errors: true, trigger_request: nil)
       @pipeline = Ci::Pipeline.new(
         project: project,
         ref: ref,
@@ -36,7 +36,7 @@ module Ci
         return error(pipeline.yaml_errors, save: save_on_errors)
       end
 
-      if skip_ci && skip_ci?
+      if !ignore_skip_ci && skip_ci?
         return error('Creation of pipeline is skipped', save: save_on_errors)
       end
 
@@ -46,6 +46,7 @@ module Ci
 
       pipeline.save
       pipeline.process!
+      pipeline.reload_status!
       pipeline
     end
 
diff --git a/app/services/ci/create_trigger_request_service.rb b/app/services/ci/create_trigger_request_service.rb
index c6be8f41ca4..6af3c1ca5b1 100644
--- a/app/services/ci/create_trigger_request_service.rb
+++ b/app/services/ci/create_trigger_request_service.rb
@@ -4,7 +4,7 @@ module Ci
       trigger_request = trigger.trigger_requests.create(variables: variables)
 
       pipeline = Ci::CreatePipelineService.new(project, nil, ref: ref).
-        execute(skip_ci: false, trigger_request: trigger_request)
+        execute(ignore_skip_ci: true, trigger_request: trigger_request)
       if pipeline.persisted?
         trigger_request
       end
diff --git a/features/steps/shared/builds.rb b/features/steps/shared/builds.rb
index 4d6b258f577..c7f61da05fa 100644
--- a/features/steps/shared/builds.rb
+++ b/features/steps/shared/builds.rb
@@ -10,20 +10,22 @@ module SharedBuilds
   end
 
   step 'project has a recent build' do
-    @pipeline = create(:ci_pipeline, project: @project, sha: @project.commit.sha, ref: 'master')
+    @pipeline = create(:ci_empty_pipeline, project: @project, sha: @project.commit.sha, ref: 'master')
     @build = create(:ci_build_with_coverage, pipeline: @pipeline)
+    @pipeline.reload_status!
   end
 
   step 'recent build is successful' do
-    @build.update(status: 'success')
+    @build.success
   end
 
   step 'recent build failed' do
-    @build.update(status: 'failed')
+    @build.drop
   end
 
   step 'project has another build that is running' do
     create(:ci_build, pipeline: @pipeline, name: 'second build', status: 'running')
+    @pipeline.reload_status!
   end
 
   step 'I visit recent build details page' do
diff --git a/spec/factories/commit_statuses.rb b/spec/factories/commit_statuses.rb
index 1e5c479616c..995f2080f10 100644
--- a/spec/factories/commit_statuses.rb
+++ b/spec/factories/commit_statuses.rb
@@ -7,6 +7,30 @@ FactoryGirl.define do
     started_at 'Tue, 26 Jan 2016 08:21:42 +0100'
     finished_at 'Tue, 26 Jan 2016 08:23:42 +0100'
 
+    trait :success do
+      status 'success'
+    end
+
+    trait :failed do
+      status 'failed'
+    end
+
+    trait :canceled do
+      status 'canceled'
+    end
+
+    trait :running do
+      status 'running'
+    end
+
+    trait :pending do
+      status 'pending'
+    end
+
+    trait :created do
+      status 'created'
+    end
+
     after(:build) do |build, evaluator|
       build.project = build.pipeline.project
     end
diff --git a/spec/features/pipelines_spec.rb b/spec/features/pipelines_spec.rb
index 1256f73196b..f88b8f8e60b 100644
--- a/spec/features/pipelines_spec.rb
+++ b/spec/features/pipelines_spec.rb
@@ -33,7 +33,10 @@ describe "Pipelines" do
     context 'cancelable pipeline' do
       let!(:running) { create(:ci_build, :running, pipeline: pipeline, stage: 'test', commands: 'test') }
 
-      before { visit namespace_project_pipelines_path(project.namespace, project) }
+      before do
+        pipeline.reload_status!
+        visit namespace_project_pipelines_path(project.namespace, project)
+      end
 
       it { expect(page).to have_link('Cancel') }
       it { expect(page).to have_selector('.ci-running') }
@@ -49,7 +52,10 @@ describe "Pipelines" do
     context 'retryable pipelines' do
       let!(:failed) { create(:ci_build, :failed, pipeline: pipeline, stage: 'test', commands: 'test') }
 
-      before { visit namespace_project_pipelines_path(project.namespace, project) }
+      before do
+        pipeline.reload_status!
+        visit namespace_project_pipelines_path(project.namespace, project)
+      end
 
       it { expect(page).to have_link('Retry') }
       it { expect(page).to have_selector('.ci-failed') }
@@ -80,7 +86,10 @@ describe "Pipelines" do
       context 'when running' do
         let!(:running) { create(:generic_commit_status, status: 'running', pipeline: pipeline, stage: 'test') }
 
-        before { visit namespace_project_pipelines_path(project.namespace, project) }
+        before do
+          pipeline.reload_status!
+          visit namespace_project_pipelines_path(project.namespace, project)
+        end
 
         it 'is not cancelable' do
           expect(page).not_to have_link('Cancel')
@@ -92,9 +101,12 @@ describe "Pipelines" do
       end
 
       context 'when failed' do
-        let!(:running) { create(:generic_commit_status, status: 'failed', pipeline: pipeline, stage: 'test') }
+        let!(:failed) { create(:generic_commit_status, status: 'failed', pipeline: pipeline, stage: 'test') }
 
-        before { visit namespace_project_pipelines_path(project.namespace, project) }
+        before do
+          pipeline.reload_status!
+          visit namespace_project_pipelines_path(project.namespace, project)
+        end
 
         it 'is not retryable' do
           expect(page).not_to have_link('Retry')
diff --git a/spec/lib/ci/charts_spec.rb b/spec/lib/ci/charts_spec.rb
index 034ea098193..2cd6b00dad6 100644
--- a/spec/lib/ci/charts_spec.rb
+++ b/spec/lib/ci/charts_spec.rb
@@ -5,6 +5,7 @@ describe Ci::Charts, lib: true do
     before do
       @pipeline = FactoryGirl.create(:ci_pipeline)
       FactoryGirl.create(:ci_build, pipeline: @pipeline)
+      @pipeline.reload_status!
     end
 
     it 'returns build times in minutes' do
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index 79141a9d94e..0a57734ca6e 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -143,7 +143,7 @@ describe Ci::Pipeline, models: true do
     let(:pipeline) { create :ci_empty_pipeline, project: project }
 
     context 'dependent objects' do
-      let(:commit_status) { create :commit_status, pipeline: pipeline }
+      let(:commit_status) { create :commit_status, :pending, pipeline: pipeline }
 
       it 'executes reload_status! after succeeding dependent object' do
         expect(pipeline).to receive(:reload_status!).and_return(true)
@@ -151,16 +151,17 @@ describe Ci::Pipeline, models: true do
       end
     end
 
-    context 'reload state' do
+    context 'updates' do
       let(:current) { Time.now.change(usec: 0) }
       let(:build) { FactoryGirl.create :ci_build, pipeline: pipeline, started_at: current - 120, finished_at: current - 60 }
 
       before do
-        build.success
+        build
+        pipeline.reload_status!
       end
 
       [:status, :started_at, :finished_at, :duration].each do |param|
-        it "update #{param}" do
+        it "#{param}" do
           expect(pipeline.send(param)).to eq(build.send(param))
         end
       end
diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb
index 966d302dfd3..a4cdd8f3140 100644
--- a/spec/requests/api/builds_spec.rb
+++ b/spec/requests/api/builds_spec.rb
@@ -238,6 +238,10 @@ describe API::API, api: true do
         it { expect(response.headers).to include(download_headers) }
       end
 
+      before do
+        pipeline.reload_status!
+      end
+
       context 'with regular branch' do
         before do
           pipeline.update(ref: 'master',
diff --git a/spec/services/ci/image_for_build_service_spec.rb b/spec/services/ci/image_for_build_service_spec.rb
index 3a3e3efe709..259062406c7 100644
--- a/spec/services/ci/image_for_build_service_spec.rb
+++ b/spec/services/ci/image_for_build_service_spec.rb
@@ -5,8 +5,8 @@ module Ci
     let(:service) { ImageForBuildService.new }
     let(:project) { FactoryGirl.create(:empty_project) }
     let(:commit_sha) { '01234567890123456789' }
-    let(:commit) { project.ensure_pipeline(commit_sha, 'master') }
-    let(:build) { FactoryGirl.create(:ci_build, pipeline: commit) }
+    let(:pipeline) { project.ensure_pipeline(commit_sha, 'master') }
+    let(:build) { FactoryGirl.create(:ci_build, pipeline: pipeline) }
 
     describe '#execute' do
       before { build }
@@ -14,6 +14,7 @@ module Ci
       context 'branch name' do
         before { allow(project).to receive(:commit).and_return(OpenStruct.new(sha: commit_sha)) }
         before { build.run! }
+        before { pipeline.reload_status! }
         let(:image) { service.execute(project, ref: 'master') }
 
         it { expect(image).to be_kind_of(OpenStruct) }
@@ -31,6 +32,7 @@ module Ci
 
       context 'commit sha' do
         before { build.run! }
+        before { pipeline.reload_status! }
         let(:image) { service.execute(project, sha: build.sha) }
 
         it { expect(image).to be_kind_of(OpenStruct) }
diff --git a/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb b/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb
index e094a6077a1..520e906b21f 100644
--- a/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb
+++ b/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb
@@ -110,9 +110,9 @@ describe MergeRequests::MergeWhenBuildSucceedsService do
 
     context 'properly handles multiple stages' do
       let(:ref) { mr_merge_if_green_enabled.source_branch }
-      let(:pipeline) { create(:ci_pipeline_without_jobs, ref: mr_merge_if_green_enabled.source_branch, project: project) }
       let!(:build) { create(:ci_build, :created, pipeline: pipeline, ref: ref, name: 'build', stage: 'build') }
       let!(:test) { create(:ci_build, :created, pipeline: pipeline, ref: ref, name: 'test', stage: 'test') }
+      let(:pipeline) { create(:ci_empty_pipeline, ref: mr_merge_if_green_enabled.source_branch, project: project) }
 
       before do
         # This behavior of MergeRequest: we instantiate a new object