diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 18c674b453bb428491e4d671f8f63a1a6adcfa64..92a539fe72c0f6c2d094b486a55ebe3e7b6fc326 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -1230,8 +1230,18 @@ def draft_title
alias_method :wip_title, :draft_title
def skipped_mergeable_checks(options = {})
+ merge_when_checks_pass_strat = options[:auto_merge_strategy] == ::AutoMergeService::STRATEGY_MERGE_WHEN_CHECKS_PASS
+
+ skip_additional_checks = merge_when_checks_pass_strat &&
+ ::Feature.enabled?(:additional_merge_when_checks_ready, project)
+
{
- skip_ci_check: options.fetch(:auto_merge_requested, false)
+ skip_ci_check: options.fetch(:auto_merge_requested, false),
+ skip_approved_check: merge_when_checks_pass_strat,
+ skip_draft_check: skip_additional_checks,
+ skip_blocked_check: skip_additional_checks,
+ skip_discussions_check: skip_additional_checks,
+ skip_external_status_check: skip_additional_checks
}
end
@@ -1280,31 +1290,28 @@ def self.all_mergeability_checks
# skip_approved_check
# skip_blocked_check
# skip_external_status_check
- def mergeable_state?(**mergeable_state_check_params)
- additional_checks = execute_merge_checks(
- self.class.mergeable_state_checks,
- params: mergeable_state_check_params
- )
- additional_checks.success?
+ def mergeable_state?(**params)
+ results = check_mergeability_states(checks: self.class.mergeable_state_checks, **params)
+
+ results.success?
end
- def mergeable_git_state?(skip_rebase_check: false)
- checks = execute_merge_checks(
- self.class.mergeable_git_state_checks,
- params: {
- skip_rebase_check: skip_rebase_check
- }
- )
+ # This runs only git related checks
+ def mergeable_git_state?(**params)
+ results = check_mergeability_states(checks: self.class.mergeable_git_state_checks, **params)
+
+ results.success?
+ end
+
+ # This runs all the checks
+ def mergeability_checks_pass?(**params)
+ results = check_mergeability_states(checks: self.class.all_mergeability_checks, **params)
- checks.success?
+ results.success?
end
def all_mergeability_checks_results
- execute_merge_checks(
- self.class.all_mergeability_checks,
- params: {},
- execute_all: true
- ).payload[:results]
+ check_mergeability_states(checks: self.class.all_mergeability_checks, execute_all: true).payload[:results]
end
def ff_merge_possible?
@@ -2236,6 +2243,14 @@ def auto_merge_when_incomplete_pipeline_succeeds_enabled?
)
end
+ def check_mergeability_states(checks:, execute_all: false, **params)
+ execute_merge_checks(
+ checks,
+ params: params,
+ execute_all: execute_all
+ )
+ end
+
def merge_base_pipelines
return ::Ci::Pipeline.none unless diff_head_pipeline&.target_sha
diff --git a/app/services/auto_merge/base_service.rb b/app/services/auto_merge/base_service.rb
index 9d2d89e56650e81c6773fb412e51fb9884eb76b6..61d2ab6e70a8406039b092f119a81702dfcca227 100644
--- a/app/services/auto_merge/base_service.rb
+++ b/app/services/auto_merge/base_service.rb
@@ -58,16 +58,29 @@ def abort(merge_request, reason)
def available_for?(merge_request)
strong_memoize("available_for_#{merge_request.id}") do
- merge_request.can_be_merged_by?(current_user) &&
- merge_request.open? &&
- !merge_request.broken? &&
- overrideable_available_for_checks(merge_request) &&
- yield
+ if Feature.enabled?(:refactor_auto_merge, merge_request.project, type: :gitlab_com_derisk)
+ merge_request.can_be_merged_by?(current_user) &&
+ merge_request.mergeability_checks_pass?(**skippable_available_for_checks(merge_request)) &&
+ yield
+ else
+ merge_request.can_be_merged_by?(current_user) &&
+ merge_request.open? &&
+ !merge_request.broken? &&
+ overrideable_available_for_checks(merge_request) &&
+ yield
+ end
end
end
private
+ def skippable_available_for_checks(merge_request)
+ merge_request.skipped_mergeable_checks(
+ auto_merge_requested: true,
+ auto_merge_strategy: strategy
+ )
+ end
+
def overrideable_available_for_checks(merge_request)
!merge_request.draft? &&
merge_request.mergeable_discussions_state? &&
diff --git a/app/services/auto_merge/merge_when_pipeline_succeeds_service.rb b/app/services/auto_merge/merge_when_pipeline_succeeds_service.rb
index db67a1a9f26982146b5f78188a80aff25e1daacb..a669eea337a6ffac0f9458aa9ce79cc62802df01 100644
--- a/app/services/auto_merge/merge_when_pipeline_succeeds_service.rb
+++ b/app/services/auto_merge/merge_when_pipeline_succeeds_service.rb
@@ -34,7 +34,7 @@ def abort(merge_request, reason)
def available_for?(merge_request)
super do
- check_availability(merge_request)
+ merge_request.auto_merge_available_when_pipeline_succeeds?
end
end
@@ -44,10 +44,6 @@ def add_system_note(merge_request)
SystemNoteService.merge_when_pipeline_succeeds(merge_request, project, current_user, merge_request.diff_head_pipeline.sha) if merge_request.saved_change_to_auto_merge_enabled?
end
- def check_availability(merge_request)
- merge_request.auto_merge_available_when_pipeline_succeeds?
- end
-
def notify(merge_request)
notification_service.async.merge_when_pipeline_succeeds(merge_request, current_user) if merge_request.saved_change_to_auto_merge_enabled?
end
diff --git a/app/services/auto_merge_service.rb b/app/services/auto_merge_service.rb
index 912248d3a06ea682df4b74030752515caad36cc3..71a2a3241363f73dea51c476889457f58b785a11 100644
--- a/app/services/auto_merge_service.rb
+++ b/app/services/auto_merge_service.rb
@@ -4,6 +4,8 @@ class AutoMergeService < BaseService
include Gitlab::Utils::StrongMemoize
STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS = 'merge_when_pipeline_succeeds'
+ # Currently only EE but will be moved to CE in (https://gitlab.com/gitlab-org/gitlab/-/merge_requests/146730)
+ STRATEGY_MERGE_WHEN_CHECKS_PASS = 'merge_when_checks_pass'
STRATEGIES = [STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS].freeze
class << self
diff --git a/config/feature_flags/gitlab_com_derisk/refactor_auto_merge.yml b/config/feature_flags/gitlab_com_derisk/refactor_auto_merge.yml
new file mode 100644
index 0000000000000000000000000000000000000000..901997fd1608d73cc7a92f207a8b1dd6e21046e0
--- /dev/null
+++ b/config/feature_flags/gitlab_com_derisk/refactor_auto_merge.yml
@@ -0,0 +1,9 @@
+---
+name: refactor_auto_merge
+feature_issue_url: https://gitlab.com/groups/gitlab-org/-/epics/10874
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/146153
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/443872
+milestone: '16.11'
+group: group::code review
+type: gitlab_com_derisk
+default_enabled: false
diff --git a/ee/app/models/ee/merge_request.rb b/ee/app/models/ee/merge_request.rb
index 30ffc2bd2c52dd59ae045c006e7295204a0a916b..81c8eb4da6fed22bed37080d1c7b0e7ca2ca69ff 100644
--- a/ee/app/models/ee/merge_request.rb
+++ b/ee/app/models/ee/merge_request.rb
@@ -201,20 +201,6 @@ def predefined_variables
super.concat(merge_request_approval_variables)
end
- override :skipped_mergeable_checks
- def skipped_mergeable_checks(options = {})
- skip_additional_checks = options[:auto_merge_strategy] == AutoMergeService::STRATEGY_MERGE_WHEN_CHECKS_PASS &&
- ::Feature.enabled?(:additional_merge_when_checks_ready, project)
-
- super.merge(
- skip_approved_check: options[:auto_merge_strategy] == AutoMergeService::STRATEGY_MERGE_WHEN_CHECKS_PASS,
- skip_draft_check: skip_additional_checks,
- skip_blocked_check: skip_additional_checks,
- skip_external_status_check: skip_additional_checks,
- skip_discussions_check: skip_additional_checks
- )
- end
-
override :merge_blocked_by_other_mrs?
def merge_blocked_by_other_mrs?
strong_memoize(:merge_blocked_by_other_mrs) do
diff --git a/ee/app/services/auto_merge/merge_when_checks_pass_service.rb b/ee/app/services/auto_merge/merge_when_checks_pass_service.rb
index 55459c3e7a9d16de4bc3cf60eab6fae1422fdb9e..e8ebe47630219ac9a19f81130c9d715fd8171d37 100644
--- a/ee/app/services/auto_merge/merge_when_checks_pass_service.rb
+++ b/ee/app/services/auto_merge/merge_when_checks_pass_service.rb
@@ -4,6 +4,7 @@ module AutoMerge
class MergeWhenChecksPassService < AutoMerge::BaseService
extend Gitlab::Utils::Override
+ override :execute
def execute(merge_request)
super do
add_system_note(merge_request)
@@ -49,9 +50,18 @@ def abort(merge_request, reason)
end
end
+ override :available_for
def available_for?(merge_request)
super do
- check_availability(merge_request)
+ if Feature.disabled?(:refactor_auto_merge, merge_request.project, type: :gitlab_com_derisk)
+ check_availability(merge_request)
+ else
+ next false if Feature.disabled?(:merge_when_checks_pass, merge_request.project)
+ next false if merge_request.project.merge_trains_enabled?
+ next false if merge_request.mergeable? && !merge_request.diff_head_pipeline&.active?
+
+ next true
+ end
end
end
@@ -68,10 +78,12 @@ def add_system_note(merge_request)
)
end
+ # rubocop: disable Metrics/CyclomaticComplexity -- Going to be removed once refactor FF is removed
def check_availability(merge_request)
return false if Feature.disabled?(:merge_when_checks_pass, merge_request.project)
return false unless merge_request.approval_feature_available?
return false if merge_request.project.merge_trains_enabled?
+ return false if merge_request.mergeable? && !merge_request.diff_head_pipeline&.active?
merge_request.diff_head_pipeline&.active? ||
!merge_request.approved? ||
@@ -82,6 +94,7 @@ def check_availability(merge_request)
!merge_request.mergeable_discussions_state?)
)
end
+ # rubocop: enable Metrics/CyclomaticComplexity
def notify(merge_request)
return unless merge_request.saved_change_to_auto_merge_enabled?
diff --git a/ee/app/services/ee/auto_merge_service.rb b/ee/app/services/ee/auto_merge_service.rb
index 598704dbe68fcbf84f9b20684587d254e64d554b..1f2987099754d52f56433f9a96b098c3bc628371 100644
--- a/ee/app/services/ee/auto_merge_service.rb
+++ b/ee/app/services/ee/auto_merge_service.rb
@@ -6,12 +6,11 @@ module AutoMergeService
STRATEGY_MERGE_TRAIN = 'merge_train'
STRATEGY_ADD_TO_MERGE_TRAIN_WHEN_PIPELINE_SUCCEEDS = 'add_to_merge_train_when_pipeline_succeeds'
- # `merge_when_checks_pass` enables auto-merge to be set before approval requirements are satisfied
- STRATEGY_MERGE_WHEN_CHECKS_PASS = 'merge_when_checks_pass'
+
EE_STRATEGIES = [
STRATEGY_MERGE_TRAIN,
STRATEGY_ADD_TO_MERGE_TRAIN_WHEN_PIPELINE_SUCCEEDS,
- STRATEGY_MERGE_WHEN_CHECKS_PASS
+ ::AutoMergeService::STRATEGY_MERGE_WHEN_CHECKS_PASS
].freeze
class_methods do
diff --git a/ee/spec/controllers/projects/merge_requests_controller_spec.rb b/ee/spec/controllers/projects/merge_requests_controller_spec.rb
index c881637411b4a3ec0cc3d793af7a8f6b740660d6..ece8e409957b0cde7088c895b74e72a728551035 100644
--- a/ee/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/ee/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -425,7 +425,7 @@ def update_merge_request(params = {})
context 'with STRATEGY_MERGE_WHEN_CHECKS_PASS requested' do
def merge_when_pipeline_succeeds
post :merge, params: base_params.merge(sha: merge_request.diff_head_sha, merge_when_pipeline_succeeds: '1',
- auto_merge_strategy: AutoMergeService::STRATEGY_MERGE_WHEN_CHECKS_PASS)
+ auto_merge_strategy: ::AutoMergeService::STRATEGY_MERGE_WHEN_CHECKS_PASS)
end
shared_examples_for 'merge MR when checks pass' do
diff --git a/ee/spec/features/merge_request/code_owner_approvals_reset_after_merging_to_source_branch_spec.rb b/ee/spec/features/merge_request/code_owner_approvals_reset_after_merging_to_source_branch_spec.rb
index 8fa6f6173768fd4732f2a5d962ec70ad3ecc4e2e..d7c1b7d0c93b0e43b0d64e9116a446a9ea3015b1 100644
--- a/ee/spec/features/merge_request/code_owner_approvals_reset_after_merging_to_source_branch_spec.rb
+++ b/ee/spec/features/merge_request/code_owner_approvals_reset_after_merging_to_source_branch_spec.rb
@@ -32,7 +32,8 @@
source_project: project,
source_branch: branch_name,
target_project: project,
- target_branch: 'master'
+ target_branch: 'master',
+ merge_status: 'can_be_merged'
)
end
diff --git a/ee/spec/features/merge_request/user_merges_with_push_rules_spec.rb b/ee/spec/features/merge_request/user_merges_with_push_rules_spec.rb
index 054f9eeec4f29fd2dd0f87bd819dea41219a6392..258c38cbf9d363ab55719f1232ad9a2a4c84ad81 100644
--- a/ee/spec/features/merge_request/user_merges_with_push_rules_spec.rb
+++ b/ee/spec/features/merge_request/user_merges_with_push_rules_spec.rb
@@ -5,7 +5,10 @@
RSpec.describe 'Merge request > User merges with Push Rules', :js, feature_category: :code_review_workflow do
let(:user) { create(:user) }
let(:project) { create(:project, :public, :repository, push_rule: push_rule) }
- let(:merge_request) { create(:merge_request_with_diffs, source_project: project, author: user, title: 'Bug NS-04') }
+ let(:merge_request) do
+ create(:merge_request_with_diffs, source_project: project, author: user, title: 'Bug NS-04',
+ merge_status: 'can_be_merged')
+ end
before do
project.add_maintainer(user)
diff --git a/ee/spec/features/merge_trains/two_merge_requests_on_train_spec.rb b/ee/spec/features/merge_trains/two_merge_requests_on_train_spec.rb
index ac3814dd84346b7aa861223b8c87a1294e08a5a4..cfb768d738abfb664f9fe0fe550383b6092306cb 100644
--- a/ee/spec/features/merge_trains/two_merge_requests_on_train_spec.rb
+++ b/ee/spec/features/merge_trains/two_merge_requests_on_train_spec.rb
@@ -14,14 +14,14 @@
create(:merge_request,
source_branch: 'feature', source_project: project,
target_branch: 'master', target_project: project,
- merge_status: 'unchecked')
+ merge_status: 'can_be_merged')
end
let(:merge_request_2) do
create(:merge_request,
source_branch: 'signed-commits', source_project: project,
target_branch: 'master', target_project: project,
- merge_status: 'unchecked')
+ merge_status: 'can_be_merged')
end
let(:ci_yaml) do
diff --git a/ee/spec/requests/api/merge_trains_spec.rb b/ee/spec/requests/api/merge_trains_spec.rb
index 72a1c3bb25b665bfb1d2031f6c00a7b5ff803da8..8b8f94b94385575b120bb93d627f9e5368846635 100644
--- a/ee/spec/requests/api/merge_trains_spec.rb
+++ b/ee/spec/requests/api/merge_trains_spec.rb
@@ -243,7 +243,7 @@
create(:merge_request,
source_project: project, source_branch: 'feature',
target_project: project, target_branch: 'master',
- merge_status: 'unchecked')
+ merge_status: 'can_be_merged')
end
let(:params) { {} }
diff --git a/ee/spec/services/auto_merge/merge_when_checks_pass_service_spec.rb b/ee/spec/services/auto_merge/merge_when_checks_pass_service_spec.rb
index f1ca9078e97466bdce5f0b8c207bbae52db0e23e..c20c482aa2701fc1ccddf5b8295bc7e5ef5a5b61 100644
--- a/ee/spec/services/auto_merge/merge_when_checks_pass_service_spec.rb
+++ b/ee/spec/services/auto_merge/merge_when_checks_pass_service_spec.rb
@@ -13,121 +13,310 @@
end
describe '#available_for?' do
- subject { service.available_for?(mr_merge_if_green_enabled) }
-
- let_it_be(:approver) { create(:user) }
- let(:feature_flag) { true }
- let(:draft_status) { true }
- let(:blocked_status) { true }
- let(:discussions_status) { true }
- let(:additional_feature_flag) { true }
- let(:pipeline_status) { :running }
- let(:approvals_required) { 1 }
-
- before do
- create(:ci_pipeline, pipeline_status,
- ref: mr_merge_if_green_enabled.source_branch,
- sha: mr_merge_if_green_enabled.diff_head_sha,
- project: mr_merge_if_green_enabled.source_project)
- mr_merge_if_green_enabled.update_head_pipeline
-
- approval_rule.users << approver
- stub_feature_flags(merge_when_checks_pass: feature_flag,
- additional_merge_when_checks_ready: additional_feature_flag)
- mr_merge_if_green_enabled.update!(title: 'Draft: check') if draft_status
- allow(mr_merge_if_green_enabled).to receive(:merge_blocked_by_other_mrs?).and_return(blocked_status)
- allow(mr_merge_if_green_enabled).to receive(:mergeable_discussions_state?).and_return(discussions_status)
- end
-
- where(:pipeline_status, :approvals_required, :draft_status, :blocked_status, :discussions_status,
- :external_checks_pass, :additional_feature_flag, :result) do
- :running | 0 | true | true | false | false | true | true
- :running | 0 | false | false | true | true | true | true
- :success | 0 | false | false | true | true | true | false
- :success | 0 | true | true | false | false | true | true
- :success | 0 | true | true | true | false | false | false
- :running | 1 | true | true | false | false | true | true
- :success | 1 | true | true | false | false | true | true
- :success | 1 | false | false | true | true | true | true
- :running | 1 | false | false | true | true | true | true
- end
+ context 'when refactor_auto_merge flag is disabled' do
+ subject { service.available_for?(mr_merge_if_green_enabled) }
+
+ let_it_be(:approver) { create(:user) }
+ let(:feature_flag) { true }
+ let(:draft_status) { false }
+ let(:blocked_status) { false }
+ let(:discussions_status) { true }
+ let(:additional_merge_checks_ff) { true }
+ let(:pipeline_status) { :running }
+ let(:approvals_required) { 0 }
- with_them do
- it { is_expected.to eq result }
- end
+ before do
+ if pipeline_status
+ create(:ci_pipeline, pipeline_status,
+ ref: mr_merge_if_green_enabled.source_branch,
+ sha: mr_merge_if_green_enabled.diff_head_sha,
+ project: mr_merge_if_green_enabled.source_project)
+ mr_merge_if_green_enabled.update_head_pipeline
+ end
- context 'when feature flags merge_when_checks_pass and additional_merge_when_checks_ready are disabled"' do
- let(:additional_feature_flag) { false }
- let(:feature_flag) { false }
+ approval_rule.users << approver
+ stub_feature_flags(merge_when_checks_pass: feature_flag,
+ additional_merge_when_checks_ready: additional_merge_checks_ff,
+ refactor_auto_merge: false)
+ mr_merge_if_green_enabled.update!(title: 'Draft: check') if draft_status
+ allow(mr_merge_if_green_enabled).to receive(:merge_blocked_by_other_mrs?).and_return(blocked_status)
+ allow(mr_merge_if_green_enabled).to receive(:mergeable_discussions_state?).and_return(discussions_status)
+ end
where(:pipeline_status, :approvals_required, :draft_status, :blocked_status, :discussions_status,
- :external_checks_pass, :result) do
- :running | 0 | true | true | false | false | false
- :success | 0 | false | false | true | true | false
- :running | 1 | false | false | true | true | false
- :success | 1 | true | false | true | false | false
+ :external_checks_pass, :additional_merge_checks_ff, :result) do
+ :running | 0 | false | false | true | true | true | true
+ :running | 0 | true | true | false | false | true | true
+ :running | 0 | false | false | true | true | true | true
+ :success | 0 | false | false | true | true | true | false
+ :success | 0 | true | true | false | false | true | true
+ :success | 0 | true | true | true | false | false | false
+ :running | 1 | true | true | false | false | true | true
+ :success | 1 | true | true | false | false | true | true
+ :success | 1 | false | false | true | true | true | true
+ :running | 1 | false | false | true | true | true | true
end
with_them do
it { is_expected.to eq result }
end
- end
- context 'when the user does not have permission to merge' do
- let(:pipeline_status) { :running }
- let(:approvals_required) { 0 }
+ context 'when mergeable and no pipeline' do
+ let(:pipeline_status) { nil }
- before do
- allow(mr_merge_if_green_enabled).to receive(:can_be_merged_by?).and_return(false)
+ it { is_expected.to eq false }
end
- it { is_expected.to eq false }
- end
+ context 'when feature flags merge_when_checks_pass and additional_merge_when_checks_ready are disabled"' do
+ let(:additional_merge_checks_ff) { false }
+ let(:feature_flag) { false }
- context 'when there is an open MR dependency and "additional_merge_when_checks_ready" is disabled' do
- let(:pipeline_status) { :running }
- let(:approvals_required) { 0 }
+ where(:pipeline_status, :approvals_required, :draft_status, :blocked_status, :discussions_status,
+ :external_checks_pass, :result) do
+ :running | 0 | true | true | false | false | false
+ :success | 0 | false | false | true | true | false
+ :running | 1 | false | false | true | true | false
+ :success | 1 | true | false | true | false | false
+ end
- before do
- stub_feature_flags(additional_merge_when_checks_ready: false)
- stub_licensed_features(blocking_merge_requests: true)
- create(:merge_request_block, blocked_merge_request: mr_merge_if_green_enabled)
+ with_them do
+ it { is_expected.to eq result }
+ end
end
- it { is_expected.to eq false }
+ context 'when the user does not have permission to merge' do
+ let(:pipeline_status) { :running }
+ let(:approvals_required) { 0 }
+
+ before do
+ allow(mr_merge_if_green_enabled).to receive(:can_be_merged_by?).and_return(false)
+ end
+
+ it { is_expected.to eq false }
+ end
+
+ context 'when there is an open MR dependency and "additional_merge_when_checks_ready" is disabled' do
+ let(:pipeline_status) { :success }
+ let(:approvals_required) { 0 }
+
+ before do
+ stub_feature_flags(additional_merge_when_checks_ready: false)
+ stub_licensed_features(blocking_merge_requests: true)
+ create(:merge_request_block, blocked_merge_request: mr_merge_if_green_enabled)
+ end
+
+ it { is_expected.to eq false }
+ end
+
+ context 'when merge trains are enabled' do
+ before do
+ allow(mr_merge_if_green_enabled.project).to receive(:merge_trains_enabled?).and_return(true)
+ end
+
+ it { is_expected.to eq false }
+ end
end
- context 'when merge trains are enabled' do
+ context 'when refactor_auto_merge flag is enabled' do
+ subject { service.available_for?(mr_merge_if_green_enabled) }
+
+ let(:feature_flag) { true }
+ let(:additional_checks_flag) { true }
+
before do
- allow(mr_merge_if_green_enabled.project).to receive(:merge_trains_enabled?).and_return(true)
+ stub_feature_flags(merge_when_checks_pass: feature_flag,
+ additional_merge_when_checks_ready: additional_checks_flag,
+ refactor_auto_merge: true)
+ end
+
+ context 'when immediately mergeable' do
+ context 'when a non active pipeline' do
+ before do
+ create(:ci_pipeline, :success,
+ ref: mr_merge_if_green_enabled.source_branch,
+ sha: mr_merge_if_green_enabled.diff_head_sha,
+ project: mr_merge_if_green_enabled.source_project)
+ mr_merge_if_green_enabled.update_head_pipeline
+ end
+
+ it { is_expected.to eq false }
+ end
+
+ context 'when an active pipeline' do
+ before do
+ create(:ci_pipeline, :running,
+ ref: mr_merge_if_green_enabled.source_branch,
+ sha: mr_merge_if_green_enabled.diff_head_sha,
+ project: mr_merge_if_green_enabled.source_project)
+ mr_merge_if_green_enabled.update_head_pipeline
+ end
+
+ it { is_expected.to eq true }
+ end
+ end
+
+ context 'when merge when checks pass flag is off' do
+ let(:feature_flag) { false }
+
+ it { is_expected.to eq false }
+ end
+
+ context 'when missing approvals' do
+ let(:approval_rule) do
+ create(:approval_merge_request_rule, merge_request: mr_merge_if_green_enabled,
+ approvals_required: approvals_required)
+ end
+
+ let(:approvals_required) { 1 }
+ let_it_be(:approver) { create(:user) }
+
+ before do
+ approval_rule.users << approver
+ end
+
+ it { is_expected.to eq true }
+
+ context 'when additional merge when checks flag is off' do
+ let(:additional_checks_flag) { false }
+
+ it { is_expected.to eq true }
+ end
+ end
+
+ context 'when draft status' do
+ before do
+ mr_merge_if_green_enabled.update!(title: 'Draft: check')
+ end
+
+ it { is_expected.to eq true }
+
+ context 'when additional merge when checks flag is off' do
+ let(:additional_checks_flag) { false }
+
+ it { is_expected.to eq false }
+ end
+ end
+
+ context 'when blocked status' do
+ before do
+ stub_licensed_features(blocking_merge_requests: true)
+ create(:merge_request_block, blocked_merge_request: mr_merge_if_green_enabled)
+ allow(mr_merge_if_green_enabled).to receive(:merge_blocked_by_other_mrs?).and_return(true)
+ end
+
+ it { is_expected.to eq true }
+
+ context 'when additional merge when checks flag is off' do
+ let(:additional_checks_flag) { false }
+
+ it { is_expected.to eq false }
+ end
end
- it { is_expected.to eq false }
+ context 'when discussions open' do
+ before do
+ allow(mr_merge_if_green_enabled).to receive(:mergeable_discussions_state?).and_return(false)
+ allow(mr_merge_if_green_enabled)
+ .to receive(:only_allow_merge_if_all_discussions_are_resolved?).and_return(true)
+ end
+
+ it { is_expected.to eq true }
+
+ context 'when additional merge when checks flag is off' do
+ let(:additional_checks_flag) { false }
+
+ it { is_expected.to eq false }
+ end
+ end
+
+ context 'when pipline is active' do
+ before do
+ create(:ci_pipeline, :running,
+ ref: mr_merge_if_green_enabled.source_branch,
+ sha: mr_merge_if_green_enabled.diff_head_sha,
+ project: mr_merge_if_green_enabled.source_project)
+
+ mr_merge_if_green_enabled.update_head_pipeline
+ end
+
+ it { is_expected.to eq true }
+
+ context 'when additional merge when checks flag is off' do
+ let(:additional_checks_flag) { false }
+
+ it { is_expected.to eq true }
+ end
+ end
+
+ context 'when the user does not have permission to merge' do
+ before do
+ allow(mr_merge_if_green_enabled).to receive(:can_be_merged_by?).and_return(false)
+ end
+
+ it { is_expected.to eq false }
+ end
+
+ context 'when merge trains are enabled' do
+ before do
+ allow(mr_merge_if_green_enabled.project).to receive(:merge_trains_enabled?).and_return(true)
+ end
+
+ it { is_expected.to eq false }
+ end
end
end
describe "#execute" do
- it_behaves_like 'auto_merge service #execute' do
- let(:auto_merge_strategy) { AutoMergeService::STRATEGY_MERGE_WHEN_CHECKS_PASS }
- let(:expected_note) do
- "enabled an automatic merge when all merge checks for #{pipeline.sha} pass"
- end
+ let(:merge_request) do
+ create(:merge_request, target_project: project, source_project: project,
+ source_branch: 'feature', target_branch: 'master')
+ end
+
+ context 'when the MR is available for auto merge' do
+ let(:auto_merge_strategy) { ::AutoMergeService::STRATEGY_MERGE_WHEN_CHECKS_PASS }
before do
- merge_request.update!(merge_params: { sha: pipeline.sha })
+ merge_request.update!(merge_params: { sha: pipeline.sha }, title: 'Draft: check')
end
- end
- context 'when no pipeline exists' do
- it_behaves_like 'auto_merge service #execute' do
- let(:pipeline) { nil }
- let(:auto_merge_strategy) { AutoMergeService::STRATEGY_MERGE_WHEN_CHECKS_PASS }
- let(:expected_note) do
- "enabled an automatic merge when all merge checks for 123456 pass"
+ context 'when first time enabling' do
+ before do
+ allow(merge_request)
+ .to receive_messages(head_pipeline: pipeline, diff_head_pipeline: pipeline)
+ allow(MailScheduler::NotificationServiceWorker).to receive(:perform_async)
+
+ service.execute(merge_request)
end
- before do
- merge_request.update!(merge_params: { sha: "123456" })
+ it 'sets the params, merge_user, and flag' do
+ expect(merge_request).to be_valid
+ expect(merge_request.merge_when_pipeline_succeeds).to be_truthy
+ expect(merge_request.merge_params).to include 'commit_message' => 'Awesome message'
+ expect(merge_request.merge_user).to be user
+ expect(merge_request.auto_merge_strategy).to eq auto_merge_strategy
+ end
+
+ it 'schedules a notification' do
+ expect(MailScheduler::NotificationServiceWorker).to have_received(:perform_async).with(
+ 'merge_when_pipeline_succeeds', merge_request, user).once
+ end
+
+ it 'creates a system note' do
+ pipeline = build(:ci_pipeline)
+ allow(merge_request).to receive(:diff_head_pipeline) { pipeline }
+
+ note = merge_request.notes.last
+ expect(note.note).to match "enabled an automatic merge when all merge checks for #{pipeline.sha} pass"
+ end
+ end
+
+ context 'when mergeable' do
+ it 'updates the merge params' do
+ expect(SystemNoteService).not_to receive(:merge_when_pipeline_succeeds)
+ expect(MailScheduler::NotificationServiceWorker).not_to receive(:perform_async).with(
+ 'merge_when_pipeline_succeeds', any_args)
+
+ service.execute(mr_merge_if_green_enabled)
end
end
end
diff --git a/ee/spec/services/merge_trains/add_merge_request_service_spec.rb b/ee/spec/services/merge_trains/add_merge_request_service_spec.rb
index 5b5acba9450301e0c9f17d3ea1788306da43ecc8..e852942e46d547b52d8974f6aa5ab99f66bd920a 100644
--- a/ee/spec/services/merge_trains/add_merge_request_service_spec.rb
+++ b/ee/spec/services/merge_trains/add_merge_request_service_spec.rb
@@ -14,7 +14,7 @@
create(:merge_request,
source_project: project, source_branch: 'feature',
target_project: project, target_branch: 'master',
- merge_status: 'unchecked')
+ merge_status: 'can_be_merged')
end
let(:service) { described_class.new(merge_request, user, params) }
diff --git a/spec/features/merge_request/user_resolves_wip_mr_spec.rb b/spec/features/merge_request/user_resolves_wip_mr_spec.rb
index dd1e73cf57f30680a627ff34a7ddcd7363ff2245..6dda5b606c43c4d9de9a40945d00a47872396739 100644
--- a/spec/features/merge_request/user_resolves_wip_mr_spec.rb
+++ b/spec/features/merge_request/user_resolves_wip_mr_spec.rb
@@ -30,10 +30,12 @@
end
context 'when there is active pipeline for merge request' do
+ let(:feature_flags_state) { true }
+
before do
- create(:ci_build, pipeline: pipeline)
+ stub_feature_flags(merge_when_checks_pass: feature_flags_state, merge_blocked_component: feature_flags_state)
- stub_feature_flags(merge_blocked_component: false)
+ create(:ci_build, pipeline: pipeline)
sign_in(user)
visit project_merge_request_path(project, merge_request)
@@ -41,8 +43,17 @@
end
it 'retains merge request data after clicking Resolve WIP status' do
+ # rubocop:disable RSpec/AvoidConditionalStatements -- remove when Auto merge goes to Foss
+ # https://gitlab.com/gitlab-org/gitlab/-/merge_requests/146730
+ expected_message = if Gitlab.ee?
+ "Set to auto-merge"
+ else
+ "Merge blocked:"
+ end
+ # rubocop:enable RSpec/AvoidConditionalStatements
+
expect(page.find('.ci-widget-content')).to have_content("Pipeline ##{pipeline.id}")
- expect(page).to have_content "Merge blocked: Select Mark as ready to remove it from Draft status."
+ expect(page).to have_content expected_message
page.within('.mr-state-widget') do
click_button('Mark as ready')
@@ -54,7 +65,28 @@
# merge request widget refreshes, which masks missing elements
# that should already be present.
expect(page.find('.ci-widget-content', wait: 0)).to have_content("Pipeline ##{pipeline.id}")
- expect(page).not_to have_content("Merge blocked: Select Mark as ready to remove it from Draft status.")
+ expect(page).to have_content("Set to auto-merge")
+ end
+
+ context 'when the new merge_when_checks_pass and merge blocked components are disabled' do
+ let(:feature_flags_state) { false }
+
+ it 'retains merge request data after clicking Resolve WIP status' do
+ expect(page.find('.ci-widget-content')).to have_content("Pipeline ##{pipeline.id}")
+ expect(page).to have_content "Merge blocked: Select Mark as ready to remove it from Draft status."
+
+ page.within('.mr-state-widget') do
+ click_button('Mark as ready')
+ end
+
+ wait_for_requests
+
+ # If we don't disable the wait here, the test will wait until the
+ # merge request widget refreshes, which masks missing elements
+ # that should already be present.
+ expect(page.find('.ci-widget-content', wait: 0)).to have_content("Pipeline ##{pipeline.id}")
+ expect(page).not_to have_content("Merge blocked")
+ end
end
end
end
diff --git a/spec/features/merge_request/user_sees_merge_widget_spec.rb b/spec/features/merge_request/user_sees_merge_widget_spec.rb
index 5448d6867cb0a1991ed15e317d05e8d12f5ddd93..0928394b5710a44a12533d5d6449b800e3bd83e3 100644
--- a/spec/features/merge_request/user_sees_merge_widget_spec.rb
+++ b/spec/features/merge_request/user_sees_merge_widget_spec.rb
@@ -331,12 +331,31 @@ def click_expand_button
visit project_merge_request_path(project_only_mwps, merge_request_in_only_mwps_project)
end
- it 'is allowed to merge' do
- # Wait for the `ci_status` and `merge_check` requests
- wait_for_requests
+ context 'when using merge when pipeline succeeds' do
+ before do
+ stub_feature_flags(merge_when_checks_pass: false)
+ end
+
+ it 'is not allowed to merge' do
+ # Wait for the `ci_status` and `merge_check` requests
+ wait_for_requests
- expect(page).not_to have_selector('.accept-merge-request')
+ expect(page).not_to have_selector('.accept-merge-request')
+ end
end
+ # TODO When moving merge when checks pass to FOSS
+ # context 'when using merge when checks pass' do
+ # before do
+ # stub_feature_flags(merge_when_checks_pass: true)
+ # end
+ #
+ # it 'is not allowed to set auto merge' do
+ # # Wait for the `ci_status` and `merge_check` requests
+ # wait_for_requests
+ #
+ # expect(page).to have_selector('.accept-merge-request')
+ # end
+ # end
end
context 'view merge request with MWPS enabled but automatically merge fails' do
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index a0b887655b2693b0dc6a303d5e240b414ed4b1a4..9b81144cf4fe615d33aa3bdfa7d0088edc849e6d 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -3627,15 +3627,40 @@ def set_compare(merge_request)
describe '#skipped_mergeable_checks' do
subject { build_stubbed(:merge_request).skipped_mergeable_checks(options) }
+ let(:feature_flag) { true }
+
+ before do
+ stub_feature_flags(additional_merge_when_checks_ready: feature_flag)
+ end
+
where(:options, :skip_ci_check) do
{} | false
{ auto_merge_requested: false } | false
{ auto_merge_requested: true } | true
end
-
with_them do
it { is_expected.to include(skip_ci_check: skip_ci_check) }
end
+
+ context 'when auto_merge_requested is true' do
+ let(:options) { { auto_merge_requested: true, auto_merge_strategy: auto_merge_strategy } }
+
+ where(:auto_merge_strategy, :skip_approved_check, :skip_draft_check, :skip_blocked_check,
+ :skip_discussions_check, :skip_external_status_check, :feature_flag) do
+ '' | false | false | false | false | false | true
+ AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS | false | false | false | false | false | true
+ AutoMergeService::STRATEGY_MERGE_WHEN_CHECKS_PASS | true | true | true | true | true | true
+ AutoMergeService::STRATEGY_MERGE_WHEN_CHECKS_PASS | true | false | false | false | false | false
+ end
+
+ with_them do
+ it do
+ is_expected.to include(skip_approved_check: skip_approved_check, skip_draft_check: skip_draft_check,
+ skip_blocked_check: skip_blocked_check, skip_discussions_check: skip_discussions_check,
+ skip_external_status_check: skip_external_status_check)
+ end
+ end
+ end
end
describe '#check_mergeability' do
@@ -6252,6 +6277,26 @@ def transition!
end
end
+ describe '#mergeability_checks_pass?' do
+ let(:merge_request) { build_stubbed(:merge_request) }
+ let(:result) { instance_double(ServiceResponse, success?: { results: ['result'] }) }
+
+ it 'executes MergeRequests::Mergeability::RunChecksService with all mergeability checks and returns a boolean' do
+ expect_next_instance_of(
+ MergeRequests::Mergeability::RunChecksService,
+ merge_request: merge_request,
+ params: {}
+ ) do |svc|
+ expect(svc)
+ .to receive(:execute)
+ .with(described_class.all_mergeability_checks, execute_all: false)
+ .and_return(result)
+ end
+
+ expect(merge_request.mergeability_checks_pass?).to be_truthy
+ end
+ end
+
describe '#only_allow_merge_if_pipeline_succeeds?' do
let(:merge_request) { build_stubbed(:merge_request) }
diff --git a/spec/services/auto_merge/base_service_spec.rb b/spec/services/auto_merge/base_service_spec.rb
index f4b3430200c105d34763214654c1aef086bceb3f..5d3ce64898424c3ff5290e312f869fd5f44adb3a 100644
--- a/spec/services/auto_merge/base_service_spec.rb
+++ b/spec/services/auto_merge/base_service_spec.rb
@@ -305,22 +305,65 @@ def abort_with_error_in_yield
describe '#available_for?' do
using RSpec::Parameterized::TableSyntax
- subject(:available_for) { service.available_for?(merge_request) { true } }
+ subject(:available_for) { service.available_for?(merge_request) { yield_result } }
let(:merge_request) { create(:merge_request) }
+ let(:yield_result) { true }
+ let(:checks_pass) { true }
+ let(:can_be_merged) { true }
- where(:can_be_merged, :open, :broken, :discussions, :blocked, :draft, :result) do
- true | true | false | true | false | false | true
- false | true | false | true | false | false | false
- true | false | false | true | false | false | false
- true | true | true | true | false | false | false
- true | true | false | false | false | false | false
- true | true | false | true | true | false | false
- true | true | false | true | false | true | false
+ context 'when can_be_merged is true' do
+ before do
+ allow(merge_request).to receive(:can_be_merged_by?).and_return(can_be_merged)
+ allow(merge_request).to receive(:mergeability_checks_pass?).and_return(checks_pass)
+ end
+
+ context 'when the mergeabilty checks pass is true' do
+ context 'when the yield is true' do
+ it 'returns true' do
+ expect(available_for).to be_truthy
+ end
+ end
+
+ context 'when the yield is false' do
+ let(:yield_result) { false }
+
+ it 'returns false' do
+ expect(available_for).to be_falsey
+ end
+ end
+ end
+
+ context 'when the mergeabilty checks pass is false' do
+ let(:checks_pass) { false }
+
+ context 'when the yield is true' do
+ it 'returns false' do
+ expect(available_for).to be_falsey
+ end
+ end
+
+ context 'when the yield is false' do
+ let(:yield_result) { false }
+
+ it 'returns false' do
+ expect(available_for).to be_falsey
+ end
+ end
+ end
end
- with_them do
+ context 'when can_be_merged is false' do
+ let(:can_be_merged) { false }
+
+ it 'returns false' do
+ expect(available_for).to be_falsey
+ end
+ end
+
+ context 'when refactor_auto_merge is disabled' do
before do
+ stub_feature_flags(refactor_auto_merge: false)
allow(merge_request).to receive(:can_be_merged_by?).and_return(can_be_merged)
allow(merge_request).to receive(:open?).and_return(open)
allow(merge_request).to receive(:broken?).and_return(broken)
@@ -329,8 +372,20 @@ def abort_with_error_in_yield
allow(merge_request).to receive(:merge_blocked_by_other_mrs?).and_return(blocked)
end
- it 'returns the expected results' do
- expect(available_for).to eq(result)
+ where(:can_be_merged, :open, :broken, :discussions, :blocked, :draft, :result) do
+ true | true | false | true | false | false | true
+ false | true | false | true | false | false | false
+ true | false | false | true | false | false | false
+ true | true | true | true | false | false | false
+ true | true | false | false | false | false | false
+ true | true | false | true | true | false | false
+ true | true | false | true | false | true | false
+ end
+
+ with_them do
+ it 'returns the expected results' do
+ expect(available_for).to eq(result)
+ end
end
end
end
diff --git a/spec/services/merge_requests/merge_orchestration_service_spec.rb b/spec/services/merge_requests/merge_orchestration_service_spec.rb
index 7e6eeec2a2d13e4377e27f132faa4365a7151e12..faec5ac2fde5b6fb58051048081bbdc642282da9 100644
--- a/spec/services/merge_requests/merge_orchestration_service_spec.rb
+++ b/spec/services/merge_requests/merge_orchestration_service_spec.rb
@@ -102,7 +102,7 @@
context 'when merge request is not mergeable' do
before do
- allow(merge_request).to receive(:mergeable?) { false }
+ merge_request.update!(merge_status: 'cannot_be_merged')
end
it { is_expected.to eq(false) }
diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb
index e253c06125faf784adccfc0fb4e262cf86e9494f..aee39f8beb20ecb5ec41d7d2af17075d0865e3d9 100644
--- a/spec/services/merge_requests/update_service_spec.rb
+++ b/spec/services/merge_requests/update_service_spec.rb
@@ -553,7 +553,13 @@ def update_merge_request(opts)
head_pipeline_of: merge_request
)
- expect(AutoMerge::MergeWhenPipelineSucceedsService).to receive(:new).with(project, user, { sha: merge_request.diff_head_sha })
+ expected_class = if Gitlab.ee?
+ AutoMerge::MergeWhenChecksPassService
+ else
+ AutoMerge::MergeWhenPipelineSucceedsService
+ end
+
+ expect(expected_class).to receive(:new).with(project, user, { sha: merge_request.diff_head_sha })
.and_return(service_mock)
allow(service_mock).to receive(:available_for?) { true }
expect(service_mock).to receive(:execute).with(merge_request)