Commit b947661c authored by Douwe Maan's avatar Douwe Maan Committed by Rémy Coutable

Merge branch 'merge-when-succeeded' into 'master'

Fix bugs in MergeWhenSucceeded

1. This fixes support for merge when succeeded for statuses without ref.
2. This fixes support for merge when succeeded for multiple stages. Stages are created after all builds for previous one are finished.

Fixes: https://gitlab.com/gitlab-org/gitlab-ce/issues/9060 https://gitlab.com/gitlab-org/gitlab-ce/issues/8108 https://gitlab.com/gitlab-org/gitlab-ce/issues/12931 https://gitlab.com/gitlab-org/gitlab-ce/issues/13269

/cc @grzesiek @DouweM @rspeicher 


See merge request !2894
parent 0e41e657
...@@ -46,12 +46,14 @@ v 8.5.0 (unreleased) ...@@ -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/comments". Use "merge_requests/:merge_request_id/notes" instead
- Deprecate API "merge_request/:merge_request_id/...". Use "merge_requests/:merge_request_id/..." 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 - 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 - 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) - 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) - API: Allow to set or update a merge-request's milestone (Kirill Skachkov)
- Improve UI consistency between projects and groups lists - Improve UI consistency between projects and groups lists
- Add sort dropdown to dashboard projects page - Add sort dropdown to dashboard projects page
- Fixed logo animation on Safari (Roman Rott) - 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) - 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 - In seach autocomplete show only groups and projects you are member of
- Don't process cross-reference notes from forks - Don't process cross-reference notes from forks
......
...@@ -107,15 +107,18 @@ module Ci ...@@ -107,15 +107,18 @@ module Ci
end end
state_machine :status, initial: :pending do state_machine :status, initial: :pending do
after_transition pending: :running do |build, transition| after_transition pending: :running do |build|
build.execute_hooks build.execute_hooks
end end
after_transition any => [:success, :failed, :canceled] do |build, transition| # We use around_transition to create builds for next stage as soon as possible, before the `after_*` is executed
return unless build.project 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.update_coverage
build.commit.create_next_builds(build)
build.execute_hooks build.execute_hooks
end end
end end
...@@ -179,6 +182,7 @@ module Ci ...@@ -179,6 +182,7 @@ module Ci
end end
def update_coverage def update_coverage
return unless project
coverage_regex = project.build_coverage_regex coverage_regex = project.build_coverage_regex
return unless coverage_regex return unless coverage_regex
coverage = extract_coverage(trace, coverage_regex) coverage = extract_coverage(trace, coverage_regex)
...@@ -334,6 +338,7 @@ module Ci ...@@ -334,6 +338,7 @@ module Ci
end end
def execute_hooks def execute_hooks
return unless project
build_data = Gitlab::BuildDataBuilder.build(self) build_data = Gitlab::BuildDataBuilder.build(self)
project.execute_hooks(build_data.dup, :build_hooks) project.execute_hooks(build_data.dup, :build_hooks)
project.execute_services(build_data.dup, :build_hooks) project.execute_services(build_data.dup, :build_hooks)
......
...@@ -75,16 +75,16 @@ class CommitStatus < ActiveRecord::Base ...@@ -75,16 +75,16 @@ class CommitStatus < ActiveRecord::Base
transition [:pending, :running] => :canceled transition [:pending, :running] => :canceled
end end
after_transition pending: :running do |build, transition| after_transition pending: :running do |commit_status|
build.update_attributes started_at: Time.now commit_status.update_attributes started_at: Time.now
end end
after_transition any => [:success, :failed, :canceled] do |build, transition| after_transition any => [:success, :failed, :canceled] do |commit_status|
build.update_attributes finished_at: Time.now commit_status.update_attributes finished_at: Time.now
end end
after_transition [:pending, :running] => :success do |build, transition| after_transition [:pending, :running] => :success do |commit_status|
MergeRequests::MergeWhenBuildSucceedsService.new(build.commit.project, nil).trigger(build) MergeRequests::MergeWhenBuildSucceedsService.new(commit_status.commit.project, nil).trigger(commit_status)
end end
state :pending, value: 'pending' state :pending, value: 'pending'
......
...@@ -19,8 +19,8 @@ module MergeRequests ...@@ -19,8 +19,8 @@ module MergeRequests
end end
# Triggers the automatic merge of merge_request once the build succeeds # Triggers the automatic merge of merge_request once the build succeeds
def trigger(build) def trigger(commit_status)
merge_requests = merge_request_from(build) merge_requests = merge_request_from(commit_status)
merge_requests.each do |merge_request| merge_requests.each do |merge_request|
next unless merge_request.merge_when_build_succeeds? next unless merge_request.merge_when_build_succeeds?
...@@ -45,9 +45,14 @@ module MergeRequests ...@@ -45,9 +45,14 @@ module MergeRequests
private private
def merge_request_from(build) def merge_request_from(commit_status)
merge_requests = @project.origin_merge_requests.opened.where(source_branch: build.ref).to_a branches = commit_status.ref
merge_requests += @project.fork_merge_requests.opened.where(source_branch: build.ref).to_a
# 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) merge_requests.uniq.select(&:source_project)
end end
......
...@@ -54,6 +54,7 @@ describe MergeRequests::MergeWhenBuildSucceedsService do ...@@ -54,6 +54,7 @@ describe MergeRequests::MergeWhenBuildSucceedsService do
end end
describe "#trigger" do describe "#trigger" do
context 'build with ref' do
let(:build) { create(:ci_build, ref: mr_merge_if_green_enabled.source_branch, status: "success") } 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 it "merges all merge requests with merge when build succeeds enabled" do
...@@ -65,6 +66,59 @@ describe MergeRequests::MergeWhenBuildSucceedsService do ...@@ -65,6 +66,59 @@ describe MergeRequests::MergeWhenBuildSucceedsService do
end end
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(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
describe "#cancel" do describe "#cancel" do
before do before do
service.cancel(mr_merge_if_green_enabled) service.cancel(mr_merge_if_green_enabled)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment