Commit e38dc10c authored by Oswaldo Ferreira's avatar Oswaldo Ferreira

Clean merge_jid whenever necessary on the merge process

MergeRequest#merge_jid should be cleaned up whenever we hit a known error on MergeService#execute. This way we can keep track if the MR is really "ongoing" or "stuck"
parent 38a848be
...@@ -14,13 +14,13 @@ module MergeRequests ...@@ -14,13 +14,13 @@ module MergeRequests
@merge_request = merge_request @merge_request = merge_request
unless @merge_request.mergeable? unless @merge_request.mergeable?
return log_merge_error('Merge request is not mergeable', save_message_on_model: true) return handle_merge_error(log_message: 'Merge request is not mergeable', save_message_on_model: true)
end end
@source = find_merge_source @source = find_merge_source
unless @source unless @source
return log_merge_error('No source for merge', save_message_on_model: true) return handle_merge_error(log_message: 'No source for merge', save_message_on_model: true)
end end
merge_request.in_locked_state do merge_request.in_locked_state do
...@@ -31,8 +31,7 @@ module MergeRequests ...@@ -31,8 +31,7 @@ module MergeRequests
end end
end end
rescue MergeError => e rescue MergeError => e
clean_merge_jid handle_merge_error(log_message: e.message, save_message_on_model: true)
log_merge_error(e.message, save_message_on_model: true)
end end
private private
...@@ -74,10 +73,16 @@ module MergeRequests ...@@ -74,10 +73,16 @@ module MergeRequests
@merge_request.force_remove_source_branch? ? @merge_request.author : current_user @merge_request.force_remove_source_branch? ? @merge_request.author : current_user
end end
def log_merge_error(message, save_message_on_model: false) # Logs merge error message and cleans `MergeRequest#merge_jid`.
Rails.logger.error("MergeService ERROR: #{merge_request_info} - #{message}") #
def handle_merge_error(log_message:, save_message_on_model: false)
Rails.logger.error("MergeService ERROR: #{merge_request_info} - #{log_message}")
@merge_request.update(merge_error: message) if save_message_on_model if save_message_on_model
@merge_request.update(merge_error: log_message, merge_jid: nil)
else
clean_merge_jid
end
end end
def merge_request_info def merge_request_info
......
---
title: Adjust MRs being stuck on "process of being merged" for more than 2 hours
merge_request:
author:
type: fixed
...@@ -13,20 +13,21 @@ describe MergeRequests::MergeService do ...@@ -13,20 +13,21 @@ describe MergeRequests::MergeService do
describe '#execute' do describe '#execute' do
context 'MergeRequest#merge_jid' do context 'MergeRequest#merge_jid' do
let(:service) do
described_class.new(project, user, commit_message: 'Awesome message')
end
before do before do
merge_request.update_column(:merge_jid, 'hash-123') merge_request.update_column(:merge_jid, 'hash-123')
end end
it 'is cleaned when no error is raised' do it 'is cleaned when no error is raised' do
service = described_class.new(project, user, commit_message: 'Awesome message')
service.execute(merge_request) service.execute(merge_request)
expect(merge_request.reload.merge_jid).to be_nil expect(merge_request.reload.merge_jid).to be_nil
end end
it 'is cleaned when expected error is raised' do it 'is cleaned when expected error is raised' do
service = described_class.new(project, user, commit_message: 'Awesome message')
allow(service).to receive(:commit).and_raise(described_class::MergeError) allow(service).to receive(:commit).and_raise(described_class::MergeError)
service.execute(merge_request) service.execute(merge_request)
...@@ -34,6 +35,22 @@ describe MergeRequests::MergeService do ...@@ -34,6 +35,22 @@ describe MergeRequests::MergeService do
expect(merge_request.reload.merge_jid).to be_nil expect(merge_request.reload.merge_jid).to be_nil
end end
it 'is cleaned when merge request is not mergeable' do
allow(merge_request).to receive(:mergeable?).and_return(false)
service.execute(merge_request)
expect(merge_request.reload.merge_jid).to be_nil
end
it 'is cleaned when no source is found' do
allow(merge_request).to receive(:diff_head_sha).and_return(nil)
service.execute(merge_request)
expect(merge_request.reload.merge_jid).to be_nil
end
it 'is not cleaned when unexpected error is raised' do it 'is not cleaned when unexpected error is raised' do
service = described_class.new(project, user, commit_message: 'Awesome message') service = described_class.new(project, user, commit_message: 'Awesome message')
allow(service).to receive(:commit).and_raise(StandardError) allow(service).to receive(:commit).and_raise(StandardError)
......
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