summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorKamil Trzciński <ayufan@ayufan.eu>2019-03-13 13:57:12 +0000
committerKamil Trzciński <ayufan@ayufan.eu>2019-03-13 13:57:12 +0000
commit3c40c57383cdcc9e35b93d1f8142f531557ee414 (patch)
tree05e35d955f379bb341eda9a8806c1616953b38f8
parent535410a9f1661f7437614e21de95edcde0af292e (diff)
parentb913169d887bab6b02469cac4d245e2612deb76d (diff)
downloadgitlab-ce-3c40c57383cdcc9e35b93d1f8142f531557ee414.tar.gz
Merge branch 'use-only-all-pipelines' into 'master'
Refactor all_pipelines in Merge request in order to support all pipelines for merge request See merge request gitlab-org/gitlab-ce!25676
-rw-r--r--app/models/ci/pipeline.rb29
-rw-r--r--app/models/merge_request.rb17
-rw-r--r--changelogs/unreleased/use-only-all-pipelines.yml5
-rw-r--r--spec/factories/merge_requests.rb4
-rw-r--r--spec/models/ci/pipeline_spec.rb170
-rw-r--r--spec/models/merge_request_spec.rb66
-rw-r--r--spec/serializers/pipeline_entity_spec.rb2
-rw-r--r--spec/serializers/pipeline_serializer_spec.rb6
8 files changed, 256 insertions, 43 deletions
diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index ca9725f7a04..adffdc0355e 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -13,6 +13,7 @@ module Ci
include EnumWithNil
include HasRef
include ShaAttribute
+ include FromUnion
sha_attribute :source_sha
sha_attribute :target_sha
@@ -185,32 +186,30 @@ module Ci
end
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 :for_sha, -> (sha) { where(sha: sha) }
+ 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)) }
scope :triggered_by_merge_request, -> (merge_request) do
where(source: :merge_request_event, merge_request: merge_request)
end
- scope :detached_merge_request_pipelines, -> (merge_request) do
- triggered_by_merge_request(merge_request).where(target_sha: nil)
+ scope :detached_merge_request_pipelines, -> (merge_request, sha) do
+ triggered_by_merge_request(merge_request).for_sha(sha)
end
- scope :merge_request_pipelines, -> (merge_request) do
- triggered_by_merge_request(merge_request).where.not(target_sha: nil)
+ scope :merge_request_pipelines, -> (merge_request, source_sha) do
+ triggered_by_merge_request(merge_request).for_source_sha(source_sha)
end
scope :mergeable_merge_request_pipelines, -> (merge_request) do
triggered_by_merge_request(merge_request).where(target_sha: merge_request.target_branch_sha)
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
# limited to a number of references.
#
@@ -298,8 +297,8 @@ module Ci
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
+ def self.branch_pipeline_sources
+ @branch_pipeline_sources ||= sources.reject { |source| source == 'merge_request_event' }.values
end
def self.ci_sources_values
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index af8cb37bfb6..aeb6acf0ac0 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -1149,12 +1149,18 @@ class MergeRequest < ActiveRecord::Base
diverged_commits_count > 0
end
- def all_pipelines(shas: all_commit_shas)
+ def all_pipelines
return Ci::Pipeline.none unless source_project
- @all_pipelines ||=
- source_project.ci_pipelines
- .for_merge_request(self, source_branch, all_commit_shas)
+ shas = 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
def update_head_pipeline
@@ -1389,8 +1395,7 @@ class MergeRequest < ActiveRecord::Base
private
def find_actual_head_pipeline
- source_project&.ci_pipelines
- &.latest_for_merge_request(self, source_branch, diff_head_sha)
+ all_pipelines.for_sha_or_source_sha(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 00000000000..68364d2a923
--- /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 18f724770b5..ecd7ea65fb7 100644
--- a/spec/factories/merge_requests.rb
+++ b/spec/factories/merge_requests.rb
@@ -106,7 +106,9 @@ FactoryBot.define do
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 d0b42d103a5..7eeaa7a18ef 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -130,22 +130,118 @@ describe Ci::Pipeline, :mailer do
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
- subject { described_class.detached_merge_request_pipelines(merge_request) }
+ subject { described_class.detached_merge_request_pipelines(merge_request, sha) }
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
let(:merge_request) { create(:merge_request) }
- let(:target_sha) { nil }
+ let(:sha) { merge_request.diff_head_sha }
it 'returns detached merge request pipelines' do
is_expected.to eq([pipeline])
end
- context 'when target sha exists' do
- let(:target_sha) { merge_request.target_branch_sha }
+ context 'when sha does not exist' do
+ 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
is_expected.to be_empty
@@ -173,21 +269,31 @@ describe Ci::Pipeline, :mailer do
end
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
- 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
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
is_expected.to eq([pipeline])
end
- context 'when target sha is empty' do
- let(:target_sha) { nil }
+ context 'when source sha is empty' do
+ 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
is_expected.to be_empty
@@ -256,6 +362,50 @@ describe Ci::Pipeline, :mailer do
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
subject { described_class.merge_request_event }
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index a35d3f14df8..42c49e330cc 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1350,7 +1350,7 @@ describe MergeRequest do
sha: shas.second)
end
- let!(:merge_request_pipeline) do
+ let!(:detached_merge_request_pipeline) do
create(:ci_pipeline,
source: :merge_request_event,
project: project,
@@ -1376,7 +1376,7 @@ describe MergeRequest do
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
@@ -1389,7 +1389,7 @@ describe MergeRequest do
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,
@@ -1400,8 +1400,8 @@ describe MergeRequest do
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
@@ -1416,7 +1416,7 @@ describe MergeRequest do
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,
@@ -1439,16 +1439,35 @@ describe MergeRequest do
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
@@ -1489,6 +1508,37 @@ describe MergeRequest do
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
it 'does not update the head pipeline' do
expect { subject }
diff --git a/spec/serializers/pipeline_entity_spec.rb b/spec/serializers/pipeline_entity_spec.rb
index 11040862129..d510293ae38 100644
--- a/spec/serializers/pipeline_entity_spec.rb
+++ b/spec/serializers/pipeline_entity_spec.rb
@@ -4,12 +4,14 @@ describe PipelineEntity do
include Gitlab::Routing
set(:user) { create(:user) }
+ set(:project) { create(:project) }
let(:request) { double('request') }
before do
stub_not_protect_default_branch
allow(request).to receive(:current_user).and_return(user)
+ allow(request).to receive(:project).and_return(project)
end
let(:entity) do
diff --git a/spec/serializers/pipeline_serializer_spec.rb b/spec/serializers/pipeline_serializer_spec.rb
index a21487938a0..cead75c0895 100644
--- a/spec/serializers/pipeline_serializer_spec.rb
+++ b/spec/serializers/pipeline_serializer_spec.rb
@@ -5,7 +5,7 @@ describe PipelineSerializer do
set(:user) { create(:user) }
let(:serializer) do
- described_class.new(current_user: user)
+ described_class.new(current_user: user, project: project)
end
before do
@@ -106,7 +106,7 @@ describe PipelineSerializer do
target_project: project,
target_branch: 'master',
source_project: project,
- source_branch: 'feature-1')
+ source_branch: 'feature')
end
let!(:merge_request_2) do
@@ -115,7 +115,7 @@ describe PipelineSerializer do
target_project: project,
target_branch: 'master',
source_project: project,
- source_branch: 'feature-2')
+ source_branch: '2-mb-file')
end
before do