diff options
author | Shinya Maeda <shinya@gitlab.com> | 2019-03-27 15:46:29 +0700 |
---|---|---|
committer | Shinya Maeda <shinya@gitlab.com> | 2019-05-06 13:20:47 +0700 |
commit | d4d2cf7327ed077e063d64dcfe3b8275286c2d06 (patch) | |
tree | e34dd4a38e8dd37670f6922e1b884634132c89b9 | |
parent | 9aa81c0a95b0e54bbafca4755e95f27ee1eeb759 (diff) | |
download | gitlab-ce-fix-merge-request-pipeline-exist-method.tar.gz |
Fix merge request pipeline exist methodfix-merge-request-pipeline-exist-method
Refactor
-rw-r--r-- | app/models/merge_request.rb | 10 | ||||
-rw-r--r-- | app/services/merge_requests/base_service.rb | 2 | ||||
-rw-r--r-- | changelogs/unreleased/fix-merge-request-pipeline-exist-method.yml | 5 | ||||
-rw-r--r-- | spec/factories/merge_requests.rb | 6 | ||||
-rw-r--r-- | spec/lib/gitlab/import_export/all_models.yml | 2 | ||||
-rw-r--r-- | spec/models/ci/build_spec.rb | 4 | ||||
-rw-r--r-- | spec/models/ci/pipeline_spec.rb | 2 | ||||
-rw-r--r-- | spec/models/concerns/has_ref_spec.rb | 4 | ||||
-rw-r--r-- | spec/serializers/pipeline_entity_spec.rb | 4 | ||||
-rw-r--r-- | spec/services/merge_requests/create_service_spec.rb | 6 | ||||
-rw-r--r-- | spec/services/merge_requests/refresh_service_spec.rb | 22 |
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 |