From 683d8b7f007ad750d088b074e50339c66bf5dd82 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Mon, 12 Sep 2016 18:55:49 +0800 Subject: Fix the ordering of transition callbacks: Because pipeline status could be changed for the builds in the next stages, if we process next stages first, the current build would be out of synchronized, and would need a reload for that matter. Alternatively, like what I did in this commit, we could process the next stages later (by using `after_transition` rather than `around_transition`), and complete what're doing for the current build first. This way we don't have to reload because nothing is out synchronized. Note that since giving `false` in `after_transition` would halt the callbacks chain, according to: https://github.com/state-machines/state_machines-activemodel/blob/v0.4.0/lib/state_machines/integrations/active_model.rb#L426-L429 We'll need to make sure we're not returning false because we don't intend to interrupt the chain. This fixes #22010. After this fix, both pipeline events and build events would only show up once. --- app/models/commit_status.rb | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index 4a628924499..438f807b118 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -69,17 +69,15 @@ class CommitStatus < ActiveRecord::Base commit_status.update_attributes finished_at: Time.now end - # 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 - - commit_status.pipeline.try(:process!) - end - after_transition do |commit_status, transition| commit_status.pipeline.try(:build_updated) unless transition.loopback? end + after_transition any => [:success, :failed, :canceled] do |commit_status| + commit_status.pipeline.try(:process!) + true + end + after_transition [:created, :pending, :running] => :success do |commit_status| MergeRequests::MergeWhenBuildSucceedsService.new(commit_status.pipeline.project, nil).trigger(commit_status) end -- cgit v1.2.1 From 47dccfb32935426a4f1cf386466961f61225208a Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Mon, 12 Sep 2016 20:22:59 +0800 Subject: Fix test (credits to Kamil) --- app/models/concerns/has_status.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/app/models/concerns/has_status.rb b/app/models/concerns/has_status.rb index f7b8352405c..77fc66a7105 100644 --- a/app/models/concerns/has_status.rb +++ b/app/models/concerns/has_status.rb @@ -8,8 +8,9 @@ module HasStatus class_methods do def status_sql - scope = all.relevant + scope = all builds = scope.select('count(*)').to_sql + created = scope.created.select('count(*)').to_sql success = scope.success.select('count(*)').to_sql ignored = scope.ignored.select('count(*)').to_sql if scope.respond_to?(:ignored) ignored ||= '0' @@ -22,9 +23,9 @@ module HasStatus WHEN (#{builds})=0 THEN NULL WHEN (#{builds})=(#{skipped}) THEN 'skipped' WHEN (#{builds})=(#{success})+(#{ignored})+(#{skipped}) THEN 'success' - WHEN (#{builds})=(#{pending})+(#{skipped}) THEN 'pending' + WHEN (#{builds})=(#{created})+(#{pending})+(#{skipped}) THEN 'pending' WHEN (#{builds})=(#{canceled})+(#{success})+(#{ignored})+(#{skipped}) THEN 'canceled' - WHEN (#{running})+(#{pending})>0 THEN 'running' + WHEN (#{running})+(#{pending})+(#{created})>0 THEN 'running' ELSE 'failed' END)" -- cgit v1.2.1 From 575dc2b0d78b2680d6e5bb43f293c98d378b8733 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Mon, 12 Sep 2016 20:37:44 +0800 Subject: reload instead, so that we don't have to change order --- app/models/commit_status.rb | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index 438f807b118..ad4e1c25231 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -69,15 +69,22 @@ class CommitStatus < ActiveRecord::Base commit_status.update_attributes finished_at: Time.now end - after_transition do |commit_status, transition| - commit_status.pipeline.try(:build_updated) unless transition.loopback? - end + # 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 - after_transition any => [:success, :failed, :canceled] do |commit_status| commit_status.pipeline.try(:process!) true end + after_transition do |commit_status, transition| + pipeline = commit_status.pipeline + if !transition.loopback? && pipeline + pipeline.reload + pipeline.build_updated + end + end + after_transition [:created, :pending, :running] => :success do |commit_status| MergeRequests::MergeWhenBuildSucceedsService.new(commit_status.pipeline.project, nil).trigger(commit_status) end -- cgit v1.2.1 From 6baf9971db231a2c9923bd4d73e1f59fe255aab8 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Mon, 12 Sep 2016 21:27:46 +0800 Subject: Revert "reload instead, so that we don't have to change order" This reverts commit 575dc2b0d78b2680d6e5bb43f293c98d378b8733. --- app/models/commit_status.rb | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index ad4e1c25231..438f807b118 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -69,22 +69,15 @@ class CommitStatus < ActiveRecord::Base commit_status.update_attributes finished_at: Time.now end - # 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 + after_transition do |commit_status, transition| + commit_status.pipeline.try(:build_updated) unless transition.loopback? + end + after_transition any => [:success, :failed, :canceled] do |commit_status| commit_status.pipeline.try(:process!) true end - after_transition do |commit_status, transition| - pipeline = commit_status.pipeline - if !transition.loopback? && pipeline - pipeline.reload - pipeline.build_updated - end - end - after_transition [:created, :pending, :running] => :success do |commit_status| MergeRequests::MergeWhenBuildSucceedsService.new(commit_status.pipeline.project, nil).trigger(commit_status) end -- cgit v1.2.1 From af7d383675269523ca4c6b7bddf8a34f6054f6af Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Mon, 12 Sep 2016 21:27:59 +0800 Subject: should show the status of the latest one --- app/models/commit.rb | 2 +- spec/requests/api/commits_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/commit.rb b/app/models/commit.rb index e64fd1e0c1b..0e2ab76d347 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -231,7 +231,7 @@ class Commit def status return @status if defined?(@status) - @status ||= pipelines.status + @status ||= pipelines.order(:id).last.try(:status) end def revert_branch_name diff --git a/spec/requests/api/commits_spec.rb b/spec/requests/api/commits_spec.rb index 5b3dc60aba2..e24e92e063c 100644 --- a/spec/requests/api/commits_spec.rb +++ b/spec/requests/api/commits_spec.rb @@ -110,7 +110,7 @@ describe API::API, api: true do get api("/projects/#{project.id}/repository/commits/#{project.repository.commit.id}", user) expect(response).to have_http_status(200) - expect(json_response['status']).to be_nil + expect(json_response['status']).to eq('created') end end -- cgit v1.2.1 From ddcb8c880cbeb27a10e36fe1acb1e46963c3a377 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 13 Sep 2016 15:53:10 +0800 Subject: Fix styling --- spec/models/commit_status_spec.rb | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/spec/models/commit_status_spec.rb b/spec/models/commit_status_spec.rb index fcfa3138ce5..6355f636bb9 100644 --- a/spec/models/commit_status_spec.rb +++ b/spec/models/commit_status_spec.rb @@ -40,7 +40,7 @@ describe CommitStatus, models: true do it { is_expected.to be_falsey } end - %w(running success failed).each do |status| + %w[running success failed].each do |status| context "if commit status is #{status}" do before { commit_status.status = status } @@ -48,7 +48,7 @@ describe CommitStatus, models: true do end end - %w(pending canceled).each do |status| + %w[pending canceled].each do |status| context "if commit status is #{status}" do before { commit_status.status = status } @@ -60,7 +60,7 @@ describe CommitStatus, models: true do describe '#active?' do subject { commit_status.active? } - %w(pending running).each do |state| + %w[pending running].each do |state| context "if commit_status.status is #{state}" do before { commit_status.status = state } @@ -68,7 +68,7 @@ describe CommitStatus, models: true do end end - %w(success failed canceled).each do |state| + %w[success failed canceled].each do |state| context "if commit_status.status is #{state}" do before { commit_status.status = state } @@ -80,7 +80,7 @@ describe CommitStatus, models: true do describe '#complete?' do subject { commit_status.complete? } - %w(success failed canceled).each do |state| + %w[success failed canceled].each do |state| context "if commit_status.status is #{state}" do before { commit_status.status = state } @@ -88,7 +88,7 @@ describe CommitStatus, models: true do end end - %w(pending running).each do |state| + %w[pending running].each do |state| context "if commit_status.status is #{state}" do before { commit_status.status = state } @@ -187,7 +187,7 @@ describe CommitStatus, models: true do subject { CommitStatus.where(pipeline: pipeline).stages } it 'returns ordered list of stages' do - is_expected.to eq(%w(build test deploy)) + is_expected.to eq(%w[build test deploy]) end end -- cgit v1.2.1 From 18d7ae43099040d21dddbb114761c34c833ec766 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 13 Sep 2016 16:14:20 +0800 Subject: Add a test for #22010 The observed faulty state transition is probably hard to test, because we need to hook into internal states to observe them. Namely this: 07:30:16 | Build#ruby-2.2 enqueue: created -> pending 07:30:16 | Pipeline#32 enqueue: created -> pending 07:30:16 | Build#ruby-2.3 enqueue: created -> pending 07:30:16 | Build#ruby-2.2 run: pending -> running 07:30:16 | Pipeline#32 run: pending -> running 07:30:29 | Build#ruby-2.2 drop: running -> failed 07:30:29 | Pipeline#32 run: running -> running 07:30:29 | Build#ruby-2.3 run: pending -> running 07:30:30 | Pipeline#32 run: running -> running 07:30:57 | Build#gem:build skip: created -> skipped 07:30:57 | Pipeline#32 drop: running -> failed 07:30:57 | Build#gem:release skip: created -> skipped 07:30:57 | Pipeline#32 drop: failed -> failed 07:30:57 | Build#ruby-2.3 drop: running -> failed 07:30:57 | Pipeline#32 drop: running -> failed ^^^ Should be failed -> failed However, the consequence of this, executing hooks twice would be easy enough to observe. So we could at least test against this. Keep in mind that if we ever changed how we execute the hooks this won't be testing against faulty state transition. --- spec/models/ci/pipeline_spec.rb | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index fbf945c757c..5d9063c0bc5 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -373,8 +373,8 @@ describe Ci::Pipeline, models: true do end describe '#execute_hooks' do - let!(:build_a) { create_build('a') } - let!(:build_b) { create_build('b') } + let!(:build_a) { create_build('a', 0) } + let!(:build_b) { create_build('b', 1) } let!(:hook) do create(:project_hook, project: project, pipeline_events: enabled) @@ -427,6 +427,16 @@ describe Ci::Pipeline, models: true do end end + context 'when stage one failed' do + before do + build_a.drop + end + + it 'receive a failed event once' do + expect(WebMock).to have_requested_pipeline_hook('failed').once + end + end + def have_requested_pipeline_hook(status) have_requested(:post, hook.url).with do |req| json_body = JSON.parse(req.body) @@ -450,8 +460,12 @@ describe Ci::Pipeline, models: true do end end - def create_build(name) - create(:ci_build, :created, pipeline: pipeline, name: name) + def create_build(name, stage_idx) + create(:ci_build, + :created, + pipeline: pipeline, + name: name, + stage_idx: stage_idx) end end end -- cgit v1.2.1 From 05faeec708824bf97c3114b23235859663631b27 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 13 Sep 2016 16:22:02 +0800 Subject: Fix English --- spec/models/ci/pipeline_spec.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 5d9063c0bc5..f1857f846dc 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -398,7 +398,7 @@ describe Ci::Pipeline, models: true do build_b.enqueue end - it 'receive a pending event once' do + it 'receives a pending event once' do expect(WebMock).to have_requested_pipeline_hook('pending').once end end @@ -411,7 +411,7 @@ describe Ci::Pipeline, models: true do build_b.run end - it 'receive a running event once' do + it 'receives a running event once' do expect(WebMock).to have_requested_pipeline_hook('running').once end end @@ -422,7 +422,7 @@ describe Ci::Pipeline, models: true do build_b.success end - it 'receive a success event once' do + it 'receives a success event once' do expect(WebMock).to have_requested_pipeline_hook('success').once end end @@ -432,7 +432,7 @@ describe Ci::Pipeline, models: true do build_a.drop end - it 'receive a failed event once' do + it 'receives a failed event once' do expect(WebMock).to have_requested_pipeline_hook('failed').once end end -- cgit v1.2.1 From 50e62b3eb8ab030e123e990d1335f68478cc4a4b Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 14 Sep 2016 01:25:26 +0800 Subject: Fix Commit#status, feedback: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6305#note_15230024 --- app/models/commit.rb | 2 +- app/models/concerns/has_status.rb | 2 +- spec/requests/api/commits_spec.rb | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/commit.rb b/app/models/commit.rb index 0e2ab76d347..e64fd1e0c1b 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -231,7 +231,7 @@ class Commit def status return @status if defined?(@status) - @status ||= pipelines.order(:id).last.try(:status) + @status ||= pipelines.status end def revert_branch_name diff --git a/app/models/concerns/has_status.rb b/app/models/concerns/has_status.rb index 77fc66a7105..d658552f695 100644 --- a/app/models/concerns/has_status.rb +++ b/app/models/concerns/has_status.rb @@ -20,7 +20,7 @@ module HasStatus skipped = scope.skipped.select('count(*)').to_sql deduce_status = "(CASE - WHEN (#{builds})=0 THEN NULL + WHEN (#{builds})=(#{created}) THEN NULL WHEN (#{builds})=(#{skipped}) THEN 'skipped' WHEN (#{builds})=(#{success})+(#{ignored})+(#{skipped}) THEN 'success' WHEN (#{builds})=(#{created})+(#{pending})+(#{skipped}) THEN 'pending' diff --git a/spec/requests/api/commits_spec.rb b/spec/requests/api/commits_spec.rb index e24e92e063c..5b3dc60aba2 100644 --- a/spec/requests/api/commits_spec.rb +++ b/spec/requests/api/commits_spec.rb @@ -110,7 +110,7 @@ describe API::API, api: true do get api("/projects/#{project.id}/repository/commits/#{project.repository.commit.id}", user) expect(response).to have_http_status(200) - expect(json_response['status']).to eq('created') + expect(json_response['status']).to be_nil end end -- cgit v1.2.1