Commit e495db2f authored by Bob Van Landuyt's avatar Bob Van Landuyt

Merge branch '26293-prevent-merge-if-pipeline-has-failed' into 'master'

Update merge-when-pipeline-succeeds behavior on API

See merge request gitlab-org/gitlab!19641
parents 2bb32cd2 33de4153
...@@ -241,6 +241,9 @@ class MergeRequest < ApplicationRecord ...@@ -241,6 +241,9 @@ class MergeRequest < ApplicationRecord
alias_attribute :auto_merge_enabled, :merge_when_pipeline_succeeds alias_attribute :auto_merge_enabled, :merge_when_pipeline_succeeds
alias_method :issuing_parent, :target_project alias_method :issuing_parent, :target_project
delegate :active?, to: :head_pipeline, prefix: true, allow_nil: true
delegate :success?, to: :actual_head_pipeline, prefix: true, allow_nil: true
RebaseLockTimeout = Class.new(StandardError) RebaseLockTimeout = Class.new(StandardError)
REBASE_LOCK_MESSAGE = _("Failed to enqueue the rebase operation, possibly due to a long-lived transaction. Try again later.") REBASE_LOCK_MESSAGE = _("Failed to enqueue the rebase operation, possibly due to a long-lived transaction. Try again later.")
......
...@@ -11,7 +11,7 @@ module AutoMerge ...@@ -11,7 +11,7 @@ module AutoMerge
end end
def process(merge_request) def process(merge_request)
return unless merge_request.actual_head_pipeline&.success? return unless merge_request.actual_head_pipeline_success?
return unless merge_request.mergeable? return unless merge_request.mergeable?
merge_request.merge_async(merge_request.merge_user_id, merge_request.merge_params) merge_request.merge_async(merge_request.merge_user_id, merge_request.merge_params)
......
...@@ -87,7 +87,7 @@ module MergeRequests ...@@ -87,7 +87,7 @@ module MergeRequests
merge_request.update(merge_error: nil) merge_request.update(merge_error: nil)
if merge_request.head_pipeline && merge_request.head_pipeline.active? if merge_request.head_pipeline_active?
AutoMergeService.new(project, current_user, { sha: last_diff_sha }).execute(merge_request, AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS) AutoMergeService.new(project, current_user, { sha: last_diff_sha }).execute(merge_request, AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS)
else else
merge_request.merge_async(current_user.id, { sha: last_diff_sha }) merge_request.merge_async(current_user.id, { sha: last_diff_sha })
......
---
title: Update merging an MR behavior on the API when pipeline fails
merge_request: 19641
author: briankabiro
type: fixed
...@@ -9,7 +9,7 @@ module AutoMerge ...@@ -9,7 +9,7 @@ module AutoMerge
end end
def process(merge_request) def process(merge_request)
return unless merge_request.actual_head_pipeline&.success? return unless merge_request.actual_head_pipeline_success?
merge_train_service = AutoMerge::MergeTrainService.new(project, merge_request.merge_user) merge_train_service = AutoMerge::MergeTrainService.new(project, merge_request.merge_user)
......
...@@ -68,6 +68,10 @@ module API ...@@ -68,6 +68,10 @@ module API
end end
end end
def not_automatically_mergeable?(merge_when_pipeline_succeeds, merge_request)
merge_when_pipeline_succeeds && !merge_request.head_pipeline_active? && !merge_request.actual_head_pipeline_success?
end
def serializer_options_for(merge_requests) def serializer_options_for(merge_requests)
options = { with: Entities::MergeRequestBasic, current_user: current_user } options = { with: Entities::MergeRequestBasic, current_user: current_user }
...@@ -391,12 +395,13 @@ module API ...@@ -391,12 +395,13 @@ module API
merge_request = find_project_merge_request(params[:merge_request_iid]) merge_request = find_project_merge_request(params[:merge_request_iid])
merge_when_pipeline_succeeds = to_boolean(params[:merge_when_pipeline_succeeds]) merge_when_pipeline_succeeds = to_boolean(params[:merge_when_pipeline_succeeds])
not_automatically_mergeable = not_automatically_mergeable?(merge_when_pipeline_succeeds, merge_request)
# Merge request can not be merged # Merge request can not be merged
# because user dont have permissions to push into target branch # because user dont have permissions to push into target branch
unauthorized! unless merge_request.can_be_merged_by?(current_user) unauthorized! unless merge_request.can_be_merged_by?(current_user)
not_allowed! unless merge_request.mergeable_state?(skip_ci_check: merge_when_pipeline_succeeds) not_allowed! if !merge_request.mergeable_state?(skip_ci_check: merge_when_pipeline_succeeds) || not_automatically_mergeable
render_api_error!('Branch cannot be merged', 406) unless merge_request.mergeable?(skip_ci_check: merge_when_pipeline_succeeds) render_api_error!('Branch cannot be merged', 406) unless merge_request.mergeable?(skip_ci_check: merge_when_pipeline_succeeds)
...@@ -411,7 +416,7 @@ module API ...@@ -411,7 +416,7 @@ module API
sha: params[:sha] || merge_request.diff_head_sha sha: params[:sha] || merge_request.diff_head_sha
) )
if merge_when_pipeline_succeeds && merge_request.head_pipeline && merge_request.head_pipeline.active? if merge_when_pipeline_succeeds
AutoMergeService.new(merge_request.target_project, current_user, merge_params) AutoMergeService.new(merge_request.target_project, current_user, merge_params)
.execute(merge_request, AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS) .execute(merge_request, AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS)
else else
......
...@@ -2326,6 +2326,26 @@ describe MergeRequest do ...@@ -2326,6 +2326,26 @@ describe MergeRequest do
end end
end end
describe "#head_pipeline_active? " do
it do
is_expected
.to delegate_method(:active?)
.to(:head_pipeline)
.with_prefix
.with_arguments(allow_nil: true)
end
end
describe "#actual_head_pipeline_success? " do
it do
is_expected
.to delegate_method(:success?)
.to(:actual_head_pipeline)
.with_prefix
.with_arguments(allow_nil: true)
end
end
describe '#mergeable_ci_state?' do describe '#mergeable_ci_state?' do
let(:project) { create(:project, only_allow_merge_if_pipeline_succeeds: true) } let(:project) { create(:project, only_allow_merge_if_pipeline_succeeds: true) }
let(:pipeline) { create(:ci_empty_pipeline) } let(:pipeline) { create(:ci_empty_pipeline) }
......
...@@ -1567,6 +1567,18 @@ describe API::MergeRequests do ...@@ -1567,6 +1567,18 @@ describe API::MergeRequests do
expect(response).to have_gitlab_http_status(200) expect(response).to have_gitlab_http_status(200)
end end
it 'does not merge if merge_when_pipeline_succeeds is passed and the pipeline has failed' do
create(:ci_pipeline,
:failed,
sha: merge_request.diff_head_sha,
merge_requests_as_head_pipeline: [merge_request])
put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/merge", user), params: { merge_when_pipeline_succeeds: true }
expect(response).to have_gitlab_http_status(405)
expect(merge_request.reload.state).to eq('opened')
end
it "enables merge when pipeline succeeds if the pipeline is active" do it "enables merge when pipeline succeeds if the pipeline is active" do
allow_any_instance_of(MergeRequest).to receive_messages(head_pipeline: pipeline, actual_head_pipeline: pipeline) allow_any_instance_of(MergeRequest).to receive_messages(head_pipeline: pipeline, actual_head_pipeline: pipeline)
allow(pipeline).to receive(:active?).and_return(true) allow(pipeline).to receive(:active?).and_return(true)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment