Use state machine for pipeline event processingfix-pipeline-processing

Conflicts:
	app/models/ci/pipeline.rb
	spec/models/ci/pipeline_spec.rb

9 files changed, 75 insertions, 66 deletions

diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index 8de799d6088..1c8fe550153 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -19,6 +19,40 @@ module Ci
 
     after_save :keep_around_commits
 
+    state_machine :status, initial: :created do
+      event :start do
+        transition any => :pending, if: ->(pipeline) { pipeline.can_transition_to?('pending') }
+      end
+
+      event :run do
+        transition any => :running, if: ->(pipeline) { pipeline.can_transition_to?('running') }
+      end
+
+      event :drop do
+        transition any => :failed, if: ->(pipeline) { pipeline.can_transition_to?('failed') }
+      end
+
+      event :succeed do
+        transition any => :success, if: ->(pipeline) { pipeline.can_transition_to?('success') }
+      end
+
+      event :cancel do
+        transition any => :canceled, if: ->(pipeline) { pipeline.can_transition_to?('canceled') }
+      end
+
+      event :skip do
+        transition any => :skipped, if: ->(pipeline) { pipeline.can_transition_to?('skipped') }
+      end
+
+      before_transition do |pipeline|
+        pipeline.update_counters
+      end
+
+      after_transition any => any do |pipeline, transition|
+        pipeline.execute_hooks unless transition.loopback?
+      end
+    end
+
     # ref can't be HEAD or SHA, can only be branch/tag name
     scope :latest_successful_for, ->(ref = default_branch) do
       where(ref: ref).success.order(id: :desc).limit(1)
@@ -89,16 +123,12 @@ 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?
@@ -185,8 +215,6 @@ module Ci
 
     def process!
       Ci::ProcessPipelineService.new(project, user).execute(self)
-
-      reload_status!
     end
 
     def predefined_variables
@@ -194,15 +222,12 @@ module Ci
         { key: 'CI_PIPELINE_ID', value: id.to_s, public: true }
       ]
     end
+    
+    def can_transition_to?(expected_status)
+      current_status == expected_status
+    end
 
-    def reload_status!
-      reload
-      self.status =
-        if yaml_errors.blank?
-          statuses.latest.status || 'skipped'
-        else
-          'failed'
-        end
+    def update_counters
       self.started_at = statuses.started_at
       self.finished_at = statuses.finished_at
       self.duration = statuses.latest.duration
@@ -211,6 +236,12 @@ module Ci
 
     private
 
+    def current_status
+      return 'failed' unless yaml_errors.blank?
+
+      statuses.latest.status || 'skipped'
+    end
+
     def keep_around_commits
       return unless project
 
diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb
index 3ab44461179..5773545a602 100644
--- a/app/models/commit_status.rb
+++ b/app/models/commit_status.rb
@@ -74,13 +74,33 @@ class CommitStatus < ActiveRecord::Base
     around_transition any => [:success, :failed, :canceled] do |commit_status, block|
       block.call
 
-      commit_status.pipeline.process! if commit_status.pipeline
+      commit_status.pipeline.try(:process!)
     end
 
-    around_transition any => [:pending, :running] do |commit_status, block|
-      block.call
+    # Try to update the pipeline status
+
+    after_transition any => :pending do |commit_status|
+      commit_status.pipeline.try(:start)
+    end
+
+    after_transition any => :running do |commit_status|
+      commit_status.pipeline.try(:run)
+    end
+
+    after_transition any => :success do |commit_status|
+      commit_status.pipeline.try(:succeed)
+    end
+
+    after_transition any => :failed do |commit_status|
+      commit_status.pipeline.try(:drop)
+    end
+
+    after_transition any => :skipped do |commit_status|
+      commit_status.pipeline.try(:skip)
+    end
 
-      commit_status.pipeline.reload_status! if commit_status.pipeline
+    after_transition any => :canceled do |commit_status|
+      commit_status.pipeline.try(:cancel)
     end
   end
 
diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index 7398fd8e10a..cde856b0186 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -37,7 +37,8 @@ module Ci
       end
 
       if !ignore_skip_ci && skip_ci?
-        return error('Creation of pipeline is skipped', save: save_on_errors)
+        pipeline.skip if save_on_errors
+        return pipeline
       end
 
       unless pipeline.config_builds_attributes.present?
@@ -93,7 +94,7 @@ module Ci
 
     def error(message, save: false)
       pipeline.errors.add(:base, message)
