diff options
| -rw-r--r-- | CHANGELOG | 2 | ||||
| -rw-r--r-- | app/models/ci/build.rb | 13 | ||||
| -rw-r--r-- | app/models/commit_status.rb | 12 | ||||
| -rw-r--r-- | app/services/merge_requests/merge_when_build_succeeds_service.rb | 15 | ||||
| -rw-r--r-- | spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb | 66 |
5 files changed, 87 insertions, 21 deletions
diff --git a/CHANGELOG b/CHANGELOG index 8a95909eee9..b31e134918c 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -46,12 +46,14 @@ v 8.5.0 (unreleased) - Deprecate API "merge_request/:merge_request_id/comments". Use "merge_requests/:merge_request_id/notes" instead - Deprecate API "merge_request/:merge_request_id/...". Use "merge_requests/:merge_request_id/..." instead - Prevent parse error when name of project ends with .atom and prevent path issues + - Discover branches for commit statuses ref-less when doing merge when succeeded - Mark inline difference between old and new paths when a file is renamed - Support Akismet spam checking for creation of issues via API (Stan Hu) - API: Allow to set or update a merge-request's milestone (Kirill Skachkov) - Improve UI consistency between projects and groups lists - Add sort dropdown to dashboard projects page - Fixed logo animation on Safari (Roman Rott) + - Fix Merge When Succeeded when multiple stages - Hide remove source branch button when the MR is merged but new commits are pushed (Zeger-Jan van de Weg) - In seach autocomplete show only groups and projects you are member of - Don't process cross-reference notes from forks diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 5d800cd1f85..1227458e525 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -107,15 +107,18 @@ module Ci end state_machine :status, initial: :pending do - after_transition pending: :running do |build, transition| + after_transition pending: :running do |build| build.execute_hooks end - after_transition any => [:success, :failed, :canceled] do |build, transition| - return unless build.project + # We use around_transition to create builds for next stage as soon as possible, before the `after_*` is executed + around_transition any => [:success, :failed, :canceled] do |build, block| + block.call + build.commit.create_next_builds(build) if build.commit + end + after_transition any => [:success, :failed, :canceled] do |build| build.update_coverage - build.commit.create_next_builds(build) build.execute_hooks end end @@ -179,6 +182,7 @@ module Ci end def update_coverage + return unless project coverage_regex = project.build_coverage_regex return unless coverage_regex coverage = extract_coverage(trace, coverage_regex) @@ -334,6 +338,7 @@ module Ci end def execute_hooks + return unless project build_data = Gitlab::BuildDataBuilder.build(self) project.execute_hooks(build_data.dup, :build_hooks) project.execute_services(build_data.dup, :build_hooks) diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index 434b3560d09..7ef50836322 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -75,16 +75,16 @@ class CommitStatus < ActiveRecord::Base transition [:pending, :running] => :canceled end - after_transition pending: :running do |build, transition| - build.update_attributes started_at: Time.now + after_transition pending: :running do |commit_status| + commit_status.update_attributes started_at: Time.now end - after_transition any => [:success, :failed, :canceled] do |build, transition| - build.update_attributes finished_at: Time.now + after_transition any => [:success, :failed, :canceled] do |commit_status| + commit_status.update_attributes finished_at: Time.now end - after_transition [:pending, :running] => :success do |build, transition| - MergeRequests::MergeWhenBuildSucceedsService.new(build.commit.project, nil).trigger(build) + after_transition [:pending, :running] => :success do |commit_status| + MergeRequests::MergeWhenBuildSucceedsService.new(commit_status.commit.project, nil).trigger(commit_status) end state :pending, value: 'pending' diff --git a/app/services/merge_requests/merge_when_build_succeeds_service.rb b/app/services/merge_requests/merge_when_build_succeeds_service.rb index 5cf7404a493..531bbc9b067 100644 --- a/app/services/merge_requests/merge_when_build_succeeds_service.rb +++ b/app/services/merge_requests/merge_when_build_succeeds_service.rb @@ -19,8 +19,8 @@ module MergeRequests end # Triggers the automatic merge of merge_request once the build succeeds - def trigger(build) - merge_requests = merge_request_from(build) + def trigger(commit_status) + merge_requests = merge_request_from(commit_status) merge_requests.each do |merge_request| next unless merge_request.merge_when_build_succeeds? @@ -45,9 +45,14 @@ module MergeRequests private - def merge_request_from(build) - merge_requests = @project.origin_merge_requests.opened.where(source_branch: build.ref).to_a - merge_requests += @project.fork_merge_requests.opened.where(source_branch: build.ref).to_a + def merge_request_from(commit_status) + branches = commit_status.ref + + # This is for ref-less builds + branches ||= @project.repository.branch_names_contains(commit_status.sha) + + merge_requests = @project.origin_merge_requests.opened.where(source_branch: branches).to_a + merge_requests += @project.fork_merge_requests.opened.where(source_branch: branches).to_a merge_requests.uniq.select(&:source_project) end diff --git a/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb b/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb index de9fed2b7dd..f285517cdac 100644 --- a/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb +++ b/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb @@ -54,14 +54,68 @@ describe MergeRequests::MergeWhenBuildSucceedsService do end describe "#trigger" do - let(:build) { create(:ci_build, ref: mr_merge_if_green_enabled.source_branch, status: "success") } + context 'build with ref' do + let(:build) { create(:ci_build, ref: mr_merge_if_green_enabled.source_branch, status: "success") } - it "merges all merge requests with merge when build succeeds enabled" do - allow_any_instance_of(MergeRequest).to receive(:ci_commit).and_return(ci_commit) - allow(ci_commit).to receive(:success?).and_return(true) + it "merges all merge requests with merge when build succeeds enabled" do + allow_any_instance_of(MergeRequest).to receive(:ci_commit).and_return(ci_commit) + allow(ci_commit).to receive(:success?).and_return(true) + + expect(MergeWorker).to receive(:perform_async) + service.trigger(build) + end + end + + context 'commit status without ref' do + let(:commit_status) { create(:generic_commit_status, status: 'success') } + + it "doesn't merge a requests for status on other branch" do + allow(project.repository).to receive(:branch_names_contains).with(commit_status.sha).and_return([]) + + expect(MergeWorker).to_not receive(:perform_async) + service.trigger(commit_status) + end + + it 'discovers branches and merges all merge requests when status is success' do + allow(project.repository).to receive(:branch_names_contains). + with(commit_status.sha).and_return([mr_merge_if_green_enabled.source_branch]) + allow(ci_commit).to receive(:success?).and_return(true) + allow_any_instance_of(MergeRequest).to receive(:ci_commit).and_return(ci_commit) + allow(ci_commit).to receive(:success?).and_return(true) - expect(MergeWorker).to receive(:perform_async) - service.trigger(build) + expect(MergeWorker).to receive(:perform_async) + service.trigger(commit_status) + end + end + + context 'properly handles multiple stages' do + let(:ref) { mr_merge_if_green_enabled.source_branch } + let(:build) { create(:ci_build, commit: ci_commit, ref: ref, name: 'build', stage: 'build') } + let(:test) { create(:ci_build, commit: ci_commit, ref: ref, name: 'test', stage: 'test') } + + before do + # This behavior of MergeRequest: we instantiate a new object + allow_any_instance_of(MergeRequest).to receive(:ci_commit).and_wrap_original do + Ci::Commit.find(ci_commit.id) + end + + # We create test after the build + allow(ci_commit).to receive(:create_next_builds).and_wrap_original do + test + end + end + + it "doesn't merge if some stages failed" do + expect(MergeWorker).to_not receive(:perform_async) + build.success + test.drop + end + + it 'merge when all stages succeeded' do + expect(MergeWorker).to receive(:perform_async) + build.success + test.success + end end end |