Merge branch 'do-not-abort-merge-trains-on-ff' into 'master'

Abort only MWPS when FF only merge is impossible

Closes #34117

See merge request gitlab-org/gitlab!18591

app/models/merge_request.rb

@@ -209,14 +209,20 @@ class MergeRequest < ApplicationRecord
   scope :by_target_branch, ->(branch_name) { where(target_branch: branch_name) }
   scope :preload_source_project, -> { preload(:source_project) }
 
-  scope :with_open_merge_when_pipeline_succeeds, -> do
-    with_state(:opened).where(merge_when_pipeline_succeeds: true)
+  scope :with_auto_merge_enabled, -> do
+    with_state(:opened).where(auto_merge_enabled: true)
   end
 
   after_save :keep_around_commit
 
   alias_attribute :project, :target_project
   alias_attribute :project_id, :target_project_id
+
+  # Currently, `merge_when_pipeline_succeeds` column is used as a flag
+  # to check if _any_ auto merge strategy is activated on the merge request.
+  # Today, we have multiple strategies and MWPS is one of them.
+  # we'd eventually rename the column for avoiding confusions, but in the mean time
+  # please use `auto_merge_enabled` alias instead of `merge_when_pipeline_succeeds`.
   alias_attribute :auto_merge_enabled, :merge_when_pipeline_succeeds
   alias_method :issuing_parent, :target_project
 

app/services/merge_requests/refresh_service.rb

@@ -152,7 +152,8 @@ module MergeRequests
     def abort_ff_merge_requests_with_when_pipeline_succeeds
       return unless @project.ff_merge_must_be_possible?
 
-      requests_with_auto_merge_enabled_to(@push.branch_name).each do |merge_request|
+      merge_requests_with_auto_merge_enabled_to(@push.branch_name).each do |merge_request|
+        next unless merge_request.auto_merge_strategy == AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS
         next unless merge_request.should_be_rebased?
 
         abort_auto_merge_with_todo(merge_request, 'target branch was updated')
@@ -167,11 +168,11 @@ module MergeRequests
       todo_service.merge_request_became_unmergeable(merge_request)
     end
 
-    def requests_with_auto_merge_enabled_to(target_branch)
+    def merge_requests_with_auto_merge_enabled_to(target_branch)
       @project
         .merge_requests
         .by_target_branch(target_branch)
-        .with_open_merge_when_pipeline_succeeds
+        .with_auto_merge_enabled
     end
 
     def mark_pending_todos_done

changelogs/unreleased/do-not-abort-merge-trains-on-ff.yml

+---
+title: Abort only MWPS when FF only merge is impossible
+merge_request: 18591
+author:
+type: fixed

db/migrate/20191023132005_add_merge_requests_index_on_target_project_and_branch.rb

+# frozen_string_literal: true
+
+class AddMergeRequestsIndexOnTargetProjectAndBranch < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :merge_requests, [:target_project_id, :target_branch],
+      where: "state_id = 1 AND merge_when_pipeline_succeeds = true"
+  end
+
+  def down
+    remove_concurrent_index :merge_requests, [:target_project_id, :target_branch]
+  end
+end

db/schema.rb

@@ -2342,6 +2342,7 @@ ActiveRecord::Schema.define(version: 2019_10_26_041447) do
     t.index ["target_project_id", "iid"], name: "index_merge_requests_on_target_project_id_and_iid", unique: true
     t.index ["target_project_id", "iid"], name: "index_merge_requests_on_target_project_id_and_iid_opened", where: "((state)::text = 'opened'::text)"
     t.index ["target_project_id", "merge_commit_sha", "id"], name: "index_merge_requests_on_tp_id_and_merge_commit_sha_and_id"
+    t.index ["target_project_id", "target_branch"], name: "index_merge_requests_on_target_project_id_and_target_branch", where: "((state_id = 1) AND (merge_when_pipeline_succeeds = true))"
     t.index ["title"], name: "index_merge_requests_on_title"
     t.index ["title"], name: "index_merge_requests_on_title_trigram", opclass: :gin_trgm_ops, using: :gin
     t.index ["updated_by_id"], name: "index_merge_requests_on_updated_by_id", where: "(updated_by_id IS NOT NULL)"

ee/spec/services/ee/merge_requests/refresh_service_spec.rb

@@ -4,6 +4,7 @@ require 'spec_helper'
 
 describe MergeRequests::RefreshService do
   include ProjectForksHelper
+  include ProjectHelpers
 
   let(:group) { create(:group) }
   let(:project) { create(:project, :repository, namespace: group, approvals_before_merge: 1, reset_approvals_on_push: true) }
@@ -147,4 +148,59 @@ describe MergeRequests::RefreshService do
       end
     end
   end
+
+  describe '#abort_ff_merge_requests_with_when_pipeline_succeeds' do
+    let_it_be(:project) { create(:project, :repository, merge_method: 'ff') }
+    let_it_be(:author) { create_user_from_membership(project, :developer) }
+    let_it_be(:user) { create(:user) }
+
+    let_it_be(:merge_request, refind: true) do
+      create(:merge_request,
+             author: author,
+             source_project: project,
+             source_branch: 'feature',
+             target_branch: 'master',
+             target_project: project,
+             auto_merge_enabled: true,
+             merge_user: user)
+    end
+
+    let_it_be(:newrev) do
+      project
+        .repository
+        .create_file(user, 'test1.txt', 'Test data',
+                     message: 'Test commit', branch_name: 'master')
+    end
+
+    let_it_be(:oldrev) do
+      project
+        .repository
+        .commit(newrev)
+        .parent_id
+    end
+
+    let(:refresh_service) { described_class.new(project, user) }
+
+    before do
+      merge_request.auto_merge_strategy = auto_merge_strategy
+      merge_request.save!
+
+      refresh_service.execute(oldrev, newrev, 'refs/heads/master')
+      merge_request.reload
+    end
+
+    context 'with add to merge train when pipeline succeeds strategy' do
+      let(:auto_merge_strategy) do
+        AutoMergeService::STRATEGY_ADD_TO_MERGE_TRAIN_WHEN_PIPELINE_SUCCEEDS
+      end
+
+      it_behaves_like 'maintained merge requests for MWPS'
+    end
+
+    context 'with merge train strategy' do
+      let(:auto_merge_strategy) { AutoMergeService::STRATEGY_MERGE_TRAIN }
+
+      it_behaves_like 'maintained merge requests for MWPS'
+    end
+  end
 end

spec/models/merge_request_spec.rb

@@ -3368,7 +3368,7 @@ describe MergeRequest do
     end
   end
 
-  describe '.with_open_merge_when_pipeline_succeeds' do
+  describe '.with_auto_merge_enabled' do
     let!(:project) { create(:project) }
     let!(:fork) { fork_project(project) }
     let!(:merge_request1) do
@@ -3380,15 +3380,6 @@ describe MergeRequest do
              source_branch: 'feature-1')
     end
 
-    let!(:merge_request2) do
-      create(:merge_request,
-             :merge_when_pipeline_succeeds,
-             target_project: project,
-             target_branch: 'master',
-             source_project: fork,
-             source_branch: 'fork-feature-1')
-    end
-
     let!(:merge_request4) do
       create(:merge_request,
              target_project: project,
@@ -3397,9 +3388,9 @@ describe MergeRequest do
              source_branch: 'fork-feature-2')
     end
 
-    let(:query) { described_class.with_open_merge_when_pipeline_succeeds }
+    let(:query) { described_class.with_auto_merge_enabled }
 
-    it { expect(query).to contain_exactly(merge_request1, merge_request2) }
+    it { expect(query).to contain_exactly(merge_request1) }
   end
 
   it_behaves_like 'versioned description'

spec/services/merge_requests/refresh_service_spec.rb

@@ -769,7 +769,7 @@ describe MergeRequests::RefreshService do
       fork_project(target_project, author, repository: true)
     end
 
-    let_it_be(:merge_request) do
+    let_it_be(:merge_request, refind: true) do
       create(:merge_request,
              author: author,
              source_project: source_project,
@@ -795,88 +795,58 @@ describe MergeRequests::RefreshService do
         .parent_id
     end
 
+    let(:auto_merge_strategy) { AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS }
     let(:refresh_service) { service.new(project, user) }
 
     before do
       target_project.merge_method = merge_method
       target_project.save!
+      merge_request.auto_merge_strategy = auto_merge_strategy
+      merge_request.save!
 
       refresh_service.execute(oldrev, newrev, 'refs/heads/master')
       merge_request.reload
     end
 
-    let(:aborted_message) do
-      /aborted the automatic merge because target branch was updated/
-    end
-
-    shared_examples 'aborted MWPS' do
-      it 'aborts auto_merge' do
-        expect(merge_request.auto_merge_enabled?).to be_falsey
-        expect(merge_request.notes.last.note).to match(aborted_message)
-      end
-
-      it 'removes merge_user' do
-        expect(merge_request.merge_user).to be_nil
-      end
-
-      it 'does not add todos for merge user' do
-        expect(user.todos.for_target(merge_request)).to be_empty
-      end
-
-      it 'adds todos for merge author' do
-        expect(author.todos.for_target(merge_request)).to be_present.and be_all(&:pending?)
-      end
-    end
-
     context 'when Project#merge_method is set to FF' do
       let(:merge_method) { :ff }
 
-      it_behaves_like 'aborted MWPS'
+      it_behaves_like 'aborted merge requests for MWPS'
 
       context 'with forked project' do
         let(:source_project) { forked_project }
 
-        it_behaves_like 'aborted MWPS'
+        it_behaves_like 'aborted merge requests for MWPS'
+      end
+
+      context 'with bogus auto merge strategy' do
+        let(:auto_merge_strategy) { 'bogus' }
+
+        it_behaves_like 'maintained merge requests for MWPS'
       end
     end
 
     context 'when Project#merge_method is set to rebase_merge' do
       let(:merge_method) { :rebase_merge }
 
-      it_behaves_like 'aborted MWPS'
+      it_behaves_like 'aborted merge requests for MWPS'
 
       context 'with forked project' do
         let(:source_project) { forked_project }
 
-        it_behaves_like 'aborted MWPS'
+        it_behaves_like 'aborted merge requests for MWPS'
       end
     end
 
     context 'when Project#merge_method is set to merge' do
       let(:merge_method) { :merge }
 
-      shared_examples 'maintained MWPS' do
-        it 'does not cancel auto merge' do
-          expect(merge_request.auto_merge_enabled?).to be_truthy
-          expect(merge_request.notes).to be_empty
-        end
-
-        it 'does not change merge_user' do
-          expect(merge_request.merge_user).to eq(user)
-        end
-
-        it 'does not add todos' do
-          expect(author.todos.for_target(merge_request)).to be_empty
-          expect(user.todos.for_target(merge_request)).to be_empty
-        end
-      end
-
-      it_behaves_like 'maintained MWPS'
+      it_behaves_like 'maintained merge requests for MWPS'
 
       context 'with forked project' do
         let(:source_project) { forked_project }
 
-        it_behaves_like 'maintained MWPS'
+        it_behaves_like 'maintained merge requests for MWPS'
       end
     end
   end

spec/support/shared_examples/ci/auto_merge_merge_requests_examples.rb

+# frozen_string_literal: true
+
+shared_examples 'aborted merge requests for MWPS' do
+  let(:aborted_message) do
+    /aborted the automatic merge because target branch was updated/
+  end
+
+  it 'aborts auto_merge' do
+    expect(merge_request.auto_merge_enabled?).to be_falsey
+    expect(merge_request.notes.last.note).to match(aborted_message)
+  end
+
+  it 'removes merge_user' do
+    expect(merge_request.merge_user).to be_nil
+  end
+
+  it 'does not add todos for merge user' do
+    expect(user.todos.for_target(merge_request)).to be_empty
+  end
+
+  it 'adds todos for merge author' do
+    expect(author.todos.for_target(merge_request)).to be_present.and be_all(&:pending?)
+  end
+end
+
+shared_examples 'maintained merge requests for MWPS' do
+  it 'does not cancel auto merge' do
+    expect(merge_request.auto_merge_enabled?).to be_truthy
+    expect(merge_request.notes).to be_empty
+  end
+
+  it 'does not change merge_user' do
+    expect(merge_request.merge_user).to eq(user)
+  end
+
+  it 'does not add todos' do
+    expect(author.todos.for_target(merge_request)).to be_empty
+    expect(user.todos.for_target(merge_request)).to be_empty
+  end
+end