Commit d4d2cf73 authored by Shinya Maeda's avatar Shinya Maeda

Fix merge request pipeline exist method

Refactor
parent 9aa81c0a
...@@ -66,7 +66,7 @@ class MergeRequest < ApplicationRecord ...@@ -66,7 +66,7 @@ class MergeRequest < ApplicationRecord
dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
has_many :cached_closes_issues, through: :merge_requests_closing_issues, source: :issue has_many :cached_closes_issues, through: :merge_requests_closing_issues, source: :issue
has_many :merge_request_pipelines, foreign_key: 'merge_request_id', class_name: 'Ci::Pipeline' has_many :pipelines_for_merge_request, foreign_key: 'merge_request_id', class_name: 'Ci::Pipeline'
has_many :suggestions, through: :notes has_many :suggestions, through: :notes
has_many :merge_request_assignees has_many :merge_request_assignees
...@@ -1157,10 +1157,6 @@ class MergeRequest < ApplicationRecord ...@@ -1157,10 +1157,6 @@ class MergeRequest < ApplicationRecord
end end
end end
def merge_request_pipeline_exists?
merge_request_pipelines.exists?(sha: diff_head_sha)
end
def has_test_reports? def has_test_reports?
actual_head_pipeline&.has_reports?(Ci::JobArtifact.test_reports) actual_head_pipeline&.has_reports?(Ci::JobArtifact.test_reports)
end end
...@@ -1379,12 +1375,12 @@ class MergeRequest < ApplicationRecord ...@@ -1379,12 +1375,12 @@ class MergeRequest < ApplicationRecord
source_project.repository.squash_in_progress?(id) source_project.repository.squash_in_progress?(id)
end end
private
def find_actual_head_pipeline def find_actual_head_pipeline
all_pipelines.for_sha_or_source_sha(diff_head_sha).first all_pipelines.for_sha_or_source_sha(diff_head_sha).first
end end
private
def source_project_variables def source_project_variables
Gitlab::Ci::Variables::Collection.new.tap do |variables| Gitlab::Ci::Variables::Collection.new.tap do |variables|
break variables unless source_project break variables unless source_project
......
...@@ -81,7 +81,7 @@ module MergeRequests ...@@ -81,7 +81,7 @@ module MergeRequests
## ##
# UpdateMergeRequestsWorker could be retried by an exception. # UpdateMergeRequestsWorker could be retried by an exception.
# pipelines for merge request should not be recreated in such case. # pipelines for merge request should not be recreated in such case.
return false if merge_request.merge_request_pipeline_exists? return false if merge_request.find_actual_head_pipeline&.triggered_by_merge_request?
return false if merge_request.has_no_commits? return false if merge_request.has_no_commits?
true true
......
---
title: Fix duplicate merge request pipelines created by Sidekiq worker retry
merge_request: 26643
author:
type: fixed
...@@ -119,7 +119,7 @@ FactoryBot.define do ...@@ -119,7 +119,7 @@ FactoryBot.define do
trait :with_legacy_detached_merge_request_pipeline do trait :with_legacy_detached_merge_request_pipeline do
after(:create) do |merge_request| after(:create) do |merge_request|
merge_request.merge_request_pipelines << create(:ci_pipeline, merge_request.pipelines_for_merge_request << create(:ci_pipeline,
source: :merge_request_event, source: :merge_request_event,
merge_request: merge_request, merge_request: merge_request,
project: merge_request.source_project, project: merge_request.source_project,
...@@ -130,7 +130,7 @@ FactoryBot.define do ...@@ -130,7 +130,7 @@ FactoryBot.define do
trait :with_detached_merge_request_pipeline do trait :with_detached_merge_request_pipeline do
after(:create) do |merge_request| after(:create) do |merge_request|
merge_request.merge_request_pipelines << create(:ci_pipeline, merge_request.pipelines_for_merge_request << create(:ci_pipeline,
source: :merge_request_event, source: :merge_request_event,
merge_request: merge_request, merge_request: merge_request,
project: merge_request.source_project, project: merge_request.source_project,
...@@ -147,7 +147,7 @@ FactoryBot.define do ...@@ -147,7 +147,7 @@ FactoryBot.define do
end end
after(:create) do |merge_request, evaluator| after(:create) do |merge_request, evaluator|
merge_request.merge_request_pipelines << create(:ci_pipeline, merge_request.pipelines_for_merge_request << create(:ci_pipeline,
source: :merge_request_event, source: :merge_request_event,
merge_request: merge_request, merge_request: merge_request,
project: merge_request.source_project, project: merge_request.source_project,
......
...@@ -99,7 +99,7 @@ merge_requests: ...@@ -99,7 +99,7 @@ merge_requests:
- timelogs - timelogs
- head_pipeline - head_pipeline
- latest_merge_request_diff - latest_merge_request_diff
- merge_request_pipelines - pipelines_for_merge_request
- merge_request_assignees - merge_request_assignees
- suggestions - suggestions
- assignees - assignees
......
...@@ -2817,7 +2817,7 @@ describe Ci::Build do ...@@ -2817,7 +2817,7 @@ describe Ci::Build do
context 'when ref is merge request' do context 'when ref is merge request' do
let(:merge_request) { create(:merge_request, :with_detached_merge_request_pipeline) } let(:merge_request) { create(:merge_request, :with_detached_merge_request_pipeline) }
let(:pipeline) { merge_request.merge_request_pipelines.first } let(:pipeline) { merge_request.pipelines_for_merge_request.first }
let(:build) { create(:ci_build, ref: merge_request.source_branch, tag: false, pipeline: pipeline, project: project) } let(:build) { create(:ci_build, ref: merge_request.source_branch, tag: false, pipeline: pipeline, project: project) }
context 'when ref is protected' do context 'when ref is protected' do
...@@ -2875,7 +2875,7 @@ describe Ci::Build do ...@@ -2875,7 +2875,7 @@ describe Ci::Build do
context 'when ref is merge request' do context 'when ref is merge request' do
let(:merge_request) { create(:merge_request, :with_detached_merge_request_pipeline) } let(:merge_request) { create(:merge_request, :with_detached_merge_request_pipeline) }
let(:pipeline) { merge_request.merge_request_pipelines.first } let(:pipeline) { merge_request.pipelines_for_merge_request.first }
let(:build) { create(:ci_build, ref: merge_request.source_branch, tag: false, pipeline: pipeline, project: project) } let(:build) { create(:ci_build, ref: merge_request.source_branch, tag: false, pipeline: pipeline, project: project) }
context 'when ref is protected' do context 'when ref is protected' do
......
...@@ -466,7 +466,7 @@ describe Ci::Pipeline, :mailer do ...@@ -466,7 +466,7 @@ describe Ci::Pipeline, :mailer do
target_branch: 'master') target_branch: 'master')
end end
let(:pipeline) { merge_request.merge_request_pipelines.first } let(:pipeline) { merge_request.pipelines_for_merge_request.first }
it 'does not return the pipeline' do it 'does not return the pipeline' do
is_expected.to be_empty is_expected.to be_empty
......
...@@ -19,7 +19,7 @@ describe HasRef do ...@@ -19,7 +19,7 @@ describe HasRef do
context 'when it was triggered by merge request' do context 'when it was triggered by merge request' do
let(:merge_request) { create(:merge_request, :with_detached_merge_request_pipeline) } let(:merge_request) { create(:merge_request, :with_detached_merge_request_pipeline) }
let(:pipeline) { merge_request.merge_request_pipelines.first } let(:pipeline) { merge_request.pipelines_for_merge_request.first }
let(:build) { create(:ci_build, pipeline: pipeline) } let(:build) { create(:ci_build, pipeline: pipeline) }
it 'returns false' do it 'returns false' do
...@@ -68,7 +68,7 @@ describe HasRef do ...@@ -68,7 +68,7 @@ describe HasRef do
context 'when it is triggered by a merge request' do context 'when it is triggered by a merge request' do
let(:merge_request) { create(:merge_request, :with_detached_merge_request_pipeline) } let(:merge_request) { create(:merge_request, :with_detached_merge_request_pipeline) }
let(:pipeline) { merge_request.merge_request_pipelines.first } let(:pipeline) { merge_request.pipelines_for_merge_request.first }
let(:build) { create(:ci_build, tag: false, pipeline: pipeline) } let(:build) { create(:ci_build, tag: false, pipeline: pipeline) }
it 'returns nil' do it 'returns nil' do
......
...@@ -137,7 +137,7 @@ describe PipelineEntity do ...@@ -137,7 +137,7 @@ describe PipelineEntity do
context 'when pipeline is detached merge request pipeline' do context 'when pipeline is detached merge request pipeline' do
let(:merge_request) { create(:merge_request, :with_detached_merge_request_pipeline) } let(:merge_request) { create(:merge_request, :with_detached_merge_request_pipeline) }
let(:project) { merge_request.target_project } let(:project) { merge_request.target_project }
let(:pipeline) { merge_request.merge_request_pipelines.first } let(:pipeline) { merge_request.pipelines_for_merge_request.first }
it 'makes detached flag true' do it 'makes detached flag true' do
expect(subject[:flags][:detached_merge_request_pipeline]).to be_truthy expect(subject[:flags][:detached_merge_request_pipeline]).to be_truthy
...@@ -185,7 +185,7 @@ describe PipelineEntity do ...@@ -185,7 +185,7 @@ describe PipelineEntity do
context 'when pipeline is merge request pipeline' do context 'when pipeline is merge request pipeline' do
let(:merge_request) { create(:merge_request, :with_merge_request_pipeline, merge_sha: 'abc') } let(:merge_request) { create(:merge_request, :with_merge_request_pipeline, merge_sha: 'abc') }
let(:project) { merge_request.target_project } let(:project) { merge_request.target_project }
let(:pipeline) { merge_request.merge_request_pipelines.first } let(:pipeline) { merge_request.pipelines_for_merge_request.first }
it 'makes detached flag false' do it 'makes detached flag false' do
expect(subject[:flags][:detached_merge_request_pipeline]).to be_falsy expect(subject[:flags][:detached_merge_request_pipeline]).to be_falsy
......
...@@ -195,7 +195,7 @@ describe MergeRequests::CreateService do ...@@ -195,7 +195,7 @@ describe MergeRequests::CreateService do
expect(merge_request).to be_persisted expect(merge_request).to be_persisted
merge_request.reload merge_request.reload
expect(merge_request.merge_request_pipelines.count).to eq(1) expect(merge_request.pipelines_for_merge_request.count).to eq(1)
expect(merge_request.actual_head_pipeline).to be_detached_merge_request_pipeline expect(merge_request.actual_head_pipeline).to be_detached_merge_request_pipeline
end end
...@@ -247,7 +247,7 @@ describe MergeRequests::CreateService do ...@@ -247,7 +247,7 @@ describe MergeRequests::CreateService do
expect(merge_request).to be_persisted expect(merge_request).to be_persisted
merge_request.reload merge_request.reload
expect(merge_request.merge_request_pipelines.count).to eq(0) expect(merge_request.pipelines_for_merge_request.count).to eq(0)
end end
end end
...@@ -281,7 +281,7 @@ describe MergeRequests::CreateService do ...@@ -281,7 +281,7 @@ describe MergeRequests::CreateService do
expect(merge_request).to be_persisted expect(merge_request).to be_persisted
merge_request.reload merge_request.reload
expect(merge_request.merge_request_pipelines.count).to eq(0) expect(merge_request.pipelines_for_merge_request.count).to eq(0)
end end
end end
end end
......
...@@ -166,8 +166,8 @@ describe MergeRequests::RefreshService do ...@@ -166,8 +166,8 @@ describe MergeRequests::RefreshService do
it 'create detached merge request pipeline with commits' do it 'create detached merge request pipeline with commits' do
expect { subject } expect { subject }
.to change { @merge_request.merge_request_pipelines.count }.by(1) .to change { @merge_request.pipelines_for_merge_request.count }.by(1)
.and change { @another_merge_request.merge_request_pipelines.count }.by(0) .and change { @another_merge_request.pipelines_for_merge_request.count }.by(0)
expect(@merge_request.has_commits?).to be_truthy expect(@merge_request.has_commits?).to be_truthy
expect(@another_merge_request.has_commits?).to be_falsy expect(@another_merge_request.has_commits?).to be_falsy
...@@ -175,13 +175,13 @@ describe MergeRequests::RefreshService do ...@@ -175,13 +175,13 @@ describe MergeRequests::RefreshService do
it 'does not create detached merge request pipeline for forked project' do it 'does not create detached merge request pipeline for forked project' do
expect { subject } expect { subject }
.not_to change { @fork_merge_request.merge_request_pipelines.count } .not_to change { @fork_merge_request.pipelines_for_merge_request.count }
end end
it 'create detached merge request pipeline for non-fork merge request' do it 'create detached merge request pipeline for non-fork merge request' do
subject subject
expect(@merge_request.merge_request_pipelines.first) expect(@merge_request.pipelines_for_merge_request.first)
.to be_detached_merge_request_pipeline .to be_detached_merge_request_pipeline
end end
...@@ -190,7 +190,7 @@ describe MergeRequests::RefreshService do ...@@ -190,7 +190,7 @@ describe MergeRequests::RefreshService do
it 'does not create detached merge request pipeline' do it 'does not create detached merge request pipeline' do
expect { subject } expect { subject }
.not_to change { @merge_request.merge_request_pipelines.count } .not_to change { @merge_request.pipelines_for_merge_request.count }
end end
end end
...@@ -199,9 +199,9 @@ describe MergeRequests::RefreshService do ...@@ -199,9 +199,9 @@ describe MergeRequests::RefreshService do
it 'creates legacy detached merge request pipeline for fork merge request' do it 'creates legacy detached merge request pipeline for fork merge request' do
expect { subject } expect { subject }
.to change { @fork_merge_request.merge_request_pipelines.count }.by(1) .to change { @fork_merge_request.pipelines_for_merge_request.count }.by(1)
expect(@fork_merge_request.merge_request_pipelines.first) expect(@fork_merge_request.pipelines_for_merge_request.first)
.to be_legacy_detached_merge_request_pipeline .to be_legacy_detached_merge_request_pipeline
end end
end end
...@@ -214,7 +214,7 @@ describe MergeRequests::RefreshService do ...@@ -214,7 +214,7 @@ describe MergeRequests::RefreshService do
it 'create legacy detached merge request pipeline for non-fork merge request' do it 'create legacy detached merge request pipeline for non-fork merge request' do
subject subject
expect(@merge_request.merge_request_pipelines.first) expect(@merge_request.pipelines_for_merge_request.first)
.to be_legacy_detached_merge_request_pipeline .to be_legacy_detached_merge_request_pipeline
end end
end end
...@@ -245,11 +245,11 @@ describe MergeRequests::RefreshService do ...@@ -245,11 +245,11 @@ describe MergeRequests::RefreshService do
it 'does not re-create a duplicate detached merge request pipeline' do it 'does not re-create a duplicate detached merge request pipeline' do
expect do expect do
service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/master') service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/master')
end.to change { @merge_request.merge_request_pipelines.count }.by(1) end.to change { @merge_request.pipelines_for_merge_request.count }.by(1)
expect do expect do
service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/master') service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/master')
end.not_to change { @merge_request.merge_request_pipelines.count } end.not_to change { @merge_request.pipelines_for_merge_request.count }
end end
end end
end end
...@@ -266,7 +266,7 @@ describe MergeRequests::RefreshService do ...@@ -266,7 +266,7 @@ describe MergeRequests::RefreshService do
it 'does not create a detached merge request pipeline' do it 'does not create a detached merge request pipeline' do
expect { subject } expect { subject }
.not_to change { @merge_request.merge_request_pipelines.count } .not_to change { @merge_request.pipelines_for_merge_request.count }
end end
end end
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