From fa73f4ee196b8c9d28c3b0b035acdd71d71dadb3 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 3 Apr 2019 16:44:51 +0700 Subject: [PATCH] Fix merge requst relationships with pipeline in MWPSService MWPSService currently uses the old pipeline lookup method. It searches related merge requests with pipeline.ref, however, this doesn't work for attached/detached merge request pipelines. --- .../add_todo_when_build_fails_service.rb | 4 +-- app/services/merge_requests/base_service.rb | 13 +------- ...equest-relations-with-pipeline-on-mwps.yml | 5 +++ spec/models/ci/build_spec.rb | 25 +++++++++++++-- .../add_todo_when_build_fails_service_spec.rb | 32 +++++++++++++++++++ ...rge_when_pipeline_succeeds_service_spec.rb | 15 +++++++++ 6 files changed, 77 insertions(+), 17 deletions(-) create mode 100644 changelogs/unreleased/fix-merge-request-relations-with-pipeline-on-mwps.yml diff --git a/app/services/merge_requests/add_todo_when_build_fails_service.rb b/app/services/merge_requests/add_todo_when_build_fails_service.rb index 79c43b8e7d5..d3ef892875b 100644 --- a/app/services/merge_requests/add_todo_when_build_fails_service.rb +++ b/app/services/merge_requests/add_todo_when_build_fails_service.rb @@ -7,7 +7,7 @@ module MergeRequests def execute(commit_status) return if commit_status.allow_failure? || commit_status.retried? - commit_status_merge_requests(commit_status) do |merge_request| + pipeline_merge_requests(commit_status.pipeline) do |merge_request| todo_service.merge_request_build_failed(merge_request) end end @@ -16,7 +16,7 @@ module MergeRequests # build is retried # def close(commit_status) - commit_status_merge_requests(commit_status) do |merge_request| + pipeline_merge_requests(commit_status.pipeline) do |merge_request| todo_service.merge_request_build_retried(merge_request) end end diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb index f968e3693da..8a9e5ebb014 100644 --- a/app/services/merge_requests/base_service.rb +++ b/app/services/merge_requests/base_service.rb @@ -99,22 +99,11 @@ module MergeRequests # rubocop: enable CodeReuse/ActiveRecord def pipeline_merge_requests(pipeline) - merge_requests_for(pipeline.ref).each do |merge_request| + pipeline.all_merge_requests.opened.each do |merge_request| next unless pipeline == merge_request.head_pipeline yield merge_request end end - - def commit_status_merge_requests(commit_status) - merge_requests_for(commit_status.ref).each do |merge_request| - pipeline = merge_request.head_pipeline - - next unless pipeline - next unless pipeline.sha == commit_status.sha - - yield merge_request - end - end end end diff --git a/changelogs/unreleased/fix-merge-request-relations-with-pipeline-on-mwps.yml b/changelogs/unreleased/fix-merge-request-relations-with-pipeline-on-mwps.yml new file mode 100644 index 00000000000..9ccc79109d8 --- /dev/null +++ b/changelogs/unreleased/fix-merge-request-relations-with-pipeline-on-mwps.yml @@ -0,0 +1,5 @@ +--- +title: Fix MWPS does not work for merge request pipelines +merge_request: 26906 +author: +type: fixed diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 697fe3fda06..d0d43ee4462 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -3216,7 +3216,7 @@ describe Ci::Build do it 'does not try to create a todo' do project.add_developer(user) - expect(service).not_to receive(:commit_status_merge_requests) + expect(service).not_to receive(:pipeline_merge_requests) subject.drop! end @@ -3252,7 +3252,23 @@ describe Ci::Build do end context 'when build is not configured to be retried' do - subject { create(:ci_build, :running, project: project, user: user) } + subject { create(:ci_build, :running, project: project, user: user, pipeline: pipeline) } + + let(:pipeline) do + create(:ci_pipeline, + project: project, + ref: 'feature', + sha: merge_request.diff_head_sha, + merge_requests_as_head_pipeline: [merge_request]) + end + + let(:merge_request) do + create(:merge_request, :opened, + source_branch: 'feature', + source_project: project, + target_branch: 'master', + target_project: project) + end it 'does not retry build' do expect(described_class).not_to receive(:retry) @@ -3271,7 +3287,10 @@ describe Ci::Build do it 'creates a todo' do project.add_developer(user) - expect(service).to receive(:commit_status_merge_requests) + expect_next_instance_of(TodoService) do |todo_service| + expect(todo_service) + .to receive(:merge_request_build_failed).with(merge_request) + end subject.drop! end diff --git a/spec/services/merge_requests/add_todo_when_build_fails_service_spec.rb b/spec/services/merge_requests/add_todo_when_build_fails_service_spec.rb index af0a214c00f..39a2ef579dd 100644 --- a/spec/services/merge_requests/add_todo_when_build_fails_service_spec.rb +++ b/spec/services/merge_requests/add_todo_when_build_fails_service_spec.rb @@ -77,6 +77,22 @@ describe MergeRequests::AddTodoWhenBuildFailsService do service.execute(commit_status) end end + + context 'when build belongs to a merge request pipeline' do + let(:pipeline) do + create(:ci_pipeline, source: :merge_request_event, + ref: merge_request.merge_ref_path, + merge_request: merge_request, + merge_requests_as_head_pipeline: [merge_request]) + end + + let(:commit_status) { create(:ci_build, ref: merge_request.merge_ref_path, pipeline: pipeline) } + + it 'notifies the todo service' do + expect(todo_service).to receive(:merge_request_build_failed).with(merge_request) + service.execute(commit_status) + end + end end describe '#close' do @@ -106,6 +122,22 @@ describe MergeRequests::AddTodoWhenBuildFailsService do service.close(commit_status) end end + + context 'when build belongs to a merge request pipeline' do + let(:pipeline) do + create(:ci_pipeline, source: :merge_request_event, + ref: merge_request.merge_ref_path, + merge_request: merge_request, + merge_requests_as_head_pipeline: [merge_request]) + end + + let(:commit_status) { create(:ci_build, ref: merge_request.merge_ref_path, pipeline: pipeline) } + + it 'notifies the todo service' do + expect(todo_service).to receive(:merge_request_build_retried).with(merge_request) + service.close(commit_status) + end + end end describe '#close_all' do diff --git a/spec/services/merge_requests/merge_when_pipeline_succeeds_service_spec.rb b/spec/services/merge_requests/merge_when_pipeline_succeeds_service_spec.rb index 52bbd4e794d..1c709df58b6 100644 --- a/spec/services/merge_requests/merge_when_pipeline_succeeds_service_spec.rb +++ b/spec/services/merge_requests/merge_when_pipeline_succeeds_service_spec.rb @@ -112,6 +112,21 @@ describe MergeRequests::MergeWhenPipelineSucceedsService do service.trigger(unrelated_pipeline) end end + + context 'when pipeline is merge request pipeline' do + let(:pipeline) do + create(:ci_pipeline, :success, + source: :merge_request_event, + ref: mr_merge_if_green_enabled.merge_ref_path, + merge_request: mr_merge_if_green_enabled, + merge_requests_as_head_pipeline: [mr_merge_if_green_enabled]) + end + + it 'merges the associated merge request' do + expect(MergeWorker).to receive(:perform_async) + service.trigger(pipeline) + end + end end describe "#cancel" do