summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--app/models/merge_request.rb8
-rw-r--r--app/services/ci/create_pipeline_service.rb23
-rw-r--r--app/services/merge_requests/refresh_service.rb1
-rw-r--r--app/workers/update_head_pipeline_for_merge_request_worker.rb15
-rw-r--r--changelogs/unreleased/37354-pipelines-update.yml5
-rw-r--r--spec/features/merge_requests/pipelines_spec.rb15
-rw-r--r--spec/models/merge_request_spec.rb28
-rw-r--r--spec/services/ci/create_pipeline_service_spec.rb43
-rw-r--r--spec/services/merge_requests/refresh_service_spec.rb14
-rw-r--r--spec/workers/update_head_pipeline_for_merge_request_worker_spec.rb38
10 files changed, 162 insertions, 28 deletions
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index bbc01e9677c..d2ed6f7a8ae 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -145,6 +145,14 @@ class MergeRequest < ActiveRecord::Base
'!'
end
+ def head_pipeline
+ return unless head_pipeline_id
+
+ last_pipeline = Ci::Pipeline.find(head_pipeline_id)
+
+ last_pipeline.sha == diff_head_sha ? last_pipeline : nil
+ end
+
# Pattern used to extract `!123` merge request references from text
#
# This pattern supports cross-project references.
diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index 7b9ea223d26..1e5f2ed4dd2 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -29,7 +29,7 @@ module Ci
.new(pipeline, command, SEQUENCE)
sequence.build! do |pipeline, sequence|
- update_merge_requests_head_pipeline if pipeline.persisted?
+ schedule_head_pipeline_update
if sequence.complete?
cancel_pending_pipelines if project.auto_cancel_pending_pipelines?
@@ -38,15 +38,18 @@ module Ci
pipeline.process!
end
end
+
+ pipeline
end
private
- def update_merge_requests_head_pipeline
- return unless pipeline.latest?
+ def commit
+ @commit ||= project.commit(origin_sha || origin_ref)
+ end
- MergeRequest.where(source_project: @pipeline.project, source_branch: @pipeline.ref)
- .update_all(head_pipeline_id: @pipeline.id)
+ def sha
+ commit.try(:id)
end
def cancel_pending_pipelines
@@ -69,5 +72,15 @@ module Ci
@pipeline_created_counter ||= Gitlab::Metrics
.counter(:pipelines_created_total, "Counter of pipelines created")
end
+
+ def schedule_head_pipeline_update
+ related_merge_requests.each do |merge_request|
+ UpdateHeadPipelineForMergeRequestWorker.perform_async(merge_request.id)
+ end
+ end
+
+ def related_merge_requests
+ MergeRequest.where(source_project: pipeline.project, source_branch: pipeline.ref)
+ end
end
end
diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb
index bf3d4855122..434dda89db0 100644
--- a/app/services/merge_requests/refresh_service.rb
+++ b/app/services/merge_requests/refresh_service.rb
@@ -76,6 +76,7 @@ module MergeRequests
end
merge_request.mark_as_unchecked
+ UpdateHeadPipelineForMergeRequestWorker.perform_async(merge_request.id)
end
end
diff --git a/app/workers/update_head_pipeline_for_merge_request_worker.rb b/app/workers/update_head_pipeline_for_merge_request_worker.rb
new file mode 100644
index 00000000000..ebe2435bbe5
--- /dev/null
+++ b/app/workers/update_head_pipeline_for_merge_request_worker.rb
@@ -0,0 +1,15 @@
+class UpdateHeadPipelineForMergeRequestWorker
+ include Sidekiq::Worker
+
+ sidekiq_options queue: 'pipeline_default'
+
+ def perform(merge_request_id)
+ merge_request = MergeRequest.find(merge_request_id)
+ pipeline = Ci::Pipeline.where(project: merge_request.source_project, ref: merge_request.source_branch).last
+
+ return unless pipeline && pipeline.latest?
+ raise ArgumentError, 'merge request sha does not equal pipeline sha' if merge_request.diff_head_sha != pipeline.sha
+
+ merge_request.update_attribute(:head_pipeline_id, pipeline.id)
+ end
+end
diff --git a/changelogs/unreleased/37354-pipelines-update.yml b/changelogs/unreleased/37354-pipelines-update.yml
new file mode 100644
index 00000000000..2b6ddfe95ed
--- /dev/null
+++ b/changelogs/unreleased/37354-pipelines-update.yml
@@ -0,0 +1,5 @@
+---
+title: Make sure head pippeline always corresponds with the head sha of an MR
+merge_request:
+author:
+type: fixed
diff --git a/spec/features/merge_requests/pipelines_spec.rb b/spec/features/merge_requests/pipelines_spec.rb
index a3fcc27cab0..307c860eac4 100644
--- a/spec/features/merge_requests/pipelines_spec.rb
+++ b/spec/features/merge_requests/pipelines_spec.rb
@@ -20,10 +20,14 @@ feature 'Pipelines for Merge Requests', :js do
end
before do
- visit project_merge_request_path(project, merge_request)
+ merge_request.update_attribute(:head_pipeline_id, pipeline.id)
end
scenario 'user visits merge request pipelines tab' do
+ visit project_merge_request_path(project, merge_request)
+
+ expect(page.find('.ci-widget')).to have_content('pending')
+
page.within('.merge-request-tabs') do
click_link('Pipelines')
end
@@ -31,6 +35,15 @@ feature 'Pipelines for Merge Requests', :js do
expect(page).to have_selector('.stage-cell')
end
+
+ scenario 'pipeline sha does not equal last commit sha' do
+ pipeline.update_attribute(:sha, '19e2e9b4ef76b422ce1154af39a91323ccc57434')
+ visit project_merge_request_path(project, merge_request)
+ wait_for_requests
+
+ expect(page.find('.ci-widget')).to have_content(
+ 'Could not connect to the CI server. Please check your settings and try again')
+ end
end
context 'without pipelines' do
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 728028746d8..e10f375d42b 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -828,20 +828,28 @@ describe MergeRequest do
end
describe '#head_pipeline' do
- describe 'when the source project exists' do
- it 'returns the latest pipeline' do
- pipeline = create(:ci_empty_pipeline, project: subject.source_project, ref: 'master', status: 'running', sha: "123abc", head_pipeline_of: subject)
+ before do
+ allow(subject).to receive(:diff_head_sha).and_return('lastsha')
+ end
- expect(subject.head_pipeline).to eq(pipeline)
- end
+ it 'returns nil for MR without head_pipeline_id' do
+ subject.update_attribute(:head_pipeline_id, nil)
+
+ expect(subject.head_pipeline).to be_nil
end
- describe 'when the source project does not exist' do
- it 'returns nil' do
- allow(subject).to receive(:source_project).and_return(nil)
+ it 'returns nil for MR with old pipeline' do
+ pipeline = create(:ci_empty_pipeline, sha: 'notlatestsha')
+ subject.update_attribute(:head_pipeline_id, pipeline.id)
- expect(subject.head_pipeline).to be_nil
- end
+ expect(subject.head_pipeline).to be_nil
+ end
+
+ it 'returns the pipeline for MR with recent pipeline' do
+ pipeline = create(:ci_empty_pipeline, sha: 'lastsha')
+ subject.update_attribute(:head_pipeline_id, pipeline.id)
+
+ expect(subject.head_pipeline).to eq(pipeline)
end
end
diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb
index befd0faf1b6..b0de8d447a2 100644
--- a/spec/services/ci/create_pipeline_service_spec.rb
+++ b/spec/services/ci/create_pipeline_service_spec.rb
@@ -57,19 +57,39 @@ describe Ci::CreatePipelineService do
end
context 'when merge requests already exist for this source branch' do
- it 'updates head pipeline of each merge request' do
- merge_request_1 = create(:merge_request, source_branch: 'master',
- target_branch: "branch_1",
- source_project: project)
+ let(:merge_request_1) do
+ create(:merge_request, source_branch: 'master', target_branch: "branch_1", source_project: project)
+ end
+ let(:merge_request_2) do
+ create(:merge_request, source_branch: 'master', target_branch: "branch_2", source_project: project)
+ end
- merge_request_2 = create(:merge_request, source_branch: 'master',
- target_branch: "branch_2",
- source_project: project)
+ context 'when the head pipeline sha equals merge request sha' do
+ it 'updates head pipeline of each merge request' do
+ merge_request_1
+ merge_request_2
- head_pipeline = execute_service
+ head_pipeline = execute_service
- expect(merge_request_1.reload.head_pipeline).to eq(head_pipeline)
- expect(merge_request_2.reload.head_pipeline).to eq(head_pipeline)
+ expect(merge_request_1.reload.head_pipeline).to eq(head_pipeline)
+ expect(merge_request_2.reload.head_pipeline).to eq(head_pipeline)
+ end
+ end
+
+ context 'when the head pipeline sha does not equal merge request sha' do
+ it 'raises the ArgumentError error from worker and does not update the head piepeline of MRs' do
+ merge_request_1
+ merge_request_2
+
+ allow_any_instance_of(Ci::Pipeline).to receive(:latest?).and_return(true)
+
+ expect { execute_service(after: 'ae73cb07c9eeaf35924a10f713b364d32b2dd34f') }.to raise_error(ArgumentError)
+
+ last_pipeline = Ci::Pipeline.last
+
+ expect(merge_request_1.reload.head_pipeline).not_to eq(last_pipeline)
+ expect(merge_request_2.reload.head_pipeline).not_to eq(last_pipeline)
+ end
end
context 'when there is no pipeline for source branch' do
@@ -106,8 +126,7 @@ describe Ci::CreatePipelineService do
target_branch: "branch_1",
source_project: project)
- allow_any_instance_of(Ci::Pipeline)
- .to receive(:latest?).and_return(false)
+ allow_any_instance_of(Ci::Pipeline).to receive(:latest?).and_return(false)
execute_service
diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb
index a2c05761f6b..61ec4709c59 100644
--- a/spec/services/merge_requests/refresh_service_spec.rb
+++ b/spec/services/merge_requests/refresh_service_spec.rb
@@ -74,6 +74,20 @@ describe MergeRequests::RefreshService do
end
end
+ context 'when pipeline exists for the source branch' do
+ let!(:pipeline) { create(:ci_empty_pipeline, ref: @merge_request.source_branch, project: @project, sha: @commits.first.sha)}
+
+ subject { service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/master') }
+
+ it 'updates the head_pipeline_id for @merge_request' do
+ expect { subject }.to change { @merge_request.reload.head_pipeline_id }.from(nil).to(pipeline.id)
+ end
+
+ it 'does not update the head_pipeline_id for @fork_merge_request' do
+ expect { subject }.not_to change { @fork_merge_request.reload.head_pipeline_id }
+ end
+ end
+
context 'push to origin repo source branch when an MR was reopened' do
let(:refresh_service) { service.new(@project, @user) }
diff --git a/spec/workers/update_head_pipeline_for_merge_request_worker_spec.rb b/spec/workers/update_head_pipeline_for_merge_request_worker_spec.rb
new file mode 100644
index 00000000000..522e1566271
--- /dev/null
+++ b/spec/workers/update_head_pipeline_for_merge_request_worker_spec.rb
@@ -0,0 +1,38 @@
+require 'spec_helper'
+
+describe UpdateHeadPipelineForMergeRequestWorker do
+ describe '#perform' do
+ let(:user) { create(:user) }
+ let(:project) { create(:project, :repository) }
+ let(:merge_request) { create(:merge_request, source_project: project) }
+ let(:latest_sha) { 'b83d6e391c22777fca1ed3012fce84f633d7fed0' }
+
+ context 'when pipeline exists for the source project and branch' do
+ before do
+ create(:ci_empty_pipeline, project: project, ref: merge_request.source_branch, sha: latest_sha)
+ end
+
+ it 'updates the head_pipeline_id of the merge_request' do
+ expect { subject.perform(merge_request.id) }.to change { merge_request.reload.head_pipeline_id }
+ end
+
+ context 'when merge request sha does not equal pipeline sha' do
+ before do
+ merge_request.merge_request_diff.update(head_commit_sha: 'different_sha')
+ end
+
+ it 'does not update head_pipeline_id' do
+ expect { subject.perform(merge_request.id) }.to raise_error(ArgumentError)
+
+ expect(merge_request.reload.head_pipeline_id).to eq(nil)
+ end
+ end
+ end
+
+ context 'when pipeline does not exist for the source project and branch' do
+ it 'does not update the head_pipeline_id of the merge_request' do
+ expect { subject.perform(merge_request.id) }.not_to change { merge_request.reload.head_pipeline_id }
+ end
+ end
+ end
+end