diff options
author | Douwe Maan <douwe@gitlab.com> | 2016-02-22 03:38:51 +0300 |
---|---|---|
committer | Rémy Coutable <remy@rymai.me> | 2016-02-22 10:47:20 +0300 |
commit | b947661ce636708f9ba3818e7d358d0575d73b82 (patch) | |
tree | 620752404e3a265cb954a714f824b32491507055 | |
parent | 0e41e6571ca8320edcd3bfb61a5df61bc9043c8f (diff) |
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
-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 |