From 6a6a69f4afbe0107a75df018b662f02b5ec0166a Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Thu, 11 Aug 2016 20:54:02 +0200 Subject: Use state machine for pipeline event processing --- app/models/ci/pipeline.rb | 61 +++++++++++++++++------- app/models/commit_status.rb | 8 ++-- app/services/ci/create_pipeline_service.rb | 5 +- features/steps/shared/builds.rb | 2 - spec/features/pipelines_spec.rb | 4 -- spec/lib/ci/charts_spec.rb | 1 - spec/models/ci/pipeline_spec.rb | 38 ++------------- spec/requests/api/builds_spec.rb | 4 -- spec/services/ci/image_for_build_service_spec.rb | 2 - 9 files changed, 54 insertions(+), 71 deletions(-) diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 8de799d6088..7a0430f277a 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -19,6 +19,37 @@ module Ci after_save :keep_around_commits + state_machine :status, initial: :created do + event :skip do + transition any => :skipped + end + + event :drop do + transition any => :failed + end + + event :update_status do + transition any => :pending, if: ->(pipeline) { pipeline.can_transition_to?('pending') } + transition any => :running, if: ->(pipeline) { pipeline.can_transition_to?('running') } + transition any => :failed, if: ->(pipeline) { pipeline.can_transition_to?('failed') } + transition any => :success, if: ->(pipeline) { pipeline.can_transition_to?('success') } + transition any => :canceled, if: ->(pipeline) { pipeline.can_transition_to?('canceled') } + transition any => :skipped, if: ->(pipeline) { pipeline.can_transition_to?('skipped') } + end + + after_transition [:created, :pending] => :running do |pipeline| + pipeline.update(started_at: Time.now) + end + + after_transition any => [:success, :failed, :canceled] do |pipeline| + pipeline.update(finished_at: Time.now) + end + + after_transition do |pipeline| + pipeline.update_duration + 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 +120,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 +212,6 @@ module Ci def process! Ci::ProcessPipelineService.new(project, user).execute(self) - - reload_status! end def predefined_variables @@ -195,22 +220,22 @@ module Ci ] end - def reload_status! - reload - self.status = - if yaml_errors.blank? - statuses.latest.status || 'skipped' - else - 'failed' - end - self.started_at = statuses.started_at - self.finished_at = statuses.finished_at - self.duration = statuses.latest.duration - save + def can_transition_to?(expected_status) + latest_status == expected_status + end + + def update_duration + update(duration: statuses.latest.duration) end private + def latest_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..64ce5431d63 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -74,13 +74,13 @@ 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 - commit_status.pipeline.reload_status! if commit_status.pipeline + after_transition do |commit_status, transition| + commit_status.pipeline.try(:update_status) unless transition.loopback? 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..eb762276cbe 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' describe Ci::Pipeline, models: true do let(:project) { FactoryGirl.create :empty_project } - let(:pipeline) { FactoryGirl.create :ci_pipeline, project: project } + let(:pipeline) { FactoryGirl.create :ci_empty_pipeline, project: project } it { is_expected.to belong_to(:project) } it { is_expected.to belong_to(:user) } @@ -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) } -- cgit v1.2.1