summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorBob Van Landuyt <bob@gitlab.com>2019-05-06 10:45:21 +0000
committerBob Van Landuyt <bob@gitlab.com>2019-05-06 10:45:21 +0000
commitd7adc4cf3ca53fee784c2e0966f127334a9dad54 (patch)
treea3d8cbe984e8ef33e3db1df5a1c74bde58e73b31
parentd7eb886b9fd32ad2d0ab7bca9128dbb40e80c0da (diff)
parentd4d2cf7327ed077e063d64dcfe3b8275286c2d06 (diff)
downloadgitlab-ce-d7adc4cf3ca53fee784c2e0966f127334a9dad54.tar.gz
Merge branch 'fix-merge-request-pipeline-exist-method' into 'master'
Fix duplicate merge request pipelines created by Sidekiq worker retry See merge request gitlab-org/gitlab-ce!26643
-rw-r--r--app/models/merge_request.rb10
-rw-r--r--app/services/merge_requests/base_service.rb2
-rw-r--r--changelogs/unreleased/fix-merge-request-pipeline-exist-method.yml5
-rw-r--r--spec/factories/merge_requests.rb6
-rw-r--r--spec/lib/gitlab/import_export/all_models.yml2
-rw-r--r--spec/models/ci/build_spec.rb4
-rw-r--r--spec/models/ci/pipeline_spec.rb2
-rw-r--r--spec/models/concerns/has_ref_spec.rb4
-rw-r--r--spec/serializers/pipeline_entity_spec.rb4
-rw-r--r--spec/services/merge_requests/create_service_spec.rb6
-rw-r--r--spec/services/merge_requests/refresh_service_spec.rb22
11 files changed, 34 insertions, 33 deletions
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index c2a1487fc6e..df162e4844c 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -66,7 +66,7 @@ class MergeRequest < ApplicationRecord
dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
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 :merge_request_assignees
@@ -1157,10 +1157,6 @@ class MergeRequest < ApplicationRecord
end
end
- def merge_request_pipeline_exists?
- merge_request_pipelines.exists?(sha: diff_head_sha)
- end
-
def has_test_reports?
actual_head_pipeline&.has_reports?(Ci::JobArtifact.test_reports)
end
@@ -1379,12 +1375,12 @@ class MergeRequest < ApplicationRecord
source_project.repository.squash_in_progress?(id)
end
- private
-
def find_actual_head_pipeline
all_pipelines.for_sha_or_source_sha(diff_head_sha).first
end
+ private
+
def source_project_variables
Gitlab::Ci::Variables::Collection.new.tap do |variables|
break variables unless source_project
diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb
index a9dd26c02ad..bb9062e9b40 100644
--- a/app/services/merge_requests/base_service.rb
+++ b/app/services/merge_requests/base_service.rb
@@ -81,7 +81,7 @@ module MergeRequests
##
# UpdateMergeRequestsWorker could be retried by an exception.
# 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?
true
diff --git a/changelogs/unreleased/fix-merge-request-pipeline-exist-method.yml b/changelogs/unreleased/fix-merge-request-pipeline-exist-method.yml
new file mode 100644
index 00000000000..294a665ff3e
--- /dev/null
+++ b/changelogs/unreleased/fix-merge-request-pipeline-exist-method.yml
@@ -0,0 +1,5 @@
+---
+title: Fix duplicate merge request pipelines created by Sidekiq worker retry
+merge_request: 26643
+author:
+type: fixed
diff --git a/spec/factories/merge_requests.rb b/spec/factories/merge_requests.rb
index abf0e6bccb7..e8df5094b83 100644
--- a/spec/factories/merge_requests.rb
+++ b/spec/factories/merge_requests.rb
@@ -119,7 +119,7 @@ FactoryBot.define do
trait :with_legacy_detached_merge_request_pipeline do
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,
merge_request: merge_request,
project: merge_request.source_project,
@@ -130,7 +130,7 @@ FactoryBot.define do
trait :with_detached_merge_request_pipeline do
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,
merge_request: merge_request,
project: merge_request.source_project,
@@ -147,7 +147,7 @@ FactoryBot.define do
end
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,
merge_request: merge_request,
project: merge_request.source_project,
diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml
index 482e9c05da8..2242543daad 100644
--- a/spec/lib/gitlab/import_export/all_models.yml
+++ b/spec/lib/gitlab/import_export/all_models.yml
@@ -99,7 +99,7 @@ merge_requests:
- timelogs
- head_pipeline
- latest_merge_request_diff
-- merge_request_pipelines
+- pipelines_for_merge_request
- merge_request_assignees
- suggestions
- assignees
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 8c73f37bd32..9b489baf163 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -2817,7 +2817,7 @@ describe Ci::Build do
context 'when ref is merge request' do
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) }
context 'when ref is protected' do
@@ -2875,7 +2875,7 @@ describe Ci::Build do
context 'when ref is merge request' do
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) }
context 'when ref is protected' do
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index af455a72f50..a0319b3eb0a 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -466,7 +466,7 @@ describe Ci::Pipeline, :mailer do
target_branch: 'master')
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
is_expected.to be_empty
diff --git a/spec/models/concerns/has_ref_spec.rb b/spec/models/concerns/has_ref_spec.rb
index 6805731fed3..66b25c77430 100644
--- a/spec/models/concerns/has_ref_spec.rb
+++ b/spec/models/concerns/has_ref_spec.rb
@@ -19,7 +19,7 @@ describe HasRef do
context 'when it was triggered by merge request' do
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) }
it 'returns false' do
@@ -68,7 +68,7 @@ describe HasRef do
context 'when it is triggered by a merge request' do
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) }
it 'returns nil' do
diff --git a/spec/serializers/pipeline_entity_spec.rb b/spec/serializers/pipeline_entity_spec.rb
index dba7fd91747..47f767ae4ab 100644
--- a/spec/serializers/pipeline_entity_spec.rb
+++ b/spec/serializers/pipeline_entity_spec.rb
@@ -137,7 +137,7 @@ describe PipelineEntity do
context 'when pipeline is detached merge request pipeline' do
let(:merge_request) { create(:merge_request, :with_detached_merge_request_pipeline) }
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
expect(subject[:flags][:detached_merge_request_pipeline]).to be_truthy
@@ -185,7 +185,7 @@ describe PipelineEntity do
context 'when pipeline is merge request pipeline' do
let(:merge_request) { create(:merge_request, :with_merge_request_pipeline, merge_sha: 'abc') }
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
expect(subject[:flags][:detached_merge_request_pipeline]).to be_falsy
diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb
index c795176a1e4..ed48f4b1e44 100644
--- a/spec/services/merge_requests/create_service_spec.rb
+++ b/spec/services/merge_requests/create_service_spec.rb
@@ -195,7 +195,7 @@ describe MergeRequests::CreateService do
expect(merge_request).to be_persisted
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
end
@@ -247,7 +247,7 @@ describe MergeRequests::CreateService do
expect(merge_request).to be_persisted
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
@@ -281,7 +281,7 @@ describe MergeRequests::CreateService do
expect(merge_request).to be_persisted
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
diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb
index d20b2d81763..7258428589f 100644
--- a/spec/services/merge_requests/refresh_service_spec.rb
+++ b/spec/services/merge_requests/refresh_service_spec.rb
@@ -166,8 +166,8 @@ describe MergeRequests::RefreshService do
it 'create detached merge request pipeline with commits' do
expect { subject }
- .to change { @merge_request.merge_request_pipelines.count }.by(1)
- .and change { @another_merge_request.merge_request_pipelines.count }.by(0)
+ .to change { @merge_request.pipelines_for_merge_request.count }.by(1)
+ .and change { @another_merge_request.pipelines_for_merge_request.count }.by(0)
expect(@merge_request.has_commits?).to be_truthy
expect(@another_merge_request.has_commits?).to be_falsy
@@ -175,13 +175,13 @@ describe MergeRequests::RefreshService do
it 'does not create detached merge request pipeline for forked project' do
expect { subject }
- .not_to change { @fork_merge_request.merge_request_pipelines.count }
+ .not_to change { @fork_merge_request.pipelines_for_merge_request.count }
end
it 'create detached merge request pipeline for non-fork merge request' do
subject
- expect(@merge_request.merge_request_pipelines.first)
+ expect(@merge_request.pipelines_for_merge_request.first)
.to be_detached_merge_request_pipeline
end
@@ -190,7 +190,7 @@ describe MergeRequests::RefreshService do
it 'does not create detached merge request pipeline' do
expect { subject }
- .not_to change { @merge_request.merge_request_pipelines.count }
+ .not_to change { @merge_request.pipelines_for_merge_request.count }
end
end
@@ -199,9 +199,9 @@ describe MergeRequests::RefreshService do
it 'creates legacy detached merge request pipeline for fork merge request' do
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
end
end
@@ -214,7 +214,7 @@ describe MergeRequests::RefreshService do
it 'create legacy detached merge request pipeline for non-fork merge request' do
subject
- expect(@merge_request.merge_request_pipelines.first)
+ expect(@merge_request.pipelines_for_merge_request.first)
.to be_legacy_detached_merge_request_pipeline
end
end
@@ -245,11 +245,11 @@ describe MergeRequests::RefreshService do
it 'does not re-create a duplicate detached merge request pipeline' do
expect do
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
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
@@ -266,7 +266,7 @@ describe MergeRequests::RefreshService do
it 'does not create a detached merge request pipeline' do
expect { subject }
- .not_to change { @merge_request.merge_request_pipelines.count }
+ .not_to change { @merge_request.pipelines_for_merge_request.count }
end
end
end