Commit b913169d authored by Shinya Maeda's avatar Shinya Maeda

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

Fix

Fix spec

Fix spec

fix

Simplify the things

Memoize

OK

a
parent 77d22d37
...@@ -13,6 +13,7 @@ module Ci ...@@ -13,6 +13,7 @@ module Ci
include EnumWithNil include EnumWithNil
include HasRef include HasRef
include ShaAttribute include ShaAttribute
include FromUnion
sha_attribute :source_sha sha_attribute :source_sha
sha_attribute :target_sha sha_attribute :target_sha
...@@ -185,32 +186,30 @@ module Ci ...@@ -185,32 +186,30 @@ module Ci
end end
scope :for_user, -> (user) { where(user: user) } scope :for_user, -> (user) { where(user: user) }
scope :for_sha, -> (sha) { where(sha: sha) }
scope :for_merge_request, -> (merge_request, ref, sha) do scope :for_source_sha, -> (source_sha) { where(source_sha: source_sha) }
## scope :for_sha_or_source_sha, -> (sha) { for_sha(sha).or(for_source_sha(sha)) }
# 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 scope :triggered_by_merge_request, -> (merge_request) do
where(source: :merge_request_event, merge_request: merge_request) where(source: :merge_request_event, merge_request: merge_request)
end end
scope :detached_merge_request_pipelines, -> (merge_request) do scope :detached_merge_request_pipelines, -> (merge_request, sha) do
triggered_by_merge_request(merge_request).where(target_sha: nil) triggered_by_merge_request(merge_request).for_sha(sha)
end end
scope :merge_request_pipelines, -> (merge_request) do scope :merge_request_pipelines, -> (merge_request, source_sha) do
triggered_by_merge_request(merge_request).where.not(target_sha: nil) triggered_by_merge_request(merge_request).for_source_sha(source_sha)
end end
scope :mergeable_merge_request_pipelines, -> (merge_request) do scope :mergeable_merge_request_pipelines, -> (merge_request) do
triggered_by_merge_request(merge_request).where(target_sha: merge_request.target_branch_sha) triggered_by_merge_request(merge_request).where(target_sha: merge_request.target_branch_sha)
end end
scope :triggered_for_branch, -> (ref) do
where(source: branch_pipeline_sources).where(ref: ref, tag: false)
end
# Returns the pipelines in descending order (= newest first), optionally # Returns the pipelines in descending order (= newest first), optionally
# limited to a number of references. # limited to a number of references.
# #
...@@ -298,8 +297,8 @@ module Ci ...@@ -298,8 +297,8 @@ module Ci
sources.reject { |source| source == "external" }.values sources.reject { |source| source == "external" }.values
end end
def self.latest_for_merge_request(merge_request, ref, sha) def self.branch_pipeline_sources
for_merge_request(merge_request, ref, sha).first @branch_pipeline_sources ||= sources.reject { |source| source == 'merge_request_event' }.values
end end
def self.ci_sources_values def self.ci_sources_values
......
...@@ -1149,12 +1149,18 @@ class MergeRequest < ActiveRecord::Base ...@@ -1149,12 +1149,18 @@ class MergeRequest < ActiveRecord::Base
diverged_commits_count > 0 diverged_commits_count > 0
end end
def all_pipelines(shas: all_commit_shas) def all_pipelines
return Ci::Pipeline.none unless source_project return Ci::Pipeline.none unless source_project
@all_pipelines ||= shas = all_commit_shas
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.merge_request_pipelines(self, shas),
source_project.ci_pipelines.detached_merge_request_pipelines(self, shas),
source_project.ci_pipelines.triggered_for_branch(source_branch).for_sha(shas)],
remove_duplicates: false).sort_by_merge_request_pipelines
end
end end
def update_head_pipeline def update_head_pipeline
...@@ -1389,8 +1395,7 @@ class MergeRequest < ActiveRecord::Base ...@@ -1389,8 +1395,7 @@ class MergeRequest < ActiveRecord::Base
private private
def find_actual_head_pipeline def find_actual_head_pipeline
source_project&.ci_pipelines all_pipelines.for_sha_or_source_sha(diff_head_sha).first
&.latest_for_merge_request(self, source_branch, diff_head_sha)
end end
def source_project_variables def source_project_variables
......
---
title: Refactor all_pipelines in Merge request
merge_request: 25676
author:
type: other
...@@ -106,7 +106,9 @@ FactoryBot.define do ...@@ -106,7 +106,9 @@ FactoryBot.define do
merge_request.merge_request_pipelines << build(:ci_pipeline, merge_request.merge_request_pipelines << build(: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,
ref: merge_request.source_branch,
sha: merge_request.source_branch_sha)
end end
end end
......
...@@ -130,22 +130,118 @@ describe Ci::Pipeline, :mailer do ...@@ -130,22 +130,118 @@ describe Ci::Pipeline, :mailer do
end end
end end
describe '.for_sha' do
subject { described_class.for_sha(sha) }
let(:sha) { 'abc' }
let!(:pipeline) { create(:ci_pipeline, sha: 'abc') }
it 'returns the pipeline' do
is_expected.to contain_exactly(pipeline)
end
context 'when argument is array' do
let(:sha) { %w[abc def] }
let!(:pipeline_2) { create(:ci_pipeline, sha: 'def') }
it 'returns the pipelines' do
is_expected.to contain_exactly(pipeline, pipeline_2)
end
end
context 'when sha is empty' do
let(:sha) { nil }
it 'does not return anything' do
is_expected.to be_empty
end
end
end
describe '.for_source_sha' do
subject { described_class.for_source_sha(source_sha) }
let(:source_sha) { 'abc' }
let!(:pipeline) { create(:ci_pipeline, source_sha: 'abc') }
it 'returns the pipeline' do
is_expected.to contain_exactly(pipeline)
end
context 'when argument is array' do
let(:source_sha) { %w[abc def] }
let!(:pipeline_2) { create(:ci_pipeline, source_sha: 'def') }
it 'returns the pipelines' do
is_expected.to contain_exactly(pipeline, pipeline_2)
end
end
context 'when source_sha is empty' do
let(:source_sha) { nil }
it 'does not return anything' do
is_expected.to be_empty
end
end
end
describe '.for_sha_or_source_sha' do
subject { described_class.for_sha_or_source_sha(sha) }
let(:sha) { 'abc' }
context 'when sha is matched' do
let!(:pipeline) { create(:ci_pipeline, sha: sha) }
it 'returns the pipeline' do
is_expected.to contain_exactly(pipeline)
end
end
context 'when source sha is matched' do
let!(:pipeline) { create(:ci_pipeline, source_sha: sha) }
it 'returns the pipeline' do
is_expected.to contain_exactly(pipeline)
end
end
context 'when both sha and source sha are not matched' do
let!(:pipeline) { create(:ci_pipeline, sha: 'bcd', source_sha: 'bcd') }
it 'does not return anything' do
is_expected.to be_empty
end
end
end
describe '.detached_merge_request_pipelines' do describe '.detached_merge_request_pipelines' do
subject { described_class.detached_merge_request_pipelines(merge_request) } subject { described_class.detached_merge_request_pipelines(merge_request, sha) }
let!(:pipeline) do let!(:pipeline) do
create(:ci_pipeline, source: :merge_request_event, merge_request: merge_request, target_sha: target_sha) create(:ci_pipeline, source: :merge_request_event, merge_request: merge_request, sha: merge_request.diff_head_sha)
end end
let(:merge_request) { create(:merge_request) } let(:merge_request) { create(:merge_request) }
let(:target_sha) { nil } let(:sha) { merge_request.diff_head_sha }
it 'returns detached merge request pipelines' do it 'returns detached merge request pipelines' do
is_expected.to eq([pipeline]) is_expected.to eq([pipeline])
end end
context 'when target sha exists' do context 'when sha does not exist' do
let(:target_sha) { merge_request.target_branch_sha } let(:sha) { 'abc' }
it 'returns empty array' do
is_expected.to be_empty
end
end
context 'when pipeline is merge request pipeline' do
let!(:pipeline) do
create(:ci_pipeline, source: :merge_request_event, merge_request: merge_request, source_sha: merge_request.diff_head_sha)
end
it 'returns empty array' do it 'returns empty array' do
is_expected.to be_empty is_expected.to be_empty
...@@ -173,21 +269,31 @@ describe Ci::Pipeline, :mailer do ...@@ -173,21 +269,31 @@ describe Ci::Pipeline, :mailer do
end end
describe '.merge_request_pipelines' do describe '.merge_request_pipelines' do
subject { described_class.merge_request_pipelines(merge_request) } subject { described_class.merge_request_pipelines(merge_request, source_sha) }
let!(:pipeline) do let!(:pipeline) do
create(:ci_pipeline, source: :merge_request_event, merge_request: merge_request, target_sha: target_sha) create(:ci_pipeline, source: :merge_request_event, merge_request: merge_request, source_sha: merge_request.diff_head_sha)
end end
let(:merge_request) { create(:merge_request) } let(:merge_request) { create(:merge_request) }
let(:target_sha) { merge_request.target_branch_sha } let(:source_sha) { merge_request.diff_head_sha }
it 'returns merge pipelines' do it 'returns merge pipelines' do
is_expected.to eq([pipeline]) is_expected.to eq([pipeline])
end end
context 'when target sha is empty' do context 'when source sha is empty' do
let(:target_sha) { nil } let(:source_sha) { nil }
it 'returns empty array' do
is_expected.to be_empty
end
end
context 'when pipeline is detached merge request pipeline' do
let!(:pipeline) do
create(:ci_pipeline, source: :merge_request_event, merge_request: merge_request, sha: merge_request.diff_head_sha)
end
it 'returns empty array' do it 'returns empty array' do
is_expected.to be_empty is_expected.to be_empty
...@@ -256,6 +362,50 @@ describe Ci::Pipeline, :mailer do ...@@ -256,6 +362,50 @@ describe Ci::Pipeline, :mailer do
end end
end end
describe '.triggered_for_branch' do
subject { described_class.triggered_for_branch(ref) }
let(:project) { create(:project, :repository) }
let(:ref) { 'feature' }
let!(:pipeline) { create(:ci_pipeline, ref: ref) }
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!(:pipeline) { create(:ci_pipeline, ref: ref, 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 describe '.merge_request_event' do
subject { described_class.merge_request_event } subject { described_class.merge_request_event }
......
...@@ -1350,7 +1350,7 @@ describe MergeRequest do ...@@ -1350,7 +1350,7 @@ describe MergeRequest do
sha: shas.second) sha: shas.second)
end end
let!(:merge_request_pipeline) do let!(:detached_merge_request_pipeline) do
create(:ci_pipeline, create(:ci_pipeline,
source: :merge_request_event, source: :merge_request_event,
project: project, project: project,
...@@ -1376,7 +1376,7 @@ describe MergeRequest do ...@@ -1376,7 +1376,7 @@ describe MergeRequest do
it 'returns merge request pipeline first' do it 'returns merge request pipeline first' do
expect(merge_request.all_pipelines) expect(merge_request.all_pipelines)
.to eq([merge_request_pipeline, .to eq([detached_merge_request_pipeline,
branch_pipeline]) branch_pipeline])
end end
...@@ -1389,7 +1389,7 @@ describe MergeRequest do ...@@ -1389,7 +1389,7 @@ describe MergeRequest do
sha: shas.first) sha: shas.first)
end end
let!(:merge_request_pipeline_2) do let!(:detached_merge_request_pipeline_2) do
create(:ci_pipeline, create(:ci_pipeline,
source: :merge_request_event, source: :merge_request_event,
project: project, project: project,
...@@ -1400,8 +1400,8 @@ describe MergeRequest do ...@@ -1400,8 +1400,8 @@ describe MergeRequest do
it 'returns merge request pipelines first' do it 'returns merge request pipelines first' do
expect(merge_request.all_pipelines) expect(merge_request.all_pipelines)
.to eq([merge_request_pipeline_2, .to eq([detached_merge_request_pipeline_2,
merge_request_pipeline, detached_merge_request_pipeline,
branch_pipeline_2, branch_pipeline_2,
branch_pipeline]) branch_pipeline])
end end
...@@ -1416,7 +1416,7 @@ describe MergeRequest do ...@@ -1416,7 +1416,7 @@ describe MergeRequest do
sha: shas.first) sha: shas.first)
end end
let!(:merge_request_pipeline_2) do let!(:detached_merge_request_pipeline_2) do
create(:ci_pipeline, create(:ci_pipeline,
source: :merge_request_event, source: :merge_request_event,
project: project, project: project,
...@@ -1439,16 +1439,35 @@ describe MergeRequest do ...@@ -1439,16 +1439,35 @@ describe MergeRequest do
it 'returns only related merge request pipelines' do it 'returns only related merge request pipelines' do
expect(merge_request.all_pipelines) expect(merge_request.all_pipelines)
.to eq([merge_request_pipeline, .to eq([detached_merge_request_pipeline,
branch_pipeline_2, branch_pipeline_2,
branch_pipeline]) branch_pipeline])
expect(merge_request_2.all_pipelines) expect(merge_request_2.all_pipelines)
.to eq([merge_request_pipeline_2, .to eq([detached_merge_request_pipeline_2,
branch_pipeline_2, branch_pipeline_2,
branch_pipeline]) branch_pipeline])
end end
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
end end
...@@ -1489,6 +1508,37 @@ describe MergeRequest do ...@@ -1489,6 +1508,37 @@ describe MergeRequest do
end end
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: sha,
merge_request: merge_request)
end
let(:sha) { merge_request.diff_head_sha }
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
context 'when sha is not HEAD of the source branch' do
let(:sha) { merge_request.diff_base_sha }
it 'does not update head pipeline' do
expect { subject }.not_to change { merge_request.reload.head_pipeline }
end
end
end
context 'when there are no pipelines with the diff head sha' do context 'when there are no pipelines with the diff head sha' do
it 'does not update the head pipeline' do it 'does not update the head pipeline' do
expect { subject } expect { subject }
......
...@@ -4,12 +4,14 @@ describe PipelineEntity do ...@@ -4,12 +4,14 @@ describe PipelineEntity do
include Gitlab::Routing include Gitlab::Routing
set(:user) { create(:user) } set(:user) { create(:user) }
set(:project) { create(:project) }
let(:request) { double('request') } let(:request) { double('request') }
before do before do
stub_not_protect_default_branch stub_not_protect_default_branch
allow(request).to receive(:current_user).and_return(user) allow(request).to receive(:current_user).and_return(user)
allow(request).to receive(:project).and_return(project)
end end
let(:entity) do let(:entity) do
......
...@@ -5,7 +5,7 @@ describe PipelineSerializer do ...@@ -5,7 +5,7 @@ describe PipelineSerializer do
set(:user) { create(:user) } set(:user) { create(:user) }
let(:serializer) do let(:serializer) do
described_class.new(current_user: user) described_class.new(current_user: user, project: project)
end end
before do before do
...@@ -106,7 +106,7 @@ describe PipelineSerializer do ...@@ -106,7 +106,7 @@ describe PipelineSerializer do
target_project: project, target_project: project,
target_branch: 'master', target_branch: 'master',
source_project: project, source_project: project,
source_branch: 'feature-1') source_branch: 'feature')
end end
let!(:merge_request_2) do let!(:merge_request_2) do
...@@ -115,7 +115,7 @@ describe PipelineSerializer do ...@@ -115,7 +115,7 @@ describe PipelineSerializer do
target_project: project, target_project: project,
target_branch: 'master', target_branch: 'master',
source_project: project, source_project: project,
source_branch: 'feature-2') source_branch: '2-mb-file')
end end
before do before do
......
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