kotovalexarian-likes-gitlab/gitlab-org--gitlab-foss
1
0
Fork 0
Code Releases Activity

Revert "Automatically update MR merge-ref along merge status"

Browse source
This commit is contained in:
Oswaldo Ferreira 2019-06-11 13:08:25 -03:00
parent 74850f1f8e
commit 00a273d3a9
14 changed files with 223 additions and 381 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

6
app/controllers/projects/merge_requests_controller.rb
View file

@ -33,7 +33,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
def show
close_merge_request_if_no_source_project
@merge_request.check_mergeability
mark_merge_request_mergeable
respond_to do |format|
format.html do
@ -251,6 +251,10 @@ 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

36
app/models/merge_request.rb
View file

@ -725,16 +725,19 @@ class MergeRequest < ApplicationRecord
MergeRequests::ReloadDiffsService.new(self, current_user).execute
end
def check_mergeability
MergeRequests::MergeabilityCheckService.new(self).execute
end
# rubocop: enable CodeReuse/ServiceClass
# 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)
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
end
def merge_event
@ -760,7 +763,7 @@ class MergeRequest < ApplicationRecord
def mergeable?(skip_ci_check: false)
return false unless mergeable_state?(skip_ci_check: skip_ci_check)
check_mergeability
check_if_can_be_merged
can_be_merged? && !should_be_rebased?
end
@ -775,6 +778,15 @@ 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
@ -1087,12 +1099,6 @@ 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

20
app/services/merge_requests/merge_to_ref_service.rb
View file

@ -20,14 +20,20 @@ module MergeRequests
raise_error('Conflicts detected during merge') unless commit_id
success(commit_id: commit_id)
rescue MergeError, ArgumentError => error
commit = project.commit(commit_id)
target_id, source_id = commit.parent_ids
success(commit_id: commit.id,
target_id: target_id,
source_id: source_id)
rescue MergeError => error
error(error.message)
end
private
def validate!
authorization_check!
error_check!
end
@ -37,13 +43,21 @@ module MergeRequests
error =
if !hooks_validation_pass?(merge_request)
hooks_validation_error(merge_request)
elsif source.blank?
elsif !@merge_request.mergeable_to_ref?
"Merge request is not mergeable to #{target_ref}"
elsif !source
'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

82
app/services/merge_requests/mergeability_check_service.rb
View file

@ -1,82 +0,0 @@
# frozen_string_literal: true
module MergeRequests
class MergeabilityCheckService < ::BaseService
include Gitlab::Utils::StrongMemoize
delegate :project, 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.
#
# 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
return ServiceResponse.error(message: 'Invalid argument') unless merge_request
return ServiceResponse.error(message: 'Unsupported operation') if Gitlab::Database.read_only?
update_merge_status
unless merge_request.can_be_merged?
return ServiceResponse.error(message: 'Merge request is not mergeable')
end
unless payload.fetch(:merge_ref_head)
return ServiceResponse.error(message: 'Merge ref was not found')
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 can_git_merge?
!merge_request.broken? && repository.can_be_merged?(merge_request.diff_head_sha, merge_request.target_branch)
end
def merge_to_ref
result = MergeRequests::MergeToRefService.new(project, merge_request.author).execute(merge_request)
result[:status] == :success
end
end
end

15
app/services/service_response.rb
View file

@ -1,20 +1,19 @@
# frozen_string_literal: true
class ServiceResponse
def self.success(message: nil, payload: {})
new(status: :success, message: message, payload: payload)
def self.success(message: nil)
new(status: :success, message: message)
end
def self.error(message:, payload: {}, http_status: nil)
new(status: :error, message: message, payload: payload, http_status: http_status)
def self.error(message:, http_status: nil)
new(status: :error, message: message, http_status: http_status)
end
attr_reader :status, :message, :http_status, :payload
attr_reader :status, :message, :http_status
def initialize(status:, message: nil, payload: {}, http_status: nil)
def initialize(status:, message: nil, http_status: nil)
self.status = status
self.message = message
self.payload = payload
self.http_status = http_status
end
@ -28,5 +27,5 @@ class ServiceResponse
private
attr_writer :status, :message, :http_status, :payload
attr_writer :status, :message, :http_status
end

5
changelogs/unreleased/osw-sync-merge-ref-upon-mergeability-check.yml
View file

@ -1,5 +0,0 @@
---
title: Sync merge ref upon mergeability check
merge_request: 28513
author:
type: added

22
doc/api/merge_requests.md
View file

