Skip to content
Snippets Groups Projects
Commit d4b46936 authored by Shinya Maeda's avatar Shinya Maeda
Browse files

Abstract auto merge processes

We have one auto merge strategy today - Merge When Pipeline
Succeeds.

In order to add more strategies for Merge Train feature,
we abstract the architecture to be more extensible.

Removed arguments

Fix spec
parent 96744d0b
No related branches found
No related tags found
No related merge requests found
Showing
with 195 additions and 126 deletions
Loading
Loading
@@ -57,7 +57,7 @@ export default {
removeSourceBranch() {
const options = {
sha: this.mr.sha,
merge_when_pipeline_succeeds: true,
auto_merge_strategy: 'merge_when_pipeline_succeeds',
should_remove_source_branch: true,
};
 
Loading
Loading
@@ -85,7 +85,7 @@ export default {
<h4 class="d-flex align-items-start">
<span class="append-right-10">
{{ s__('mrWidget|Set by') }}
<mr-widget-author :author="mr.setToMWPSBy" />
<mr-widget-author :author="mr.setToAutoMergeBy" />
{{ s__('mrWidget|to be merged automatically when the pipeline succeeds') }}
</span>
<a
Loading
Loading
Loading
Loading
@@ -31,7 +31,7 @@ export default {
return {
removeSourceBranch: this.mr.shouldRemoveSourceBranch,
mergeWhenBuildSucceeds: false,
setToMergeWhenPipelineSucceeds: false,
autoMergeStrategy: null,
isMakingRequest: false,
isMergingImmediately: false,
commitMessage: this.mr.commitMessage,
Loading
Loading
@@ -42,7 +42,7 @@ export default {
};
},
computed: {
shouldShowMergeWhenPipelineSucceedsText() {
shouldShowAutoMergeText() {
return this.mr.isPipelineActive;
},
status() {
Loading
Loading
@@ -87,7 +87,7 @@ export default {
mergeButtonText() {
if (this.isMergingImmediately) {
return __('Merge in progress');
} else if (this.shouldShowMergeWhenPipelineSucceedsText) {
} else if (this.shouldShowAutoMergeText) {
return __('Merge when pipeline succeeds');
}
 
Loading
Loading
@@ -104,7 +104,7 @@ export default {
return enableSquashBeforeMerge && commitsCount > 1;
},
shouldShowMergeControls() {
return this.mr.isMergeAllowed || this.shouldShowMergeWhenPipelineSucceedsText;
return this.mr.isMergeAllowed || this.shouldShowAutoMergeText;
},
shouldShowSquashEdit() {
return this.squashBeforeMerge && this.shouldShowSquashBeforeMerge;
Loading
Loading
@@ -126,12 +126,16 @@ export default {
this.isMergingImmediately = true;
}
 
this.setToMergeWhenPipelineSucceeds = mergeWhenBuildSucceeds === true;
if (mergeWhenBuildSucceeds === true) {
this.autoMergeStrategy = 'merge_when_pipeline_succeeds'
} else {
this.autoMergeStrategy = null
}
 
const options = {
sha: this.mr.sha,
commit_message: this.commitMessage,
merge_when_pipeline_succeeds: this.setToMergeWhenPipelineSucceeds,
auto_merge_strategy: this.autoMergeStrategy,
should_remove_source_branch: this.removeSourceBranch === true,
squash: this.squashBeforeMerge,
squash_commit_message: this.squashCommitMessage,
Loading
Loading
Loading
Loading
@@ -23,7 +23,7 @@ export default function deviseState(data) {
return stateKey.pipelineBlocked;
} else if (this.isSHAMismatch) {
return stateKey.shaMismatch;
} else if (this.mergeWhenPipelineSucceeds) {
} else if (this.autoMergeEnabled) {
return this.mergeError ? stateKey.autoMergeFailed : stateKey.mergeWhenPipelineSucceeds;
} else if (!this.canMerge) {
return stateKey.notAllowedToMerge;
Loading
Loading
Loading
Loading
@@ -61,7 +61,7 @@ export default class MergeRequestStore {
 
this.updatedAt = data.updated_at;
this.metrics = MergeRequestStore.buildMetrics(data.metrics);
this.setToMWPSBy = MergeRequestStore.formatUserObject(data.merge_user || {});
this.setToAutoMergeBy = MergeRequestStore.formatUserObject(data.merge_user || {});
this.mergeUserId = data.merge_user_id;
this.currentUserId = gon.current_user_id;
this.sourceBranchPath = data.source_branch_path;
Loading
Loading
@@ -70,12 +70,13 @@ export default class MergeRequestStore {
this.targetBranchPath = data.target_branch_commits_path;
this.targetBranchTreePath = data.target_branch_tree_path;
this.conflictResolutionPath = data.conflict_resolution_path;
this.cancelAutoMergePath = data.cancel_merge_when_pipeline_succeeds_path;
this.cancelAutoMergePath = data.cancel_auto_merge_path;
this.removeWIPPath = data.remove_wip_path;
this.sourceBranchRemoved = !data.source_branch_exists;
this.shouldRemoveSourceBranch = data.remove_source_branch || false;
this.onlyAllowMergeIfPipelineSucceeds = data.only_allow_merge_if_pipeline_succeeds || false;
this.mergeWhenPipelineSucceeds = data.merge_when_pipeline_succeeds || false;
this.autoMergeEnabled = data.auto_merge_enabled || false;
this.autoMergeStrategy = data.auto_merge_strategy;
this.mergePath = data.merge_path;
this.ffOnlyEnabled = data.ff_only_enabled;
this.shouldBeRebased = !!data.should_be_rebased;
Loading
Loading
@@ -93,7 +94,7 @@ export default class MergeRequestStore {
this.canRemoveSourceBranch = currentUser.can_remove_source_branch || false;
this.canMerge = !!data.merge_path;
this.canCreateIssue = currentUser.can_create_issue || false;
this.canCancelAutomaticMerge = !!data.cancel_merge_when_pipeline_succeeds_path;
this.canCancelAutomaticMerge = !!data.cancel_auto_merge_path;
this.isSHAMismatch = this.sha !== data.diff_head_sha;
this.canBeMerged = data.can_be_merged || false;
this.isMergeAllowed = data.mergeable || false;
Loading
Loading
Loading
Loading
@@ -145,14 +145,12 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
render partial: 'projects/merge_requests/widget/commit_change_content', layout: false
end
 
def cancel_merge_when_pipeline_succeeds
unless @merge_request.can_cancel_merge_when_pipeline_succeeds?(current_user)
def cancel_auto_merge
unless @merge_request.can_cancel_auto_merge?(current_user)
return access_denied!
end
 
::MergeRequests::MergeWhenPipelineSucceedsService
.new(@project, current_user)
.cancel(@merge_request)
AutoMergeService.new(project, current_user).cancel(@merge_request)
 
render json: serialize_widget(@merge_request)
end
Loading
Loading
@@ -229,12 +227,12 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
end
 
def merge_params_attributes
[:should_remove_source_branch, :commit_message, :squash_commit_message, :squash]
[:should_remove_source_branch, :commit_message, :squash_commit_message, :squash, :auto_merge_strategy]
end
 
def merge_when_pipeline_succeeds_active?
params[:merge_when_pipeline_succeeds].present? &&
@merge_request.head_pipeline && @merge_request.head_pipeline.active?
def auto_merge_requested?
# Support params[:merge_when_pipeline_succeeds] during the transition period
params[:auto_merge_strategy].present? || params[:merge_when_pipeline_succeeds].present?
end
 
def close_merge_request_if_no_source_project
Loading
Loading
@@ -258,9 +256,9 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
end
 
def merge!
# Disable the CI check if merge_when_pipeline_succeeds is enabled since we have
# Disable the CI check if auto_merge_strategy is specified since we have
# to wait until CI completes to know
unless @merge_request.mergeable?(skip_ci_check: merge_when_pipeline_succeeds_active?)
unless @merge_request.mergeable?(skip_ci_check: auto_merge_requested?)
return :failed
end
 
Loading
Loading
@@ -274,24 +272,10 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
 
@merge_request.update(merge_error: nil, squash: merge_params.fetch(:squash, false))
 
if params[:merge_when_pipeline_succeeds].present?
return :failed unless @merge_request.actual_head_pipeline
if @merge_request.actual_head_pipeline.active?
::MergeRequests::MergeWhenPipelineSucceedsService
.new(@project, current_user, merge_params)
.execute(@merge_request)
:merge_when_pipeline_succeeds
elsif @merge_request.actual_head_pipeline.success?
# This can be triggered when a user clicks the auto merge button while
# the tests finish at about the same time
@merge_request.merge_async(current_user.id, merge_params)
:success
else
:failed
end
if auto_merge_requested?
AutoMergeService.new(project, current_user, merge_params)
.execute(merge_request,
params[:auto_merge_strategy] || AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS)
else
@merge_request.merge_async(current_user.id, merge_params)
 
Loading
Loading
Loading
Loading
@@ -103,7 +103,7 @@ module MergeRequestsHelper
 
def merge_params(merge_request)
{
merge_when_pipeline_succeeds: true,
auto_merge_strategy: AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS,
should_remove_source_branch: true,
sha: merge_request.diff_head_sha,
squash: merge_request.squash
Loading
Loading
Loading
Loading
@@ -165,7 +165,7 @@ class MergeRequest < ApplicationRecord
validates :source_branch, presence: true
validates :target_project, presence: true
validates :target_branch, presence: true
validates :merge_user, presence: true, if: :merge_when_pipeline_succeeds?, unless: :importing?
validates :merge_user, presence: true, if: :auto_merge_enabled?, unless: :importing?
validate :validate_branches, unless: [:allow_broken, :importing?, :closed_without_fork?]
validate :validate_fork, unless: :closed_without_fork?
validate :validate_target_project, on: :create
Loading
Loading
@@ -196,6 +196,7 @@ class MergeRequest < ApplicationRecord
 
alias_attribute :project, :target_project
alias_attribute :project_id, :target_project_id
alias_attribute :auto_merge_enabled, :merge_when_pipeline_succeeds
 
def self.reference_prefix
'!'
Loading
Loading
@@ -391,7 +392,7 @@ class MergeRequest < ApplicationRecord
def merge_participants
participants = [author]
 
if merge_when_pipeline_succeeds? && !participants.include?(merge_user)
if auto_merge_enabled? && !participants.include?(merge_user)
participants << merge_user
end
 
Loading
Loading
@@ -782,7 +783,7 @@ class MergeRequest < ApplicationRecord
project.ff_merge_must_be_possible? && !ff_merge_possible?
end
 
def can_cancel_merge_when_pipeline_succeeds?(current_user)
def can_cancel_auto_merge?(current_user)
can_be_merged_by?(current_user) || self.author == current_user
end
 
Loading
Loading
@@ -801,6 +802,16 @@ class MergeRequest < ApplicationRecord
Gitlab::Utils.to_boolean(merge_params['force_remove_source_branch'])
end
 
def auto_merge_strategy
return unless auto_merge_enabled?
merge_params['auto_merge_strategy'] || AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS
end
def auto_merge_strategy=(strategy)
merge_params['auto_merge_strategy'] = strategy
end
def remove_source_branch?
should_remove_source_branch? || force_remove_source_branch?
end
Loading
Loading
@@ -973,15 +984,16 @@ class MergeRequest < ApplicationRecord
end
end
 
def reset_merge_when_pipeline_succeeds
return unless merge_when_pipeline_succeeds?
def reset_auto_merge
return unless auto_merge_enabled?
 
self.merge_when_pipeline_succeeds = false
self.auto_merge_enabled = false
self.merge_user = nil
if merge_params
merge_params.delete('should_remove_source_branch')
merge_params.delete('commit_message')
merge_params.delete('squash_commit_message')
merge_params.delete('auto_merge_strategy')
end
 
self.save
Loading
Loading
Loading
Loading
@@ -22,9 +22,9 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated
end
end
 
def cancel_merge_when_pipeline_succeeds_path
if can_cancel_merge_when_pipeline_succeeds?(current_user)
cancel_merge_when_pipeline_succeeds_project_merge_request_path(project, merge_request)
def cancel_auto_merge_path
if can_cancel_auto_merge?(current_user)
cancel_auto_merge_project_merge_request_path(project, merge_request)
end
end
 
Loading
Loading
Loading
Loading
@@ -9,7 +9,11 @@ class MergeRequestWidgetEntity < IssuableEntity
expose :merge_params
expose :merge_status
expose :merge_user_id
expose :merge_when_pipeline_succeeds
expose :auto_merge_enabled
expose :auto_merge_strategy
expose :available_auto_merge_strategies do |merge_request|
AutoMergeService.new(merge_request.project, current_user).available_strategies(merge_request) # rubocop: disable CodeReuse/ServiceClass
end
expose :source_branch
expose :source_branch_protected do |merge_request|
merge_request.source_project.present? && ProtectedBranch.protected?(merge_request.source_project, merge_request.source_branch)
Loading
Loading
@@ -182,8 +186,8 @@ class MergeRequestWidgetEntity < IssuableEntity
presenter(merge_request).remove_wip_path
end
 
expose :cancel_merge_when_pipeline_succeeds_path do |merge_request|
presenter(merge_request).cancel_merge_when_pipeline_succeeds_path
expose :cancel_auto_merge_path do |merge_request|
presenter(merge_request).cancel_auto_merge_path
end
 
expose :create_issue_to_resolve_discussions_path do |merge_request|
Loading
Loading
# frozen_string_literal: true
module AutoMerge
class MergeWhenPipelineSucceedsService < BaseService
def execute(merge_request)
return :failed unless merge_request.actual_head_pipeline
if merge_request.actual_head_pipeline.active?
merge_request.merge_params.merge!(params)
unless merge_request.auto_merge_enabled?
merge_request.auto_merge_enabled = true
merge_request.merge_user = @current_user
merge_request.auto_merge_strategy = AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS
SystemNoteService.merge_when_pipeline_succeeds(merge_request, @project, @current_user, merge_request.diff_head_commit)
end
return :failed unless merge_request.save
:merge_when_pipeline_succeeds
elsif merge_request.actual_head_pipeline.success?
# This can be triggered when a user clicks the auto merge button while
# the tests finish at about the same time
merge_request.merge_async(current_user.id, merge_params)
:success
else
:failed
end
end
def process(merge_request)
return unless merge_request.actual_head_pipeline&.success?
return unless merge_request.mergeable?
merge_request.merge_async(merge_request.merge_user_id, merge_request.merge_params)
end
def cancel(merge_request)
if merge_request.reset_auto_merge
SystemNoteService.cancel_merge_when_pipeline_succeeds(merge_request, @project, @current_user)
success
else
error("Can't cancel the automatic merge", 406)
end
end
def available_for?(merge_request)
merge_request.actual_head_pipeline&.active?
end
end
end
# frozen_string_literal: true
class AutoMergeService < BaseService
STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS = 'merge_when_pipeline_succeeds'.freeze
STRATEGIES = [STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS].freeze
class << self
def all_strategies
STRATEGIES
end
def get_service_class(strategy)
return unless all_strategies.include?(strategy)
"::AutoMerge::#{strategy.camelize}Service".constantize
end
end
def execute(merge_request, strategy)
service = get_service_instance(strategy)
return :failed unless service&.available_for?(merge_request)
service.execute(merge_request)
end
def process(merge_request)
return unless merge_request.auto_merge_enabled?
get_service_instance(merge_request.auto_merge_strategy).process(merge_request)
end
def cancel(merge_request)
return error("Can't cancel the automatic merge", 406) unless merge_request.auto_merge_enabled?
get_service_instance(merge_request.auto_merge_strategy).cancel(merge_request)
end
def available_strategies(merge_request)
self.class.all_strategies.select do |strategy|
get_service_instance(strategy).available_for?(merge_request)
end
end
private
def get_service_instance(strategy)
self.class.get_service_class(strategy)&.new(project, current_user, params)
end
end
# frozen_string_literal: true
module MergeRequests
class MergeWhenPipelineSucceedsService < MergeRequests::BaseService
# Marks the passed `merge_request` to be merged when the pipeline succeeds or
# updates the params for the automatic merge
def execute(merge_request)
merge_request.merge_params.merge!(params)
# The service is also called when the merge params are updated.
already_approved = merge_request.merge_when_pipeline_succeeds?
unless already_approved
merge_request.merge_when_pipeline_succeeds = true
merge_request.merge_user = @current_user
SystemNoteService.merge_when_pipeline_succeeds(merge_request, @project, @current_user, merge_request.diff_head_commit)
end
merge_request.save
end
# Triggers the automatic merge of merge_request once the pipeline succeeds
def trigger(pipeline)
return unless pipeline.success?
pipeline_merge_requests(pipeline) do |merge_request|
next unless merge_request.merge_when_pipeline_succeeds?
next unless merge_request.mergeable?
merge_request.merge_async(merge_request.merge_user_id, merge_request.merge_params)
end
end
# Cancels the automatic merge
def cancel(merge_request)
if merge_request.merge_when_pipeline_succeeds? && merge_request.open?
merge_request.reset_merge_when_pipeline_succeeds
SystemNoteService.cancel_merge_when_pipeline_succeeds(merge_request, @project, @current_user)
success
else
error("Can't cancel the automatic merge", 406)
end
end
end
end
Loading
Loading
@@ -24,7 +24,7 @@ module MergeRequests
reload_merge_requests
outdate_suggestions
refresh_pipelines_on_merge_requests
reset_merge_when_pipeline_succeeds
cancel_auto_merge
mark_pending_todos_done
cache_merge_requests_closing_issues
 
Loading
Loading
@@ -142,8 +142,10 @@ module MergeRequests
end
end
 
def reset_merge_when_pipeline_succeeds
merge_requests_for_source_branch.each(&:reset_merge_when_pipeline_succeeds)
def cancel_auto_merge
merge_requests_for_source_branch.each do |merge_request|
AutoMergeService.new(project, current_user).cancel(merge_request)
end
end
 
def mark_pending_todos_done
Loading
Loading
Loading
Loading
@@ -89,7 +89,7 @@ module MergeRequests
merge_request.update(merge_error: nil)
 
if merge_request.head_pipeline && merge_request.head_pipeline.active?
MergeRequests::MergeWhenPipelineSucceedsService.new(project, current_user).execute(merge_request)
AutoMergeService.new(project, current_user).execute(merge_request, AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS)
else
merge_request.merge_async(current_user.id, {})
end
Loading
Loading
Loading
Loading
@@ -238,7 +238,7 @@ class NotificationService
merge_request,
current_user,
:merged_merge_request_email,
skip_current_user: !merge_request.merge_when_pipeline_succeeds?
skip_current_user: !merge_request.auto_merge_enabled?
)
end
 
Loading
Loading
Loading
Loading
@@ -9,9 +9,10 @@ class PipelineSuccessWorker
# rubocop: disable CodeReuse/ActiveRecord
def perform(pipeline_id)
Ci::Pipeline.find_by(id: pipeline_id).try do |pipeline|
MergeRequests::MergeWhenPipelineSucceedsService
.new(pipeline.project, nil)
.trigger(pipeline)
pipeline.all_merge_requests.preload(:merge_user).each do |merge_request|
AutoMergeService.new(pipeline.project, merge_request.merge_user)
.process(merge_request)
end
end
end
# rubocop: enable CodeReuse/ActiveRecord
Loading
Loading
---
title: Refactor and abstract Auto Merge Processes
merge_request: 28595
author:
type: other
Loading
Loading
@@ -208,7 +208,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
member do
get :commit_change_content
post :merge
post :cancel_merge_when_pipeline_succeeds
post :cancel_auto_merge
get :pipeline_status
get :ci_environments_status
post :toggle_subscription
Loading
Loading
Loading
Loading
@@ -386,9 +386,8 @@ module API
)
 
if merge_when_pipeline_succeeds && merge_request.head_pipeline && merge_request.head_pipeline.active?
::MergeRequests::MergeWhenPipelineSucceedsService
.new(merge_request.target_project, current_user, merge_params)
.execute(merge_request)
AutoMergeService.new(merge_request.target_project, current_user, merge_params)
.execute(merge_request, AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS)
else
::MergeRequests::MergeService
.new(merge_request.target_project, current_user, merge_params)
Loading
Loading
@@ -429,11 +428,9 @@ module API
post ':id/merge_requests/:merge_request_iid/cancel_merge_when_pipeline_succeeds' do
merge_request = find_project_merge_request(params[:merge_request_iid])
 
unauthorized! unless merge_request.can_cancel_merge_when_pipeline_succeeds?(current_user)
unauthorized! unless merge_request.can_cancel_auto_merge?(current_user)
 
::MergeRequests::MergeWhenPipelineSucceedsService
.new(merge_request.target_project, current_user)
.cancel(merge_request)
AutoMergeService.new(merge_request.target_project, current_user).cancel(merge_request)
end
 
desc 'Rebase the merge request against its target branch' do
Loading
Loading
Loading
Loading
@@ -429,8 +429,9 @@ describe Projects::MergeRequestsController do
 
it 'sets the MR to merge when the pipeline succeeds' do
service = double(:merge_when_pipeline_succeeds_service)
allow(service).to receive(:available_for?) { true }
 
expect(MergeRequests::MergeWhenPipelineSucceedsService)
expect(AutoMerge::MergeWhenPipelineSucceedsService)
.to receive(:new).with(project, anything, anything)
.and_return(service)
expect(service).to receive(:execute).with(merge_request)
Loading
Loading
@@ -713,9 +714,9 @@ describe Projects::MergeRequestsController do
end
end
 
describe 'POST cancel_merge_when_pipeline_succeeds' do
describe 'POST cancel_auto_merge' do
subject do
post :cancel_merge_when_pipeline_succeeds,
post :cancel_auto_merge,
params: {
format: :json,
namespace_id: merge_request.project.namespace.to_param,
Loading
Loading
@@ -725,14 +726,15 @@ describe Projects::MergeRequestsController do
xhr: true
end
 
it 'calls MergeRequests::MergeWhenPipelineSucceedsService' do
mwps_service = double
it 'calls AutoMergeService' do
auto_merge_service = double
 
allow(MergeRequests::MergeWhenPipelineSucceedsService)
allow(AutoMergeService)
.to receive(:new)
.and_return(mwps_service)
.and_return(auto_merge_service)
 
expect(mwps_service).to receive(:cancel).with(merge_request)
allow(auto_merge_service).to receive(:available_strategies).with(merge_request)
expect(auto_merge_service).to receive(:cancel).with(merge_request)
 
subject
end
Loading
Loading
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment