From 0d9b801a5ff9d98948954da61569668a57dde99a Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 2 Jan 2019 15:39:45 +0900 Subject: Refactor the logic of updating head pipelines Sort out some logic --- app/models/ci/pipeline.rb | 12 +++++++++ app/models/merge_request.rb | 18 +++++++++++--- app/services/merge_requests/create_service.rb | 16 +----------- ...pdate_head_pipeline_for_merge_request_worker.rb | 21 ++-------------- .../user-update-head-pipeline-worker.yml | 5 ++++ spec/models/merge_request_spec.rb | 28 +++++++++++++++++++++ spec/services/ci/create_pipeline_service_spec.rb | 3 ++- .../services/merge_requests/create_service_spec.rb | 29 ++++++++++++++++------ ..._head_pipeline_for_merge_request_worker_spec.rb | 10 ++++---- 9 files changed, 90 insertions(+), 52 deletions(-) create mode 100644 changelogs/unreleased/user-update-head-pipeline-worker.yml diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 1f5017cc3c3..8b95e83635b 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -178,6 +178,14 @@ module Ci scope :for_user, -> (user) { where(user: user) } + scope :for_merge_request, -> (merge_request, ref, sha) do + ## + # We have to filter out unrelated MR pipelines, in case, + # there are two merge requests from the same source branch + where(merge_request: [nil, merge_request], ref: ref, sha: sha) + .sort_by_merge_request_pipelines + end + # Returns the pipelines in descending order (= newest first), optionally # limited to a number of references. # @@ -265,6 +273,10 @@ 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 + 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 6092c56b925..ebd1f22ebc7 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -1092,10 +1092,15 @@ class MergeRequest < ActiveRecord::Base def all_pipelines(shas: all_commit_shas) return Ci::Pipeline.none unless source_project - @all_pipelines ||= source_project.ci_pipelines - .where(sha: shas, ref: source_branch) - .where(merge_request: [nil, self]) - .sort_by_merge_request_pipelines + @all_pipelines ||= + source_project.ci_pipelines + .for_merge_request(self, source_branch, all_commit_shas) + end + + def update_head_pipeline + self.head_pipeline = find_actual_head_pipeline + + update_column(:head_pipeline_id, head_pipeline.id) if head_pipeline_id_changed? end def merge_request_pipeline_exists? @@ -1295,6 +1300,11 @@ class MergeRequest < ActiveRecord::Base .find_by(sha: diff_base_sha) end + def find_actual_head_pipeline + source_project&.ci_pipelines + &.latest_for_merge_request(self, source_branch, diff_head_sha) + end + def discussions_rendered_on_frontend? true end diff --git a/app/services/merge_requests/create_service.rb b/app/services/merge_requests/create_service.rb index 7bb9fa60515..02c2388c05c 100644 --- a/app/services/merge_requests/create_service.rb +++ b/app/services/merge_requests/create_service.rb @@ -26,7 +26,7 @@ module MergeRequests todo_service.new_merge_request(issuable, current_user) issuable.cache_merge_request_closes_issues!(current_user) create_merge_request_pipeline(issuable, current_user) - update_merge_requests_head_pipeline(issuable) + issuable.update_head_pipeline super end @@ -45,20 +45,6 @@ module MergeRequests private - def update_merge_requests_head_pipeline(merge_request) - pipeline = head_pipeline_for(merge_request) - merge_request.update(head_pipeline_id: pipeline.id) if pipeline - end - - def head_pipeline_for(merge_request) - return unless merge_request.source_project - - sha = merge_request.source_branch_sha - return unless sha - - merge_request.all_pipelines(shas: sha).first - end - def set_projects! # @project is used to determine whether the user can set the merge request's # assignee, milestone and labels. Whether they can depends on their diff --git a/app/workers/update_head_pipeline_for_merge_request_worker.rb b/app/workers/update_head_pipeline_for_merge_request_worker.rb index e8494ffa002..4ec2b9d8fbe 100644 --- a/app/workers/update_head_pipeline_for_merge_request_worker.rb +++ b/app/workers/update_head_pipeline_for_merge_request_worker.rb @@ -7,25 +7,8 @@ class UpdateHeadPipelineForMergeRequestWorker queue_namespace :pipeline_processing def perform(merge_request_id) - merge_request = MergeRequest.find(merge_request_id) - - sha = merge_request.diff_head_sha - pipeline = merge_request.all_pipelines(shas: sha).first - - return unless pipeline && pipeline.latest? - - if merge_request.diff_head_sha != pipeline.sha - log_error_message_for(merge_request) - - return + MergeRequest.find_by_id(merge_request_id).try do |merge_request| + merge_request.update_head_pipeline end - - merge_request.update_attribute(:head_pipeline_id, pipeline.id) - end - - def log_error_message_for(merge_request) - Rails.logger.error( - "Outdated head pipeline for active merge request: id=#{merge_request.id}, source_branch=#{merge_request.source_branch}, diff_head_sha=#{merge_request.diff_head_sha}" - ) end end diff --git a/changelogs/unreleased/user-update-head-pipeline-worker.yml b/changelogs/unreleased/user-update-head-pipeline-worker.yml new file mode 100644 index 00000000000..2c8088dd78a --- /dev/null +++ b/changelogs/unreleased/user-update-head-pipeline-worker.yml @@ -0,0 +1,5 @@ +--- +title: Refactor the logic of updating head pipelines for merge requests +merge_request: 23437 +author: +type: other diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 4cc3a6a3644..96d49e86dab 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -1372,6 +1372,34 @@ describe MergeRequest do end end + describe '#update_head_pipeline' do + subject { merge_request.update_head_pipeline } + + let(:merge_request) { create(:merge_request) } + + context 'when there is a pipeline with the diff head sha' do + let!(:pipeline) do + create(:ci_empty_pipeline, + project: merge_request.project, + sha: merge_request.diff_head_sha, + ref: merge_request.source_branch) + end + + it 'updates the head pipeline' 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 } + .not_to change { merge_request.reload.head_pipeline } + end + end + end + describe '#has_test_reports?' do subject { merge_request.has_test_reports? } diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index 87b60387c52..8497e90bd8b 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -143,7 +143,8 @@ 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(MergeRequest) + .to receive(:find_actual_head_pipeline) { } execute_service diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb index 5a3ecb1019b..308f99dc0da 100644 --- a/spec/services/merge_requests/create_service_spec.rb +++ b/spec/services/merge_requests/create_service_spec.rb @@ -128,9 +128,9 @@ describe MergeRequests::CreateService do end context 'when head pipelines already exist for merge request source branch' do - let(:sha) { project.commit(opts[:source_branch]).id } - let!(:pipeline_1) { create(:ci_pipeline, project: project, ref: opts[:source_branch], project_id: project.id, sha: sha) } - let!(:pipeline_2) { create(:ci_pipeline, project: project, ref: opts[:source_branch], project_id: project.id, sha: sha) } + let(:shas) { project.repository.commits(opts[:source_branch], limit: 2).map(&:id) } + let!(:pipeline_1) { create(:ci_pipeline, project: project, ref: opts[:source_branch], project_id: project.id, sha: shas[1]) } + let!(:pipeline_2) { create(:ci_pipeline, project: project, ref: opts[:source_branch], project_id: project.id, sha: shas[0]) } let!(:pipeline_3) { create(:ci_pipeline, project: project, ref: "other_branch", project_id: project.id) } before do @@ -144,17 +144,30 @@ describe MergeRequests::CreateService do it 'sets head pipeline' do merge_request = service.execute - expect(merge_request.head_pipeline).to eq(pipeline_2) + expect(merge_request.reload.head_pipeline).to eq(pipeline_2) expect(merge_request).to be_persisted end - context 'when merge request head commit sha does not match pipeline sha' do - it 'sets the head pipeline correctly' do - pipeline_2.update(sha: 1234) + context 'when the new pipeline is associated with an old sha' do + let!(:pipeline_1) { create(:ci_pipeline, project: project, ref: opts[:source_branch], project_id: project.id, sha: shas[0]) } + let!(:pipeline_2) { create(:ci_pipeline, project: project, ref: opts[:source_branch], project_id: project.id, sha: shas[1]) } + it 'sets an old pipeline with associated with the latest sha as the head pipeline' do merge_request = service.execute - expect(merge_request.head_pipeline).to eq(pipeline_1) + expect(merge_request.reload.head_pipeline).to eq(pipeline_1) + expect(merge_request).to be_persisted + end + end + + context 'when there are no pipelines with the diff head sha' do + let!(:pipeline_1) { create(:ci_pipeline, project: project, ref: opts[:source_branch], project_id: project.id, sha: shas[1]) } + let!(:pipeline_2) { create(:ci_pipeline, project: project, ref: opts[:source_branch], project_id: project.id, sha: shas[1]) } + + it 'does not set the head pipeline' do + merge_request = service.execute + + expect(merge_request.reload.head_pipeline).to be_nil expect(merge_request).to be_persisted end end 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 index a2bc264b0f6..963237ceadf 100644 --- a/spec/workers/update_head_pipeline_for_merge_request_worker_spec.rb +++ b/spec/workers/update_head_pipeline_for_merge_request_worker_spec.rb @@ -21,17 +21,17 @@ describe UpdateHeadPipelineForMergeRequestWorker 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) }.not_to raise_error - - expect(merge_request.reload.head_pipeline_id).to eq(nil) + it 'does not update head pipeline' do + expect { subject.perform(merge_request.id) } + .not_to change { merge_request.reload.head_pipeline_id } 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 } + expect { subject.perform(merge_request.id) } + .not_to change { merge_request.reload.head_pipeline_id } end end -- cgit v1.2.1 From b6a9dce14e340bcc8dbd41fb6ba97b30b37e8657 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 2 Jan 2019 15:51:59 +0900 Subject: Make find_actual_head_pipeline private method a --- app/models/merge_request.rb | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index ebd1f22ebc7..613860ec31a 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -1300,11 +1300,6 @@ class MergeRequest < ActiveRecord::Base .find_by(sha: diff_base_sha) end - def find_actual_head_pipeline - source_project&.ci_pipelines - &.latest_for_merge_request(self, source_branch, diff_head_sha) - end - def discussions_rendered_on_frontend? true end @@ -1348,4 +1343,11 @@ class MergeRequest < ActiveRecord::Base source_project.repository.squash_in_progress?(id) end + + private + + def find_actual_head_pipeline + source_project&.ci_pipelines + &.latest_for_merge_request(self, source_branch, diff_head_sha) + end end -- cgit v1.2.1 From c6b7954bca9562d42797b8f9f5c3da6f4657cccc Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 4 Jan 2019 19:39:26 +0900 Subject: Update changelog number --- changelogs/unreleased/user-update-head-pipeline-worker.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelogs/unreleased/user-update-head-pipeline-worker.yml b/changelogs/unreleased/user-update-head-pipeline-worker.yml index 2c8088dd78a..fd88697f239 100644 --- a/changelogs/unreleased/user-update-head-pipeline-worker.yml +++ b/changelogs/unreleased/user-update-head-pipeline-worker.yml @@ -1,5 +1,5 @@ --- title: Refactor the logic of updating head pipelines for merge requests -merge_request: 23437 +merge_request: 23502 author: type: other -- cgit v1.2.1 From 4ab0b33db6f328fc68394fd3af992052f883401e Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 4 Jan 2019 19:47:39 +0900 Subject: Clarify comments about for_merge_request --- app/models/ci/pipeline.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 8b95e83635b..5b446e649a9 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -180,8 +180,9 @@ module Ci scope :for_merge_request, -> (merge_request, ref, sha) do ## - # We have to filter out unrelated MR pipelines, in case, - # there are two merge requests from the same source branch + # 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 -- cgit v1.2.1