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

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

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)

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'

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

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