diff options
| -rw-r--r-- | app/models/merge_request.rb | 8 | ||||
| -rw-r--r-- | app/services/ci/create_pipeline_service.rb | 23 | ||||
| -rw-r--r-- | app/services/merge_requests/refresh_service.rb | 1 | ||||
| -rw-r--r-- | app/workers/update_head_pipeline_for_merge_request_worker.rb | 15 | ||||
| -rw-r--r-- | changelogs/unreleased/37354-pipelines-update.yml | 5 | ||||
| -rw-r--r-- | spec/features/merge_requests/pipelines_spec.rb | 15 | ||||
| -rw-r--r-- | spec/models/merge_request_spec.rb | 28 | ||||
| -rw-r--r-- | spec/services/ci/create_pipeline_service_spec.rb | 43 | ||||
| -rw-r--r-- | spec/services/merge_requests/refresh_service_spec.rb | 14 | ||||
| -rw-r--r-- | spec/workers/update_head_pipeline_for_merge_request_worker_spec.rb | 38 |
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 |