@ -1191,29 +1191,33 @@ Parameters:
}
```
## Returns the up to date merge-ref HEAD commit
## Merge to default merge ref path
Merge the changes between the merge request source and target branches into `refs/merge-requests/:iid/merge`
ref, of the target project repository, if possible. This ref will have the state the target branch would have if
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
a regular merge action was taken.
This is not a regular merge action given it doesn't change the merge request target branch state in any manner.
This is not a regular merge action given it doesn't change the merge request state in any manner.
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.
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.
If the merge request has conflicts, is empty or already merged, you'll get a `400` and a descriptive error message.
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`.
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`.
```
GET /projects/:id/merge_requests/:merge_request_iid/merge_ref
PUT /projects/:id/merge_requests/:merge_request_iid/merge_to_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
{

2
lib/api/entities.rb
View file

@ -701,7 +701,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_mergeability
merge_request.check_if_can_be_merged
merge_request.merge_status
end
expose :diff_head_sha, as: :sha

24
lib/api/merge_requests.rb
View file

@ -397,16 +397,28 @@ module API
present merge_request, with: Entities::MergeRequest, current_user: current_user, project: user_project
end
desc 'Returns the up to date merge-ref HEAD commit'
get ':id/merge_requests/:merge_request_iid/merge_ref' do
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
merge_request = find_project_merge_request(params[:merge_request_iid])
result = ::MergeRequests::MergeabilityCheckService.new(merge_request).execute
authorize! :admin_merge_request, user_project
if result.success?
present :commit_id, result.payload.dig(:merge_ref_head, :commit_id)
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
else
render_api_error!(result.message, 400)
http_status = result[:http_status] || 400
render_api_error!(result[:message], http_status)
end
end

87
spec/models/merge_request_spec.rb
View file

@ -1996,6 +1996,57 @@ 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) }
@ -2009,7 +2060,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_mergeability)
expect(subject).to receive(:check_if_can_be_merged)
expect(subject).to receive(:can_be_merged?) { true }
expect(subject.mergeable?).to be_truthy
@ -2023,7 +2074,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_mergeability)
expect(subject).to receive(:check_if_can_be_merged)
subject.mergeable?
end
@ -3091,6 +3142,38 @@ 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])

61
spec/requests/api/merge_requests_spec.rb
View file

@ -1546,65 +1546,52 @@ describe API::MergeRequests do
end
end
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 }
describe "PUT /projects/:id/merge_requests/:merge_request_iid/merge_to_ref" do
let(:pipeline) { create(:ci_pipeline_without_jobs) }
let(:url) do
"/projects/#{project.id}/merge_requests/#{merge_request_iid}/merge_ref"
"/projects/#{project.id}/merge_requests/#{merge_request.iid}/merge_to_ref"
end
it 'returns the generated ID from the merge service in case of success' do
get api(url, user)
put api(url, user), params: { merge_commit_message: 'Custom message' }
commit = project.commit(json_response['commit_id'])
expect(response).to have_gitlab_http_status(200)
expect(json_response['commit_id']).to eq(merge_request.merge_ref_head.sha)
expect(json_response['commit_id']).to be_present
expect(commit.message).to eq('Custom message')
end
it "returns 400 if branch can't be merged" do
merge_request.update!(merge_status: 'cannot_be_merged')
merge_request.update!(state: 'merged')
get api(url, user)
put api(url, user)
expect(response).to have_gitlab_http_status(400)
expect(json_response['message']).to eq('Merge request is not mergeable')
expect(json_response['message'])
.to eq("Merge request is not mergeable to #{merge_request.merge_ref_path}")
end
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) }
it 'returns 403 if user has no permissions to merge to the ref' do
user2 = create(:user)
project.add_reporter(user2)
it 'returns 404' do
project.add_guest(user)
put api(url, user2)
get api(url, user)
expect(response).to have_gitlab_http_status(404)
expect(json_response['message']).to eq('404 Not found')
end
expect(response).to have_gitlab_http_status(403)
expect(json_response['message']).to eq('403 Forbidden')
end
context 'when invalid merge request IID' do
let(:merge_request_iid) { '12345' }
it 'returns 404 for an invalid merge request IID' do
put api("/projects/#{project.id}/merge_requests/12345/merge_to_ref", user)
it 'returns 404' do
get api(url, user)
expect(response).to have_gitlab_http_status(404)
end
expect(response).to have_gitlab_http_status(404)
end
context 'when merge request ID is used instead IID' do
let(:merge_request_iid) { merge_request.id }
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)
it 'returns 404' do
get api(url, user)
expect(response).to have_gitlab_http_status(404)
end
expect(response).to have_gitlab_http_status(404)
end
end

41
spec/services/merge_requests/merge_to_ref_service_spec.rb
View file

@ -32,8 +32,10 @@ describe MergeRequests::MergeToRefService do
expect(result[:status]).to eq(:success)
expect(result[:commit_id]).to be_present
expect(result[:source_id]).to eq(merge_request.source_branch_sha)
expect(result[:target_id]).to eq(merge_request.target_branch_sha)
expect(repository.ref_exists?(target_ref)).to be(true)
expect(ref_head.sha).to eq(result[:commit_id])
expect(ref_head.id).to eq(result[:commit_id])
end
end
@ -70,6 +72,10 @@ 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',
@ -86,12 +92,6 @@ 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
@ -136,9 +136,9 @@ describe MergeRequests::MergeToRefService do
let(:merge_method) { :merge }
it 'returns error' do
allow(project).to receive_message_chain(:repository, :merge_to_ref) { nil }
allow(merge_request).to receive(:mergeable_to_ref?) { false }
error_message = 'Conflicts detected during merge'
error_message = "Merge request is not mergeable to #{merge_request.merge_ref_path}"
result = service.execute(merge_request)
@ -170,5 +170,28 @@ 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

187
spec/services/merge_requests/mergeability_check_service_spec.rb
View file

@ -1,187 +0,0 @@
# 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 '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 MR is mergeable but merge-ref does not exists' do
before do
merge_request.mark_as_mergeable!
end
it 'keeps merge status as can_be_merged' do
expect { subject }.not_to change(merge_request, :merge_status).from('can_be_merged')
end
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 ref was not found')
end
end
end
end

16
spec/services/service_response_spec.rb
View file

@ -16,13 +16,6 @@ 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
@ -40,15 +33,6 @@ 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