-      pipeline.reload_status! if save
+      pipeline.drop if save
       pipeline
     end
   end
diff --git a/features/steps/shared/builds.rb b/features/steps/shared/builds.rb
index c7f61da05fa..5ed5cdb759f 100644
--- a/features/steps/shared/builds.rb
+++ b/features/steps/shared/builds.rb
@@ -12,7 +12,6 @@ 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)
-    @pipeline.reload_status!
   end
 
   step 'recent build is successful' do
@@ -25,7 +24,6 @@ module SharedBuilds
 
   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/features/pipelines_spec.rb b/spec/features/pipelines_spec.rb
index f88b8f8e60b..248e44a93aa 100644
--- a/spec/features/pipelines_spec.rb
+++ b/spec/features/pipelines_spec.rb
@@ -34,7 +34,6 @@ describe "Pipelines" do
       let!(:running) { create(:ci_build, :running, pipeline: pipeline, stage: 'test', commands: 'test') }
 
       before do
-        pipeline.reload_status!
         visit namespace_project_pipelines_path(project.namespace, project)
       end
 
@@ -53,7 +52,6 @@ describe "Pipelines" do
       let!(:failed) { create(:ci_build, :failed, pipeline: pipeline, stage: 'test', commands: 'test') }
 
       before do
-        pipeline.reload_status!
         visit namespace_project_pipelines_path(project.namespace, project)
       end
 
@@ -87,7 +85,6 @@ describe "Pipelines" do
         let!(:running) { create(:generic_commit_status, status: 'running', pipeline: pipeline, stage: 'test') }
 
         before do
-          pipeline.reload_status!
           visit namespace_project_pipelines_path(project.namespace, project)
         end
 
@@ -104,7 +101,6 @@ describe "Pipelines" do
         let!(:failed) { create(:generic_commit_status, status: 'failed', pipeline: pipeline, stage: 'test') }
 
         before do
-          pipeline.reload_status!
           visit namespace_project_pipelines_path(project.namespace, project)
         end
 
diff --git a/spec/lib/ci/charts_spec.rb b/spec/lib/ci/charts_spec.rb
index 2cd6b00dad6..034ea098193 100644
--- a/spec/lib/ci/charts_spec.rb
+++ b/spec/lib/ci/charts_spec.rb
@@ -5,7 +5,6 @@ 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 9a9720cfbfc..77b17bfc138 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -51,25 +51,6 @@ describe Ci::Pipeline, models: true do
     end
   end
 
-  describe "#finished_at" do
-    let(:pipeline) { FactoryGirl.create :ci_pipeline }
-
-    it "returns finished_at of latest build" do
-      build = FactoryGirl.create :ci_build, pipeline: pipeline, finished_at: Time.now - 60
-      FactoryGirl.create :ci_build, pipeline: pipeline, finished_at: Time.now - 120
-      pipeline.reload_status!
-
-      expect(pipeline.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, pipeline: pipeline
-      pipeline.reload_status!
-
-      expect(pipeline.finished_at).to be_nil
-    end
-  end
-
   describe "coverage" do
     let(:project) { FactoryGirl.create :empty_project, build_coverage_regex: "/.*/" }
     let(:pipeline) { FactoryGirl.create :ci_empty_pipeline, project: project }
@@ -139,31 +120,20 @@ describe Ci::Pipeline, models: true do
     end
   end
 
-  describe '#reload_status!' do
+  describe '#update_counters' do
     let(:pipeline) { create :ci_empty_pipeline, project: project }
 
-    context 'dependent objects' do
-      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)
-
-        commit_status.success
-      end
-    end
-
     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
-        pipeline.reload_status!
+        build.skip
       end
 
       [:status, :started_at, :finished_at, :duration].each do |param|
         it "#{param}" do
-          expect(pipeline.send(param)).to eq(build.send(param))
+          expect(pipeline.reload.send(param)).to eq(build.send(param))
         end
       end
     end
diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb
index a4cdd8f3140..966d302dfd3 100644
--- a/spec/requests/api/builds_spec.rb
+++ b/spec/requests/api/builds_spec.rb
@@ -238,10 +238,6 @@ 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 259062406c7..c931c3e4829 100644
--- a/spec/services/ci/image_for_build_service_spec.rb
+++ b/spec/services/ci/image_for_build_service_spec.rb
@@ -14,7 +14,6 @@ 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) }
@@ -32,7 +31,6 @@ 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) }