From f5d181198e8a74b8c1a393512f5456d036da293b Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 1 Mar 2019 15:18:08 +0900 Subject: [PATCH] Make all_pipelines method compatible with pipelines for merge requests Make it sane Include merge ref head Fix union Improve a bit Add spec remove add spec Add changelog fix coding offence Apply suggestion to spec/models/merge_request_spec.rb ok ok --- app/models/ci/pipeline.rb | 20 +++---- app/models/merge_request.rb | 12 ++-- .../unreleased/use-only-all-pipelines.yml | 5 ++ spec/factories/merge_requests.rb | 4 +- spec/models/ci/pipeline_spec.rb | 46 +++++++++++++++ spec/models/merge_request_spec.rb | 56 ++++++++++++++++--- 6 files changed, 116 insertions(+), 27 deletions(-) create mode 100644 changelogs/unreleased/use-only-all-pipelines.yml diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 90cb069b7b2e0f..dcf05d3a8dbdb5 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -13,6 +13,7 @@ class Pipeline < ActiveRecord::Base include EnumWithNil include HasRef include ShaAttribute + include FromUnion sha_attribute :source_sha sha_attribute :target_sha @@ -186,15 +187,6 @@ class Pipeline < ActiveRecord::Base scope :for_user, -> (user) { where(user: user) } - scope :for_merge_request, -> (merge_request, ref, sha) do - ## - # We have to filter out unrelated MR pipelines. - # When merge request is empty, it selects general pipelines, such as push sourced pipelines. - # When merge request is matched, it selects MR pipelines. - where(merge_request: [nil, merge_request], ref: ref, sha: sha) - .sort_by_merge_request_pipelines - end - scope :triggered_by_merge_request, -> (merge_request) do where(source: :merge_request_event, merge_request: merge_request) end @@ -211,6 +203,12 @@ class Pipeline < ActiveRecord::Base triggered_by_merge_request(merge_request).where(target_sha: merge_request.target_branch_sha) end + scope :triggered_for_branch, -> (ref, shas = nil) do + query = where(source: sources.keys - %w[merge_request_event]).where(ref: ref, tag: false) + query = query.where(sha: shas) if shas + query + end + # Returns the pipelines in descending order (= newest first), optionally # limited to a number of references. # @@ -298,10 +296,6 @@ def self.internal_sources sources.reject { |source| source == "external" }.values end - def self.latest_for_merge_request(merge_request, ref, sha) - for_merge_request(merge_request, ref, sha).first - end - def self.ci_sources_values config_sources.values_at(:repository_source, :auto_devops_source, :unknown_source) end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 2db3c0e28ade7b..f45ded2d9f034d 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -1131,9 +1131,12 @@ def diverged_from_target_branch? def all_pipelines(shas: all_commit_shas) return Ci::Pipeline.none unless source_project - @all_pipelines ||= - source_project.ci_pipelines - .for_merge_request(self, source_branch, all_commit_shas) + strong_memoize(:all_pipelines) do + Ci::Pipeline.from_union( + [source_project.ci_pipelines.triggered_by_merge_request(self), + source_project.ci_pipelines.triggered_for_branch(source_branch, shas)], + remove_duplicates: false).sort_by_merge_request_pipelines + end end def update_head_pipeline @@ -1368,8 +1371,7 @@ def squash_in_progress? private def find_actual_head_pipeline - source_project&.ci_pipelines - &.latest_for_merge_request(self, source_branch, diff_head_sha) + all_pipelines(shas: diff_head_sha).first end def source_project_variables diff --git a/changelogs/unreleased/use-only-all-pipelines.yml b/changelogs/unreleased/use-only-all-pipelines.yml new file mode 100644 index 00000000000000..68364d2a923043 --- /dev/null +++ b/changelogs/unreleased/use-only-all-pipelines.yml @@ -0,0 +1,5 @@ +--- +title: Refactor all_pipelines in Merge request +merge_request: 25676 +author: +type: other diff --git a/spec/factories/merge_requests.rb b/spec/factories/merge_requests.rb index 18f724770b5859..ecd7ea65fb72e8 100644 --- a/spec/factories/merge_requests.rb +++ b/spec/factories/merge_requests.rb @@ -106,7 +106,9 @@ merge_request.merge_request_pipelines << build(:ci_pipeline, source: :merge_request_event, merge_request: merge_request, - project: merge_request.source_project) + project: merge_request.source_project, + ref: merge_request.source_branch, + sha: merge_request.source_branch_sha) end end diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 31ff9e46f3ee9b..56f4f2d3307624 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -260,6 +260,52 @@ end end + describe '.triggered_for_branch' do + subject { described_class.triggered_for_branch(ref, sha) } + + let(:project) { create(:project, :repository) } + let(:ref) { 'feature' } + let(:sha) { project.repository.commit(ref).id } + let!(:pipeline) { create(:ci_pipeline, ref: ref, sha: sha) } + + it 'returns the pipeline' do + is_expected.to eq([pipeline]) + end + + context 'when sha is not specified' do + it 'returns the pipeline' do + expect(described_class.triggered_for_branch(ref)).to eq([pipeline]) + end + end + + context 'when pipeline is triggered for tag' do + let(:ref) { 'v1.1.0' } + let(:sha) { project.repository.commit(ref).id } + let!(:pipeline) { create(:ci_pipeline, ref: ref, sha: sha, tag: true) } + + it 'does not return the pipeline' do + is_expected.to be_empty + end + end + + context 'when pipeline is triggered for merge_request' do + let!(:merge_request) do + create(:merge_request, + :with_merge_request_pipeline, + source_project: project, + source_branch: ref, + target_project: project, + target_branch: 'master') + end + + let(:pipeline) { merge_request.merge_request_pipelines.first } + + it 'does not return the pipeline' do + is_expected.to be_empty + end + end + end + describe '.merge_request_event' do subject { described_class.merge_request_event } diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 85120a96b30fa6..62c6972fd53303 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -1330,7 +1330,7 @@ def set_compare(merge_request) sha: shas.second) end - let!(:merge_request_pipeline) do + let!(:detached_merge_request_pipeline) do create(:ci_pipeline, source: :merge_request_event, project: project, @@ -1356,7 +1356,7 @@ def set_compare(merge_request) it 'returns merge request pipeline first' do expect(merge_request.all_pipelines) - .to eq([merge_request_pipeline, + .to eq([detached_merge_request_pipeline, branch_pipeline]) end @@ -1369,7 +1369,7 @@ def set_compare(merge_request) sha: shas.first) end - let!(:merge_request_pipeline_2) do + let!(:detached_merge_request_pipeline_2) do create(:ci_pipeline, source: :merge_request_event, project: project, @@ -1380,8 +1380,8 @@ def set_compare(merge_request) it 'returns merge request pipelines first' do expect(merge_request.all_pipelines) - .to eq([merge_request_pipeline_2, - merge_request_pipeline, + .to eq([detached_merge_request_pipeline_2, + detached_merge_request_pipeline, branch_pipeline_2, branch_pipeline]) end @@ -1396,7 +1396,7 @@ def set_compare(merge_request) sha: shas.first) end - let!(:merge_request_pipeline_2) do + let!(:detached_merge_request_pipeline_2) do create(:ci_pipeline, source: :merge_request_event, project: project, @@ -1419,16 +1419,35 @@ def set_compare(merge_request) it 'returns only related merge request pipelines' do expect(merge_request.all_pipelines) - .to eq([merge_request_pipeline, + .to eq([detached_merge_request_pipeline, branch_pipeline_2, branch_pipeline]) expect(merge_request_2.all_pipelines) - .to eq([merge_request_pipeline_2, + .to eq([detached_merge_request_pipeline_2, branch_pipeline_2, branch_pipeline]) end end + + context 'when detached merge request pipeline is run on head ref of the merge request' do + let!(:detached_merge_request_pipeline) do + create(:ci_pipeline, + source: :merge_request_event, + project: project, + ref: merge_request.ref_path, + sha: shas.second, + merge_request: merge_request) + end + + it 'sets the head ref of the merge request to the pipeline ref' do + expect(detached_merge_request_pipeline.ref).to match(%r{refs/merge-requests/\d+/head}) + end + + it 'includes the detached merge request pipeline even though the ref is custom path' do + expect(merge_request.all_pipelines).to include(detached_merge_request_pipeline) + end + end end end @@ -1469,6 +1488,27 @@ def set_compare(merge_request) end end + context 'when detached merge request pipeline is run on head ref of the merge request' do + let!(:pipeline) do + create(:ci_pipeline, + source: :merge_request_event, + project: merge_request.source_project, + ref: merge_request.ref_path, + sha: merge_request.diff_head_sha, + merge_request: merge_request) + end + + it 'sets the head ref of the merge request to the pipeline ref' do + expect(pipeline.ref).to match(%r{refs/merge-requests/\d+/head}) + end + + it 'updates correctly even though the target branch name of the merge request is different from the pipeline ref' do + expect { subject } + .to change { merge_request.reload.head_pipeline } + .from(nil).to(pipeline) + end + end + context 'when there are no pipelines with the diff head sha' do it 'does not update the head pipeline' do expect { subject } -- GitLab