diff options
author | Oswaldo Ferreira <oswaldo@gitlab.com> | 2019-05-21 18:14:22 -0300 |
---|---|---|
committer | Oswaldo Ferreira <oswaldo@gitlab.com> | 2019-06-20 11:48:30 -0300 |
commit | 3af348b6cf28ef1d9d3025f7012049132b57798c (patch) | |
tree | 540b724d6b711e3d2247dad5c371b54ec81ea610 /app/services | |
parent | c6eb18ee0fe1b887438da87f25fc2f2a852dd393 (diff) | |
download | gitlab-ce-3af348b6cf28ef1d9d3025f7012049132b57798c.tar.gz |
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.
Diffstat (limited to 'app/services')
-rw-r--r-- | app/services/merge_requests/merge_to_ref_service.rb | 20 | ||||
-rw-r--r-- | app/services/merge_requests/mergeability_check_service.rb | 96 | ||||
-rw-r--r-- | app/services/service_response.rb | 15 |
3 files changed, 113 insertions, 18 deletions
diff --git a/app/services/merge_requests/merge_to_ref_service.rb b/app/services/merge_requests/merge_to_ref_service.rb index 87147d90c32..efe4dcd6255 100644 --- a/app/services/merge_requests/merge_to_ref_service.rb +++ b/app/services/merge_requests/merge_to_ref_service.rb @@ -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 diff --git a/app/services/merge_requests/mergeability_check_service.rb b/app/services/merge_requests/mergeability_check_service.rb new file mode 100644 index 00000000000..a8b2cdd64d2 --- /dev/null +++ b/app/services/merge_requests/mergeability_check_service.rb @@ -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 diff --git a/app/services/service_response.rb b/app/services/service_response.rb index 1de30e68d87..f3437ba16de 100644 --- a/app/services/service_response.rb +++ b/app/services/service_response.rb @@ -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 |