Commit e28fbd95 authored by Rémy Coutable's avatar Rémy Coutable

Merge branch 'fix-multiple-pipeline-events' into 'master'

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.

See merge request !6305
parents 4768521a 50e62b3e
No related merge requests found
......@@ -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
......
......@@ -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'
......@@ -19,12 +20,12 @@ 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})=(#{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)"
......
......@@ -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)
......@@ -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,11 +422,21 @@ 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
context 'when stage one failed' do
before do
build_a.drop
end
it 'receives 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
......@@ -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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment