Automatically update MR merge-ref along merge status
This couples the code that transitions the `MergeRequest#merge_status` and refs/merge-requests/:iid/merge ref update. In general, instead of directly telling `MergeToRefService` to update the merge ref, we should rely on `MergeabilityCheckService` to keep both the merge status and merge ref synced. Now, if the merge_status is `can_be_merged` it means the merge-ref is also updated to the latest. We've also updated the logic to be more systematic and less user-based.
This commit is contained in:
Oswaldo Ferreira
parent
c6eb18ee0f
commit
3af348b6cf
18 changed files with 538 additions and 219 deletions
|
|
@ -1 +1 @@
|
|||
1.47.0
|
||||
1.48.0
|
||||
|
|
|
|||
|
|
@ -33,7 +33,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
|
|||
|
||||
def show
|
||||
close_merge_request_if_no_source_project
|
||||
mark_merge_request_mergeable
|
||||
@merge_request.check_mergeability
|
||||
|
||||
respond_to do |format|
|
||||
format.html do
|
||||
|
|
@ -251,10 +251,6 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
|
|||
@merge_request.has_no_commits? && !@merge_request.target_branch_exists?
|
||||
end
|
||||
|
||||
def mark_merge_request_mergeable
|
||||
@merge_request.check_if_can_be_merged
|
||||
end
|
||||
|
||||
def merge!
|
||||
# Disable the CI check if auto_merge_strategy is specified since we have
|
||||
# to wait until CI completes to know
|
||||
|
|
|
|||
|
|
@ -231,6 +231,10 @@ class MergeRequest < ApplicationRecord
|
|||
end
|
||||
end
|
||||
|
||||
def merge_ref_auto_sync_enabled?
|
||||
Feature.enabled?(:merge_ref_auto_sync, project, default_enabled: true)
|
||||
end
|
||||
|
||||
# Use this method whenever you need to make sure the head_pipeline is synced with the
|
||||
# branch head commit, for example checking if a merge request can be merged.
|
||||
# For more information check: https://gitlab.com/gitlab-org/gitlab-ce/issues/40004
|
||||
|
|
@ -725,19 +729,16 @@ class MergeRequest < ApplicationRecord
|
|||
|
||||
MergeRequests::ReloadDiffsService.new(self, current_user).execute
|
||||
end
|
||||
|
||||
def check_mergeability
|
||||
MergeRequests::MergeabilityCheckService.new(self).execute
|
||||
end
|
||||
# rubocop: enable CodeReuse/ServiceClass
|
||||
|
||||
def check_if_can_be_merged
|
||||
return unless self.class.state_machines[:merge_status].check_state?(merge_status) && Gitlab::Database.read_write?
|
||||
|
||||
can_be_merged =
|
||||
!broken? && project.repository.can_be_merged?(diff_head_sha, target_branch)
|
||||
|
||||
if can_be_merged
|
||||
mark_as_mergeable
|
||||
else
|
||||
mark_as_unmergeable
|
||||
end
|
||||
# Returns boolean indicating the merge_status should be rechecked in order to
|
||||
# switch to either can_be_merged or cannot_be_merged.
|
||||
def recheck_merge_status?
|
||||
self.class.state_machines[:merge_status].check_state?(merge_status)
|
||||
end
|
||||
|
||||
def merge_event
|
||||
|
|
@ -763,7 +764,7 @@ class MergeRequest < ApplicationRecord
|
|||
def mergeable?(skip_ci_check: false)
|
||||
return false unless mergeable_state?(skip_ci_check: skip_ci_check)
|
||||
|
||||
check_if_can_be_merged
|
||||
check_mergeability
|
||||
|
||||
can_be_merged? && !should_be_rebased?
|
||||
end
|
||||
|
|
@ -778,15 +779,6 @@ class MergeRequest < ApplicationRecord
|
|||
true
|
||||
end
|
||||
|
||||
def mergeable_to_ref?
|
||||
return false unless mergeable_state?(skip_ci_check: true, skip_discussions_check: true)
|
||||
|
||||
# Given the `merge_ref_path` will have the same
|
||||
# state the `target_branch` would have. Ideally
|
||||
# we need to check if it can be merged to it.
|
||||
project.repository.can_be_merged?(diff_head_sha, target_branch)
|
||||
end
|
||||
|
||||
def ff_merge_possible?
|
||||
project.repository.ancestor?(target_branch_sha, diff_head_sha)
|
||||
end
|
||||
|
|
@ -1099,6 +1091,12 @@ class MergeRequest < ApplicationRecord
|
|||
target_project.repository.fetch_source_branch!(source_project.repository, source_branch, ref_path)
|
||||
end
|
||||
|
||||
# Returns the current merge-ref HEAD commit.
|
||||
#
|
||||
def merge_ref_head
|
||||
project.repository.commit(merge_ref_path)
|
||||
end
|
||||
|
||||
def ref_path
|
||||
"refs/#{Repository::REF_MERGE_REQUEST}/#{iid}/head"
|
||||
end
|
||||
|
|
|
|||
|
|
@ -11,6 +11,8 @@ module MergeRequests
|
|||
# be executed regardless of the `target_ref` current state).
|
||||
#
|
||||
class MergeToRefService < MergeRequests::MergeBaseService
|
||||
extend ::Gitlab::Utils::Override
|
||||
|
||||
def execute(merge_request)
|
||||
@merge_request = merge_request
|
||||
|
||||
|
|
@ -26,14 +28,18 @@ module MergeRequests
|
|||
success(commit_id: commit.id,
|
||||
target_id: target_id,
|
||||
source_id: source_id)
|
||||
rescue MergeError => error
|
||||
rescue MergeError, ArgumentError => error
|
||||
error(error.message)
|
||||
end
|
||||
|
||||
private
|
||||
|
||||
override :source
|
||||
def source
|
||||
merge_request.diff_head_sha
|
||||
end
|
||||
|
||||
def validate!
|
||||
authorization_check!
|
||||
error_check!
|
||||
end
|
||||
|
||||
|
|
@ -43,21 +49,13 @@ module MergeRequests
|
|||
error =
|
||||
if !hooks_validation_pass?(merge_request)
|
||||
hooks_validation_error(merge_request)
|
||||
elsif !@merge_request.mergeable_to_ref?
|
||||
"Merge request is not mergeable to #{target_ref}"
|
||||
elsif !source
|
||||
elsif source.blank?
|
||||
'No source for merge'
|
||||
end
|
||||
|
||||
raise_error(error) if error
|
||||
end
|
||||
|
||||
def authorization_check!
|
||||
unless Ability.allowed?(current_user, :admin_merge_request, project)
|
||||
raise_error("You are not allowed to merge to this ref")
|
||||
end
|
||||
end
|
||||
|
||||
def target_ref
|
||||
merge_request.merge_ref_path
|
||||
end
|
||||
|
|
|
|||
96
app/services/merge_requests/mergeability_check_service.rb
Normal file
96
app/services/merge_requests/mergeability_check_service.rb
Normal file
|
|
@ -0,0 +1,96 @@
|
|||
# frozen_string_literal: true
|
||||
|
||||
module MergeRequests
|
||||
class MergeabilityCheckService < ::BaseService
|
||||
include Gitlab::Utils::StrongMemoize
|
||||
|
||||
delegate :project, :merge_ref_auto_sync_enabled?, to: :@merge_request
|
||||
delegate :repository, to: :project
|
||||
|
||||
def initialize(merge_request)
|
||||
@merge_request = merge_request
|
||||
end
|
||||
|
||||
# Updates the MR merge_status. Whenever it switches to a can_be_merged state,
|
||||
# the merge-ref is refreshed.
|
||||
#
|
||||
# recheck - When given, it'll enforce a merge-ref refresh if the current merge_status is
|
||||
# can_be_merged or cannot_be_merged.
|
||||
# Given MergeRequests::RefreshService is called async, it might happen that the target
|
||||
# branch gets updated, but the MergeRequest#merge_status lags behind. So in scenarios
|
||||
# where we need the current state of the merge ref in repository, the `recheck`
|
||||
# argument is required.
|
||||
#
|
||||
# Returns a ServiceResponse indicating merge_status is/became can_be_merged
|
||||
# and the merge-ref is synced. Success in case of being/becoming mergeable,
|
||||
# error otherwise.
|
||||
def execute(recheck: false)
|
||||
return ServiceResponse.error(message: 'Invalid argument') unless merge_request
|
||||
return ServiceResponse.error(message: 'Unsupported operation') if Gitlab::Database.read_only?
|
||||
|
||||
recheck! if recheck
|
||||
update_merge_status
|
||||
|
||||
unless merge_request.can_be_merged?
|
||||
return ServiceResponse.error(message: 'Merge request is not mergeable')
|
||||
end
|
||||
|
||||
unless merge_ref_auto_sync_enabled?
|
||||
return ServiceResponse.error(message: 'Merge ref is outdated due to disabled feature')
|
||||
end
|
||||
|
||||
ServiceResponse.success(payload: payload)
|
||||
end
|
||||
|
||||
private
|
||||
|
||||
attr_reader :merge_request
|
||||
|
||||
def payload
|
||||
strong_memoize(:payload) do
|
||||
{
|
||||
merge_ref_head: merge_ref_head_payload
|
||||
}
|
||||
end
|
||||
end
|
||||
|
||||
def merge_ref_head_payload
|
||||
commit = merge_request.merge_ref_head
|
||||
|
||||
return unless commit
|
||||
|
||||
target_id, source_id = commit.parent_ids
|
||||
|
||||
{
|
||||
commit_id: commit.id,
|
||||
source_id: source_id,
|
||||
target_id: target_id
|
||||
}
|
||||
end
|
||||
|
||||
def update_merge_status
|
||||
return unless merge_request.recheck_merge_status?
|
||||
|
||||
if can_git_merge? && merge_to_ref
|
||||
merge_request.mark_as_mergeable
|
||||
else
|
||||
merge_request.mark_as_unmergeable
|
||||
end
|
||||
end
|
||||
|
||||
def recheck!
|
||||
merge_request.mark_as_unchecked unless merge_request.recheck_merge_status?
|
||||
end
|
||||
|
||||
def can_git_merge?
|
||||
!merge_request.broken? && repository.can_be_merged?(merge_request.diff_head_sha, merge_request.target_branch)
|
||||
end
|
||||
|
||||
def merge_to_ref
|
||||
return true unless merge_ref_auto_sync_enabled?
|
||||
|
||||
result = MergeRequests::MergeToRefService.new(project, merge_request.author).execute(merge_request)
|
||||
result[:status] == :success
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
@ -1,19 +1,20 @@
|
|||
# frozen_string_literal: true
|
||||
|
||||
class ServiceResponse
|
||||
def self.success(message: nil)
|
||||
new(status: :success, message: message)
|
||||
def self.success(message: nil, payload: {})
|
||||
new(status: :success, message: message, payload: payload)
|
||||
end
|
||||
|
||||
def self.error(message:, http_status: nil)
|
||||
new(status: :error, message: message, http_status: http_status)
|
||||
def self.error(message:, payload: {}, http_status: nil)
|
||||
new(status: :error, message: message, payload: payload, http_status: http_status)
|
||||
end
|
||||
|
||||
attr_reader :status, :message, :http_status
|
||||
attr_reader :status, :message, :http_status, :payload
|
||||
|
||||
def initialize(status:, message: nil, http_status: nil)
|
||||
def initialize(status:, message: nil, payload: {}, http_status: nil)
|
||||
self.status = status
|
||||
self.message = message
|
||||
self.payload = payload
|
||||
self.http_status = http_status
|
||||
end
|
||||
|
||||
|
|
@ -27,5 +28,5 @@ class ServiceResponse
|
|||
|
||||
private
|
||||
|
||||
attr_writer :status, :message, :http_status
|
||||
attr_writer :status, :message, :http_status, :payload
|
||||
end
|
||||
|
|
|
|||
|
|
@ -0,0 +1,5 @@
|
|||
---
|
||||
title: Sync merge ref upon mergeability check
|
||||
merge_request: 28513
|
||||
author:
|
||||
type: added
|
||||
|
|
@ -0,0 +1,25 @@
|
|||
# frozen_string_literal: true
|
||||
|
||||
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
|
||||
# for more information on how to write migrations for GitLab.
|
||||
|
||||
class EnqueueResetMergeStatusSecondRun < ActiveRecord::Migration[5.1]
|
||||
include Gitlab::Database::MigrationHelpers
|
||||
|
||||
DOWNTIME = false
|
||||
BATCH_SIZE = 10_000
|
||||
MIGRATION = 'ResetMergeStatus'
|
||||
DELAY_INTERVAL = 5.minutes.to_i
|
||||
|
||||
disable_ddl_transaction!
|
||||
|
||||
def up
|
||||
say 'Scheduling `ResetMergeStatus` jobs'
|
||||
|
||||
# We currently have more than ~5_000_000 merge request records on GitLab.com.
|
||||
# This means it'll schedule ~500 jobs (10k MRs each) with a 5 minutes gap,
|
||||
# so this should take ~41 hours for all background migrations to complete.
|
||||
# ((5_000_000 / 10_000) * 5) / 60 => 41.6666..
|
||||
queue_background_migration_jobs_by_range_at_intervals(MergeRequest, MIGRATION, DELAY_INTERVAL, batch_size: BATCH_SIZE)
|
||||
end
|
||||
end
|
||||
|
|
@ -10,7 +10,7 @@
|
|||
#
|
||||
# It's strongly recommended that you check this file into your version control system.
|
||||
|
||||
ActiveRecord::Schema.define(version: 20190619175843) do
|
||||
ActiveRecord::Schema.define(version: 20190620112608) do
|
||||
|
||||
# These are extensions that must be enabled in order to support this database
|
||||
enable_extension "plpgsql"
|
||||
|
|
|
|||
|
|
@ -1191,33 +1191,29 @@ Parameters:
|
|||
}
|
||||
```
|
||||
|
||||
## Merge to default merge ref path
|
||||
## Returns the up to date merge-ref HEAD commit
|
||||
|
||||
Merge the changes between the merge request source and target branches into `refs/merge-requests/:iid/merge`
|
||||
ref, of the target project repository. This ref will have the state the target branch would have if
|
||||
ref, of the target project repository, if possible. This ref will have the state the target branch would have if
|
||||
a regular merge action was taken.
|
||||
|
||||
This is not a regular merge action given it doesn't change the merge request state in any manner.
|
||||
This is not a regular merge action given it doesn't change the merge request target branch state in any manner.
|
||||
|
||||
This ref (`refs/merge-requests/:iid/merge`) is **always** overwritten when submitting
|
||||
requests to this API, so none of its state is kept or used in the process.
|
||||
This ref (`refs/merge-requests/:iid/merge`) isn't necessarily overwritten when submitting
|
||||
requests to this API, though it'll make sure the ref has the latest possible state.
|
||||
|
||||
If the merge request has conflicts, is empty or already merged,
|
||||
you'll get a `400` and a descriptive error message. If you don't have permissions to do so,
|
||||
you'll get a `403`.
|
||||
If the merge request has conflicts, is empty or already merged, you'll get a `400` and a descriptive error message.
|
||||
|
||||
It returns the HEAD commit of `refs/merge-requests/:iid/merge` in the response body in
|
||||
case of `200`.
|
||||
It returns the HEAD commit of `refs/merge-requests/:iid/merge` in the response body in case of `200`.
|
||||
|
||||
```
|
||||
PUT /projects/:id/merge_requests/:merge_request_iid/merge_to_ref
|
||||
GET /projects/:id/merge_requests/:merge_request_iid/merge_ref
|
||||
```
|
||||
|
||||
Parameters:
|
||||
|
||||
- `id` (required) - The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user
|
||||
- `merge_request_iid` (required) - Internal ID of MR
|
||||
- `merge_commit_message` (optional) - Custom merge commit message
|
||||
|
||||
```json
|
||||
{
|
||||
|
|
|
|||
|
|
@ -703,7 +703,7 @@ module API
|
|||
# See https://gitlab.com/gitlab-org/gitlab-ce/issues/42344 for more
|
||||
# information.
|
||||
expose :merge_status do |merge_request|
|
||||
merge_request.check_if_can_be_merged
|
||||
merge_request.check_mergeability
|
||||
merge_request.merge_status
|
||||
end
|
||||
expose :diff_head_sha, as: :sha
|
||||
|
|
|
|||
|
|
@ -397,28 +397,16 @@ module API
|
|||
present merge_request, with: Entities::MergeRequest, current_user: current_user, project: user_project
|
||||
end
|
||||
|
||||
desc 'Merge a merge request to its default temporary merge ref path'
|
||||
params do
|
||||
optional :merge_commit_message, type: String, desc: 'Custom merge commit message'
|
||||
end
|
||||
put ':id/merge_requests/:merge_request_iid/merge_to_ref' do
|
||||
desc 'Returns the up to date merge-ref HEAD commit'
|
||||
get ':id/merge_requests/:merge_request_iid/merge_ref' do
|
||||
merge_request = find_project_merge_request(params[:merge_request_iid])
|
||||
|
||||
authorize! :admin_merge_request, user_project
|
||||
result = ::MergeRequests::MergeabilityCheckService.new(merge_request).execute(recheck: true)
|
||||
|
||||
merge_params = {
|
||||
commit_message: params[:merge_commit_message]
|
||||
}
|
||||
|
||||
result = ::MergeRequests::MergeToRefService
|
||||
.new(merge_request.target_project, current_user, merge_params)
|
||||
.execute(merge_request)
|
||||
|
||||
if result[:status] == :success
|
||||
present result.slice(:commit_id), 200
|
||||
if result.success?
|
||||
present :commit_id, result.payload.dig(:merge_ref_head, :commit_id)
|
||||
else
|
||||
http_status = result[:http_status] || 400
|
||||
render_api_error!(result[:message], http_status)
|
||||
render_api_error!(result.message, 400)
|
||||
end
|
||||
end
|
||||
|
||||
|
|
|
|||
|
|
@ -0,0 +1,52 @@
|
|||
# frozen_string_literal: true
|
||||
|
||||
require 'spec_helper'
|
||||
require Rails.root.join('db', 'post_migrate', '20190620112608_enqueue_reset_merge_status_second_run.rb')
|
||||
|
||||
describe EnqueueResetMergeStatusSecondRun, :migration, :sidekiq do
|
||||
let(:namespaces) { table(:namespaces) }
|
||||
let(:projects) { table(:projects) }
|
||||
let(:namespace) { namespaces.create(name: 'gitlab', path: 'gitlab-org') }
|
||||
let(:project) { projects.create(namespace_id: namespace.id, name: 'foo') }
|
||||
let(:merge_requests) { table(:merge_requests) }
|
||||
|
||||
def create_merge_request(id, extra_params = {})
|
||||
params = {
|
||||
id: id,
|
||||
target_project_id: project.id,
|
||||
target_branch: 'master',
|
||||
source_project_id: project.id,
|
||||
source_branch: 'mr name',
|
||||
title: "mr name#{id}"
|
||||
}.merge(extra_params)
|
||||
|
||||
merge_requests.create!(params)
|
||||
end
|
||||
|
||||
it 'correctly schedules background migrations' do
|
||||
create_merge_request(1, state: 'opened', merge_status: 'can_be_merged')
|
||||
create_merge_request(2, state: 'opened', merge_status: 'can_be_merged')
|
||||
create_merge_request(3, state: 'opened', merge_status: 'can_be_merged')
|
||||
create_merge_request(4, state: 'merged', merge_status: 'can_be_merged')
|
||||
create_merge_request(5, state: 'opened', merge_status: 'unchecked')
|
||||
|
||||
stub_const("#{described_class.name}::BATCH_SIZE", 2)
|
||||
|
||||
Sidekiq::Testing.fake! do
|
||||
Timecop.freeze do
|
||||
migrate!
|
||||
|
||||
expect(described_class::MIGRATION)
|
||||
.to be_scheduled_delayed_migration(5.minutes, 1, 2)
|
||||
|
||||
expect(described_class::MIGRATION)
|
||||
.to be_scheduled_delayed_migration(10.minutes, 3, 4)
|
||||
|
||||
expect(described_class::MIGRATION)
|
||||
.to be_scheduled_delayed_migration(15.minutes, 5, 5)
|
||||
|
||||
expect(BackgroundMigrationWorker.jobs.size).to eq(3)
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
@ -1996,57 +1996,6 @@ describe MergeRequest do
|
|||
end
|
||||
end
|
||||
|
||||
describe '#check_if_can_be_merged' do
|
||||
let(:project) { create(:project, only_allow_merge_if_pipeline_succeeds: true) }
|
||||
|
||||
shared_examples 'checking if can be merged' do
|
||||
context 'when it is not broken and has no conflicts' do
|
||||
before do
|
||||
allow(subject).to receive(:broken?) { false }
|
||||
allow(project.repository).to receive(:can_be_merged?).and_return(true)
|
||||
end
|
||||
|
||||
it 'is marked as mergeable' do
|
||||
expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('can_be_merged')
|
||||
end
|
||||
end
|
||||
|
||||
context 'when broken' do
|
||||
before do
|
||||
allow(subject).to receive(:broken?) { true }
|
||||
allow(project.repository).to receive(:can_be_merged?).and_return(false)
|
||||
end
|
||||
|
||||
it 'becomes unmergeable' do
|
||||
expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('cannot_be_merged')
|
||||
end
|
||||
end
|
||||
|
||||
context 'when it has conflicts' do
|
||||
before do
|
||||
allow(subject).to receive(:broken?) { false }
|
||||
allow(project.repository).to receive(:can_be_merged?).and_return(false)
|
||||
end
|
||||
|
||||
it 'becomes unmergeable' do
|
||||
expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('cannot_be_merged')
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
context 'when merge_status is unchecked' do
|
||||
subject { create(:merge_request, source_project: project, merge_status: :unchecked) }
|
||||
|
||||
it_behaves_like 'checking if can be merged'
|
||||
end
|
||||
|
||||
context 'when merge_status is unchecked' do
|
||||
subject { create(:merge_request, source_project: project, merge_status: :cannot_be_merged_recheck) }
|
||||
|
||||
it_behaves_like 'checking if can be merged'
|
||||
end
|
||||
end
|
||||
|
||||
describe '#mergeable?' do
|
||||
let(:project) { create(:project) }
|
||||
|
||||
|
|
@ -2060,7 +2009,7 @@ describe MergeRequest do
|
|||
|
||||
it 'return true if #mergeable_state? is true and the MR #can_be_merged? is true' do
|
||||
allow(subject).to receive(:mergeable_state?) { true }
|
||||
expect(subject).to receive(:check_if_can_be_merged)
|
||||
expect(subject).to receive(:check_mergeability)
|
||||
expect(subject).to receive(:can_be_merged?) { true }
|
||||
|
||||
expect(subject.mergeable?).to be_truthy
|
||||
|
|
@ -2074,7 +2023,7 @@ describe MergeRequest do
|
|||
|
||||
it 'checks if merge request can be merged' do
|
||||
allow(subject).to receive(:mergeable_ci_state?) { true }
|
||||
expect(subject).to receive(:check_if_can_be_merged)
|
||||
expect(subject).to receive(:check_mergeability)
|
||||
|
||||
subject.mergeable?
|
||||
end
|
||||
|
|
@ -3142,38 +3091,6 @@ describe MergeRequest do
|
|||
end
|
||||
end
|
||||
|
||||
describe '#mergeable_to_ref?' do
|
||||
it 'returns true when merge request is mergeable' do
|
||||
subject = create(:merge_request)
|
||||
|
||||
expect(subject.mergeable_to_ref?).to be(true)
|
||||
end
|
||||
|
||||
it 'returns false when merge request is already merged' do
|
||||
subject = create(:merge_request, :merged)
|
||||
|
||||
expect(subject.mergeable_to_ref?).to be(false)
|
||||
end
|
||||
|
||||
it 'returns false when merge request is closed' do
|
||||
subject = create(:merge_request, :closed)
|
||||
|
||||
expect(subject.mergeable_to_ref?).to be(false)
|
||||
end
|
||||
|
||||
it 'returns false when merge request is work in progress' do
|
||||
subject = create(:merge_request, title: 'WIP: The feature')
|
||||
|
||||
expect(subject.mergeable_to_ref?).to be(false)
|
||||
end
|
||||
|
||||
it 'returns false when merge request has no commits' do
|
||||
subject = create(:merge_request, source_branch: 'empty-branch', target_branch: 'master')
|
||||
|
||||
expect(subject.mergeable_to_ref?).to be(false)
|
||||
end
|
||||
end
|
||||
|
||||
describe '#merge_participants' do
|
||||
it 'contains author' do
|
||||
expect(subject.merge_participants).to eq([subject.author])
|
||||
|
|
|
|||
|
|
@ -1546,52 +1546,80 @@ describe API::MergeRequests do
|
|||
end
|
||||
end
|
||||
|
||||
describe "PUT /projects/:id/merge_requests/:merge_request_iid/merge_to_ref" do
|
||||
let(:pipeline) { create(:ci_pipeline_without_jobs) }
|
||||
describe "GET /projects/:id/merge_requests/:merge_request_iid/merge_ref" do
|
||||
before do
|
||||
merge_request.mark_as_unchecked!
|
||||
end
|
||||
|
||||
let(:merge_request_iid) { merge_request.iid }
|
||||
|
||||
let(:url) do
|
||||
"/projects/#{project.id}/merge_requests/#{merge_request.iid}/merge_to_ref"
|
||||
"/projects/#{project.id}/merge_requests/#{merge_request_iid}/merge_ref"
|
||||
end
|
||||
|
||||
it 'returns the generated ID from the merge service in case of success' do
|
||||
put api(url, user), params: { merge_commit_message: 'Custom message' }
|
||||
|
||||
commit = project.commit(json_response['commit_id'])
|
||||
get api(url, user)
|
||||
|
||||
expect(response).to have_gitlab_http_status(200)
|
||||
expect(json_response['commit_id']).to be_present
|
||||
expect(commit.message).to eq('Custom message')
|
||||
expect(json_response['commit_id']).to eq(merge_request.merge_ref_head.sha)
|
||||
end
|
||||
|
||||
it "returns 400 if branch can't be merged" do
|
||||
merge_request.update!(state: 'merged')
|
||||
context 'when merge-ref is not synced with merge status' do
|
||||
before do
|
||||
merge_request.update!(merge_status: 'cannot_be_merged')
|
||||
end
|
||||
|
||||
put api(url, user)
|
||||
it 'returns 200 if MR can be merged' do
|
||||
get api(url, user)
|
||||
|
||||
expect(response).to have_gitlab_http_status(400)
|
||||
expect(json_response['message'])
|
||||
.to eq("Merge request is not mergeable to #{merge_request.merge_ref_path}")
|
||||
expect(response).to have_gitlab_http_status(200)
|
||||
expect(json_response['commit_id']).to eq(merge_request.merge_ref_head.sha)
|
||||
end
|
||||
|
||||
it 'returns 400 if MR cannot be merged' do
|
||||
expect_next_instance_of(MergeRequests::MergeToRefService) do |merge_request|
|
||||
expect(merge_request).to receive(:execute) { { status: :failed } }
|
||||
end
|
||||
|
||||
get api(url, user)
|
||||
|
||||
expect(response).to have_gitlab_http_status(400)
|
||||
expect(json_response['message']).to eq('Merge request is not mergeable')
|
||||
end
|
||||
end
|
||||
|
||||
it 'returns 403 if user has no permissions to merge to the ref' do
|
||||
user2 = create(:user)
|
||||
project.add_reporter(user2)
|
||||
context 'when user has no access to the MR' do
|
||||
let(:project) { create(:project, :private) }
|
||||
let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
|
||||
|
||||
put api(url, user2)
|
||||
it 'returns 404' do
|
||||
project.add_guest(user)
|
||||
|
||||
expect(response).to have_gitlab_http_status(403)
|
||||
expect(json_response['message']).to eq('403 Forbidden')
|
||||
get api(url, user)
|
||||
|
||||
expect(response).to have_gitlab_http_status(404)
|
||||
expect(json_response['message']).to eq('404 Not found')
|
||||
end
|
||||
end
|
||||
|
||||
it 'returns 404 for an invalid merge request IID' do
|
||||
put api("/projects/#{project.id}/merge_requests/12345/merge_to_ref", user)
|
||||
context 'when invalid merge request IID' do
|
||||
let(:merge_request_iid) { '12345' }
|
||||
|
||||
expect(response).to have_gitlab_http_status(404)
|
||||
it 'returns 404' do
|
||||
get api(url, user)
|
||||
|
||||
expect(response).to have_gitlab_http_status(404)
|
||||
end
|
||||
end
|
||||
|
||||
it "returns 404 if the merge request id is used instead of iid" do
|
||||
put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/merge", user)
|
||||
context 'when merge request ID is used instead IID' do
|
||||
let(:merge_request_iid) { merge_request.id }
|
||||
|
||||
expect(response).to have_gitlab_http_status(404)
|
||||
it 'returns 404' do
|
||||
get api(url, user)
|
||||
|
||||
expect(response).to have_gitlab_http_status(404)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
|
|
|
|||
|
|
@ -72,10 +72,6 @@ describe MergeRequests::MergeToRefService do
|
|||
let(:merge_request) { create(:merge_request, :simple) }
|
||||
let(:project) { merge_request.project }
|
||||
|
||||
before do
|
||||
project.add_maintainer(user)
|
||||
end
|
||||
|
||||
describe '#execute' do
|
||||
let(:service) do
|
||||
described_class.new(project, user, commit_message: 'Awesome message',
|
||||
|
|
@ -92,6 +88,12 @@ describe MergeRequests::MergeToRefService do
|
|||
it_behaves_like 'successfully evaluates pre-condition checks'
|
||||
|
||||
context 'commit history comparison with regular MergeService' do
|
||||
before do
|
||||
# The merge service needs an authorized user while merge-to-ref
|
||||
# doesn't.
|
||||
project.add_maintainer(user)
|
||||
end
|
||||
|
||||
let(:merge_ref_service) do
|
||||
described_class.new(project, user, {})
|
||||
end
|
||||
|
|
@ -104,12 +106,18 @@ describe MergeRequests::MergeToRefService do
|
|||
it_behaves_like 'MergeService for target ref'
|
||||
end
|
||||
|
||||
context 'when merge commit with squash', :quarantine do
|
||||
context 'when merge commit with squash' do
|
||||
before do
|
||||
merge_request.update!(squash: true, source_branch: 'master', target_branch: 'feature')
|
||||
merge_request.update!(squash: true)
|
||||
end
|
||||
|
||||
it_behaves_like 'MergeService for target ref'
|
||||
|
||||
it 'does not squash before merging' do
|
||||
expect(MergeRequests::SquashService).not_to receive(:new)
|
||||
|
||||
process_merge_to_ref
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
|
|
@ -136,9 +144,9 @@ describe MergeRequests::MergeToRefService do
|
|||
let(:merge_method) { :merge }
|
||||
|
||||
it 'returns error' do
|
||||
allow(merge_request).to receive(:mergeable_to_ref?) { false }
|
||||
allow(project).to receive_message_chain(:repository, :merge_to_ref) { nil }
|
||||
|
||||
error_message = "Merge request is not mergeable to #{merge_request.merge_ref_path}"
|
||||
error_message = 'Conflicts detected during merge'
|
||||
|
||||
result = service.execute(merge_request)
|
||||
|
||||
|
|
@ -170,28 +178,5 @@ describe MergeRequests::MergeToRefService do
|
|||
|
||||
it { expect(todo).not_to be_done }
|
||||
end
|
||||
|
||||
context 'when merge request is WIP state' do
|
||||
it 'fails to merge' do
|
||||
merge_request = create(:merge_request, title: 'WIP: The feature')
|
||||
|
||||
result = service.execute(merge_request)
|
||||
|
||||
expect(result[:status]).to eq(:error)
|
||||
expect(result[:message]).to eq("Merge request is not mergeable to #{merge_request.merge_ref_path}")
|
||||
end
|
||||
end
|
||||
|
||||
it 'returns error when user has no authorization to admin the merge request' do
|
||||
unauthorized_user = create(:user)
|
||||
project.add_reporter(unauthorized_user)
|
||||
|
||||
service = described_class.new(project, unauthorized_user)
|
||||
|
||||
result = service.execute(merge_request)
|
||||
|
||||
expect(result[:status]).to eq(:error)
|
||||
expect(result[:message]).to eq('You are not allowed to merge to this ref')
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
|||
218
spec/services/merge_requests/mergeability_check_service_spec.rb
Normal file
218
spec/services/merge_requests/mergeability_check_service_spec.rb
Normal file
|
|
@ -0,0 +1,218 @@
|
|||
# frozen_string_literal: true
|
||||
|
||||
require 'spec_helper'
|
||||
|
||||
describe MergeRequests::MergeabilityCheckService do
|
||||
shared_examples_for 'unmergeable merge request' do
|
||||
it 'updates or keeps merge status as cannot_be_merged' do
|
||||
subject
|
||||
|
||||
expect(merge_request.merge_status).to eq('cannot_be_merged')
|
||||
end
|
||||
|
||||
it 'does not change the merge ref HEAD' do
|
||||
expect { subject }.not_to change(merge_request, :merge_ref_head)
|
||||
end
|
||||
|
||||
it 'returns ServiceResponse.error' do
|
||||
result = subject
|
||||
|
||||
expect(result).to be_a(ServiceResponse)
|
||||
expect(result).to be_error
|
||||
end
|
||||
end
|
||||
|
||||
shared_examples_for 'mergeable merge request' do
|
||||
it 'updates or keeps merge status as can_be_merged' do
|
||||
subject
|
||||
|
||||
expect(merge_request.merge_status).to eq('can_be_merged')
|
||||
end
|
||||
|
||||
it 'updates the merge ref' do
|
||||
expect { subject }.to change(merge_request, :merge_ref_head).from(nil)
|
||||
end
|
||||
|
||||
it 'returns ServiceResponse.success' do
|
||||
result = subject
|
||||
|
||||
expect(result).to be_a(ServiceResponse)
|
||||
expect(result).to be_success
|
||||
end
|
||||
|
||||
it 'ServiceResponse has merge_ref_head payload' do
|
||||
result = subject
|
||||
|
||||
expect(result.payload.keys).to contain_exactly(:merge_ref_head)
|
||||
expect(result.payload[:merge_ref_head].keys)
|
||||
.to contain_exactly(:commit_id, :target_id, :source_id)
|
||||
end
|
||||
end
|
||||
|
||||
describe '#execute' do
|
||||
let(:project) { create(:project, :repository) }
|
||||
let(:merge_request) { create(:merge_request, merge_status: :unchecked, source_project: project, target_project: project) }
|
||||
let(:repo) { project.repository }
|
||||
|
||||
subject { described_class.new(merge_request).execute }
|
||||
|
||||
before do
|
||||
project.add_developer(merge_request.author)
|
||||
end
|
||||
|
||||
it_behaves_like 'mergeable merge request'
|
||||
|
||||
context 'when multiple calls to the service' do
|
||||
it 'returns success' do
|
||||
subject
|
||||
result = subject
|
||||
|
||||
expect(result).to be_a(ServiceResponse)
|
||||
expect(result.success?).to be(true)
|
||||
end
|
||||
|
||||
it 'second call does not change the merge-ref' do
|
||||
expect { subject }.to change(merge_request, :merge_ref_head).from(nil)
|
||||
expect { subject }.not_to change(merge_request, :merge_ref_head)
|
||||
end
|
||||
end
|
||||
|
||||
context 'disabled merge ref sync feature flag' do
|
||||
before do
|
||||
stub_feature_flags(merge_ref_auto_sync: false)
|
||||
end
|
||||
|
||||
it 'returns error and no payload' do
|
||||
result = subject
|
||||
|
||||
expect(result).to be_a(ServiceResponse)
|
||||
expect(result.error?).to be(true)
|
||||
expect(result.message).to eq('Merge ref is outdated due to disabled feature')
|
||||
expect(result.payload).to be_empty
|
||||
end
|
||||
|
||||
it 'updates the merge_status' do
|
||||
expect { subject }.to change(merge_request, :merge_status).from('unchecked').to('can_be_merged')
|
||||
end
|
||||
end
|
||||
|
||||
context 'when broken' do
|
||||
before do
|
||||
allow(merge_request).to receive(:broken?) { true }
|
||||
allow(project.repository).to receive(:can_be_merged?) { false }
|
||||
end
|
||||
|
||||
it_behaves_like 'unmergeable merge request'
|
||||
|
||||
it 'returns ServiceResponse.error' do
|
||||
result = subject
|
||||
|
||||
expect(result).to be_a(ServiceResponse)
|
||||
expect(result.error?).to be(true)
|
||||
expect(result.message).to eq('Merge request is not mergeable')
|
||||
end
|
||||
end
|
||||
|
||||
context 'when it has conflicts' do
|
||||
before do
|
||||
allow(merge_request).to receive(:broken?) { false }
|
||||
allow(project.repository).to receive(:can_be_merged?) { false }
|
||||
end
|
||||
|
||||
it_behaves_like 'unmergeable merge request'
|
||||
|
||||
it 'returns ServiceResponse.error' do
|
||||
result = subject
|
||||
|
||||
expect(result).to be_a(ServiceResponse)
|
||||
expect(result.error?).to be(true)
|
||||
expect(result.message).to eq('Merge request is not mergeable')
|
||||
end
|
||||
end
|
||||
|
||||
context 'when MR cannot be merged and has no merge ref' do
|
||||
before do
|
||||
merge_request.mark_as_unmergeable!
|
||||
end
|
||||
|
||||
it_behaves_like 'unmergeable merge request'
|
||||
|
||||
it 'returns ServiceResponse.error' do
|
||||
result = subject
|
||||
|
||||
expect(result).to be_a(ServiceResponse)
|
||||
expect(result.error?).to be(true)
|
||||
expect(result.message).to eq('Merge request is not mergeable')
|
||||
end
|
||||
end
|
||||
|
||||
context 'when MR cannot be merged and has outdated merge ref' do
|
||||
before do
|
||||
MergeRequests::MergeToRefService.new(project, merge_request.author).execute(merge_request)
|
||||
merge_request.mark_as_unmergeable!
|
||||
end
|
||||
|
||||
it_behaves_like 'unmergeable merge request'
|
||||
|
||||
it 'returns ServiceResponse.error' do
|
||||
result = subject
|
||||
|
||||
expect(result).to be_a(ServiceResponse)
|
||||
expect(result.error?).to be(true)
|
||||
expect(result.message).to eq('Merge request is not mergeable')
|
||||
end
|
||||
end
|
||||
|
||||
context 'when merge request is not given' do
|
||||
subject { described_class.new(nil).execute }
|
||||
|
||||
it 'returns ServiceResponse.error' do
|
||||
result = subject
|
||||
|
||||
expect(result).to be_a(ServiceResponse)
|
||||
expect(result.message).to eq('Invalid argument')
|
||||
end
|
||||
end
|
||||
|
||||
context 'when read only DB' do
|
||||
it 'returns ServiceResponse.error' do
|
||||
allow(Gitlab::Database).to receive(:read_only?) { true }
|
||||
|
||||
result = subject
|
||||
|
||||
expect(result).to be_a(ServiceResponse)
|
||||
expect(result.message).to eq('Unsupported operation')
|
||||
end
|
||||
end
|
||||
|
||||
context 'when fails to update the merge-ref' do
|
||||
before do
|
||||
expect_next_instance_of(MergeRequests::MergeToRefService) do |merge_to_ref|
|
||||
expect(merge_to_ref).to receive(:execute).and_return(status: :failed)
|
||||
end
|
||||
end
|
||||
|
||||
it_behaves_like 'unmergeable merge request'
|
||||
|
||||
it 'returns ServiceResponse.error' do
|
||||
result = subject
|
||||
|
||||
expect(result).to be_a(ServiceResponse)
|
||||
expect(result.error?).to be(true)
|
||||
expect(result.message).to eq('Merge request is not mergeable')
|
||||
end
|
||||
end
|
||||
|
||||
context 'recheck enforced' do
|
||||
subject { described_class.new(merge_request).execute(recheck: true) }
|
||||
|
||||
context 'when MR is mergeable but merge-ref does not exists' do
|
||||
before do
|
||||
merge_request.mark_as_mergeable!
|
||||
end
|
||||
|
||||
it_behaves_like 'mergeable merge request'
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
@ -16,6 +16,13 @@ describe ServiceResponse do
|
|||
expect(response).to be_success
|
||||
expect(response.message).to eq('Good orange')
|
||||
end
|
||||
|
||||
it 'creates a successful response with payload' do
|
||||
response = described_class.success(payload: { good: 'orange' })
|
||||
|
||||
expect(response).to be_success
|
||||
expect(response.payload).to eq(good: 'orange')
|
||||
end
|
||||
end
|
||||
|
||||
describe '.error' do
|
||||
|
|
@ -33,6 +40,15 @@ describe ServiceResponse do
|
|||
expect(response.message).to eq('Bad apple')
|
||||
expect(response.http_status).to eq(400)
|
||||
end
|
||||
|
||||
it 'creates a failed response with payload' do
|
||||
response = described_class.error(message: 'Bad apple',
|
||||
payload: { bad: 'apple' })
|
||||
|
||||
expect(response).to be_error
|
||||
expect(response.message).to eq('Bad apple')
|
||||
expect(response.payload).to eq(bad: 'apple')
|
||||
end
|
||||
end
|
||||
|
||||
describe '#success?' do
|
||||
|
|
|
|||
Loading…
Reference in a new issue