diff options
author | Shinya Maeda <shinya@gitlab.com> | 2019-05-22 18:45:27 +0700 |
---|---|---|
committer | Shinya Maeda <shinya@gitlab.com> | 2019-06-03 13:15:29 +0700 |
commit | d4b46936633a3b2a0248b4572b4a1dc7b2ba8531 (patch) | |
tree | bf5c1cf5a8ebf1568ca62576d63ff793ce29764f /app | |
parent | 96744d0befd7e298c08e6d20a4f504086a717c35 (diff) | |
download | gitlab-ce-d4b46936633a3b2a0248b4572b4a1dc7b2ba8531.tar.gz |
Abstract auto merge processes
We have one auto merge strategy today - Merge When Pipeline
Succeeds.
In order to add more strategies for Merge Train feature,
we abstract the architecture to be more extensible.
Removed arguments
Fix spec
Diffstat (limited to 'app')
16 files changed, 175 insertions, 110 deletions
diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merge_when_pipeline_succeeds.vue b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merge_when_pipeline_succeeds.vue index 1b3af2fccf2..88e1ccbaf35 100644 --- a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merge_when_pipeline_succeeds.vue +++ b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merge_when_pipeline_succeeds.vue @@ -57,7 +57,7 @@ export default { removeSourceBranch() { const options = { sha: this.mr.sha, - merge_when_pipeline_succeeds: true, + auto_merge_strategy: 'merge_when_pipeline_succeeds', should_remove_source_branch: true, }; @@ -85,7 +85,7 @@ export default { <h4 class="d-flex align-items-start"> <span class="append-right-10"> {{ s__('mrWidget|Set by') }} - <mr-widget-author :author="mr.setToMWPSBy" /> + <mr-widget-author :author="mr.setToAutoMergeBy" /> {{ s__('mrWidget|to be merged automatically when the pipeline succeeds') }} </span> <a diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/ready_to_merge.vue b/app/assets/javascripts/vue_merge_request_widget/components/states/ready_to_merge.vue index 851939d5d4e..c7915f3918b 100644 --- a/app/assets/javascripts/vue_merge_request_widget/components/states/ready_to_merge.vue +++ b/app/assets/javascripts/vue_merge_request_widget/components/states/ready_to_merge.vue @@ -31,7 +31,7 @@ export default { return { removeSourceBranch: this.mr.shouldRemoveSourceBranch, mergeWhenBuildSucceeds: false, - setToMergeWhenPipelineSucceeds: false, + autoMergeStrategy: null, isMakingRequest: false, isMergingImmediately: false, commitMessage: this.mr.commitMessage, @@ -42,7 +42,7 @@ export default { }; }, computed: { - shouldShowMergeWhenPipelineSucceedsText() { + shouldShowAutoMergeText() { return this.mr.isPipelineActive; }, status() { @@ -87,7 +87,7 @@ export default { mergeButtonText() { if (this.isMergingImmediately) { return __('Merge in progress'); - } else if (this.shouldShowMergeWhenPipelineSucceedsText) { + } else if (this.shouldShowAutoMergeText) { return __('Merge when pipeline succeeds'); } @@ -104,7 +104,7 @@ export default { return enableSquashBeforeMerge && commitsCount > 1; }, shouldShowMergeControls() { - return this.mr.isMergeAllowed || this.shouldShowMergeWhenPipelineSucceedsText; + return this.mr.isMergeAllowed || this.shouldShowAutoMergeText; }, shouldShowSquashEdit() { return this.squashBeforeMerge && this.shouldShowSquashBeforeMerge; @@ -126,12 +126,16 @@ export default { this.isMergingImmediately = true; } - this.setToMergeWhenPipelineSucceeds = mergeWhenBuildSucceeds === true; + if (mergeWhenBuildSucceeds === true) { + this.autoMergeStrategy = 'merge_when_pipeline_succeeds' + } else { + this.autoMergeStrategy = null + } const options = { sha: this.mr.sha, commit_message: this.commitMessage, - merge_when_pipeline_succeeds: this.setToMergeWhenPipelineSucceeds, + auto_merge_strategy: this.autoMergeStrategy, should_remove_source_branch: this.removeSourceBranch === true, squash: this.squashBeforeMerge, squash_commit_message: this.squashCommitMessage, diff --git a/app/assets/javascripts/vue_merge_request_widget/stores/get_state_key.js b/app/assets/javascripts/vue_merge_request_widget/stores/get_state_key.js index 0cc4fd59f5e..f56875acfe9 100644 --- a/app/assets/javascripts/vue_merge_request_widget/stores/get_state_key.js +++ b/app/assets/javascripts/vue_merge_request_widget/stores/get_state_key.js @@ -23,7 +23,7 @@ export default function deviseState(data) { return stateKey.pipelineBlocked; } else if (this.isSHAMismatch) { return stateKey.shaMismatch; - } else if (this.mergeWhenPipelineSucceeds) { + } else if (this.autoMergeEnabled) { return this.mergeError ? stateKey.autoMergeFailed : stateKey.mergeWhenPipelineSucceeds; } else if (!this.canMerge) { return stateKey.notAllowedToMerge; diff --git a/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js b/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js index 45708d78886..1e5727d6132 100644 --- a/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js +++ b/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js @@ -61,7 +61,7 @@ export default class MergeRequestStore { this.updatedAt = data.updated_at; this.metrics = MergeRequestStore.buildMetrics(data.metrics); - this.setToMWPSBy = MergeRequestStore.formatUserObject(data.merge_user || {}); + this.setToAutoMergeBy = MergeRequestStore.formatUserObject(data.merge_user || {}); this.mergeUserId = data.merge_user_id; this.currentUserId = gon.current_user_id; this.sourceBranchPath = data.source_branch_path; @@ -70,12 +70,13 @@ export default class MergeRequestStore { this.targetBranchPath = data.target_branch_commits_path; this.targetBranchTreePath = data.target_branch_tree_path; this.conflictResolutionPath = data.conflict_resolution_path; - this.cancelAutoMergePath = data.cancel_merge_when_pipeline_succeeds_path; + this.cancelAutoMergePath = data.cancel_auto_merge_path; this.removeWIPPath = data.remove_wip_path; this.sourceBranchRemoved = !data.source_branch_exists; this.shouldRemoveSourceBranch = data.remove_source_branch || false; this.onlyAllowMergeIfPipelineSucceeds = data.only_allow_merge_if_pipeline_succeeds || false; - this.mergeWhenPipelineSucceeds = data.merge_when_pipeline_succeeds || false; + this.autoMergeEnabled = data.auto_merge_enabled || false; + this.autoMergeStrategy = data.auto_merge_strategy; this.mergePath = data.merge_path; this.ffOnlyEnabled = data.ff_only_enabled; this.shouldBeRebased = !!data.should_be_rebased; @@ -93,7 +94,7 @@ export default class MergeRequestStore { this.canRemoveSourceBranch = currentUser.can_remove_source_branch || false; this.canMerge = !!data.merge_path; this.canCreateIssue = currentUser.can_create_issue || false; - this.canCancelAutomaticMerge = !!data.cancel_merge_when_pipeline_succeeds_path; + this.canCancelAutomaticMerge = !!data.cancel_auto_merge_path; this.isSHAMismatch = this.sha !== data.diff_head_sha; this.canBeMerged = data.can_be_merged || false; this.isMergeAllowed = data.mergeable || false; diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 8f177895b08..a3ceb76845e 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -145,14 +145,12 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo render partial: 'projects/merge_requests/widget/commit_change_content', layout: false end - def cancel_merge_when_pipeline_succeeds - unless @merge_request.can_cancel_merge_when_pipeline_succeeds?(current_user) + def cancel_auto_merge + unless @merge_request.can_cancel_auto_merge?(current_user) return access_denied! end - ::MergeRequests::MergeWhenPipelineSucceedsService - .new(@project, current_user) - .cancel(@merge_request) + AutoMergeService.new(project, current_user).cancel(@merge_request) render json: serialize_widget(@merge_request) end @@ -229,12 +227,12 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo end def merge_params_attributes - [:should_remove_source_branch, :commit_message, :squash_commit_message, :squash] + [:should_remove_source_branch, :commit_message, :squash_commit_message, :squash, :auto_merge_strategy] end - def merge_when_pipeline_succeeds_active? - params[:merge_when_pipeline_succeeds].present? && - @merge_request.head_pipeline && @merge_request.head_pipeline.active? + def auto_merge_requested? + # Support params[:merge_when_pipeline_succeeds] during the transition period + params[:auto_merge_strategy].present? || params[:merge_when_pipeline_succeeds].present? end def close_merge_request_if_no_source_project @@ -258,9 +256,9 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo end def merge! - # Disable the CI check if merge_when_pipeline_succeeds is enabled since we have + # Disable the CI check if auto_merge_strategy is specified since we have # to wait until CI completes to know - unless @merge_request.mergeable?(skip_ci_check: merge_when_pipeline_succeeds_active?) + unless @merge_request.mergeable?(skip_ci_check: auto_merge_requested?) return :failed end @@ -274,24 +272,10 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo @merge_request.update(merge_error: nil, squash: merge_params.fetch(:squash, false)) - if params[:merge_when_pipeline_succeeds].present? - return :failed unless @merge_request.actual_head_pipeline - - if @merge_request.actual_head_pipeline.active? - ::MergeRequests::MergeWhenPipelineSucceedsService - .new(@project, current_user, merge_params) - .execute(@merge_request) - - :merge_when_pipeline_succeeds - elsif @merge_request.actual_head_pipeline.success? - # This can be triggered when a user clicks the auto merge button while - # the tests finish at about the same time - @merge_request.merge_async(current_user.id, merge_params) - - :success - else - :failed - end + if auto_merge_requested? + AutoMergeService.new(project, current_user, merge_params) + .execute(merge_request, + params[:auto_merge_strategy] || AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS) else @merge_request.merge_async(current_user.id, merge_params) diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb index 991ca42c445..2de4e92e33e 100644 --- a/app/helpers/merge_requests_helper.rb +++ b/app/helpers/merge_requests_helper.rb @@ -103,7 +103,7 @@ module MergeRequestsHelper def merge_params(merge_request) { - merge_when_pipeline_succeeds: true, + auto_merge_strategy: AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS, should_remove_source_branch: true, sha: merge_request.diff_head_sha, squash: merge_request.squash diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 311ba1ce6bd..2602738901b 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -165,7 +165,7 @@ class MergeRequest < ApplicationRecord validates :source_branch, presence: true validates :target_project, presence: true validates :target_branch, presence: true - validates :merge_user, presence: true, if: :merge_when_pipeline_succeeds?, unless: :importing? + validates :merge_user, presence: true, if: :auto_merge_enabled?, unless: :importing? validate :validate_branches, unless: [:allow_broken, :importing?, :closed_without_fork?] validate :validate_fork, unless: :closed_without_fork? validate :validate_target_project, on: :create @@ -196,6 +196,7 @@ class MergeRequest < ApplicationRecord alias_attribute :project, :target_project alias_attribute :project_id, :target_project_id + alias_attribute :auto_merge_enabled, :merge_when_pipeline_succeeds def self.reference_prefix '!' @@ -391,7 +392,7 @@ class MergeRequest < ApplicationRecord def merge_participants participants = [author] - if merge_when_pipeline_succeeds? && !participants.include?(merge_user) + if auto_merge_enabled? && !participants.include?(merge_user) participants << merge_user end @@ -782,7 +783,7 @@ class MergeRequest < ApplicationRecord project.ff_merge_must_be_possible? && !ff_merge_possible? end - def can_cancel_merge_when_pipeline_succeeds?(current_user) + def can_cancel_auto_merge?(current_user) can_be_merged_by?(current_user) || self.author == current_user end @@ -801,6 +802,16 @@ class MergeRequest < ApplicationRecord Gitlab::Utils.to_boolean(merge_params['force_remove_source_branch']) end + def auto_merge_strategy + return unless auto_merge_enabled? + + merge_params['auto_merge_strategy'] || AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS + end + + def auto_merge_strategy=(strategy) + merge_params['auto_merge_strategy'] = strategy + end + def remove_source_branch? should_remove_source_branch? || force_remove_source_branch? end @@ -973,15 +984,16 @@ class MergeRequest < ApplicationRecord end end - def reset_merge_when_pipeline_succeeds - return unless merge_when_pipeline_succeeds? + def reset_auto_merge + return unless auto_merge_enabled? - self.merge_when_pipeline_succeeds = false + self.auto_merge_enabled = false self.merge_user = nil if merge_params merge_params.delete('should_remove_source_branch') merge_params.delete('commit_message') merge_params.delete('squash_commit_message') + merge_params.delete('auto_merge_strategy') end self.save diff --git a/app/presenters/merge_request_presenter.rb b/app/presenters/merge_request_presenter.rb index ba0711ca867..9c44ed711a6 100644 --- a/app/presenters/merge_request_presenter.rb +++ b/app/presenters/merge_request_presenter.rb @@ -22,9 +22,9 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated end end - def cancel_merge_when_pipeline_succeeds_path - if can_cancel_merge_when_pipeline_succeeds?(current_user) - cancel_merge_when_pipeline_succeeds_project_merge_request_path(project, merge_request) + def cancel_auto_merge_path + if can_cancel_auto_merge?(current_user) + cancel_auto_merge_project_merge_request_path(project, merge_request) end end diff --git a/app/serializers/merge_request_widget_entity.rb b/app/serializers/merge_request_widget_entity.rb index b130f447cce..a428930dbbf 100644 --- a/app/serializers/merge_request_widget_entity.rb +++ b/app/serializers/merge_request_widget_entity.rb @@ -9,7 +9,11 @@ class MergeRequestWidgetEntity < IssuableEntity expose :merge_params expose :merge_status expose :merge_user_id - expose :merge_when_pipeline_succeeds + expose :auto_merge_enabled + expose :auto_merge_strategy + expose :available_auto_merge_strategies do |merge_request| + AutoMergeService.new(merge_request.project, current_user).available_strategies(merge_request) # rubocop: disable CodeReuse/ServiceClass + end expose :source_branch expose :source_branch_protected do |merge_request| merge_request.source_project.present? && ProtectedBranch.protected?(merge_request.source_project, merge_request.source_branch) @@ -182,8 +186,8 @@ class MergeRequestWidgetEntity < IssuableEntity presenter(merge_request).remove_wip_path end - expose :cancel_merge_when_pipeline_succeeds_path do |merge_request| - presenter(merge_request).cancel_merge_when_pipeline_succeeds_path + expose :cancel_auto_merge_path do |merge_request| + presenter(merge_request).cancel_auto_merge_path end expose :create_issue_to_resolve_discussions_path do |merge_request| diff --git a/app/services/auto_merge/merge_when_pipeline_succeeds_service.rb b/app/services/auto_merge/merge_when_pipeline_succeeds_service.rb new file mode 100644 index 00000000000..d0586468859 --- /dev/null +++ b/app/services/auto_merge/merge_when_pipeline_succeeds_service.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +module AutoMerge + class MergeWhenPipelineSucceedsService < BaseService + def execute(merge_request) + return :failed unless merge_request.actual_head_pipeline + + if merge_request.actual_head_pipeline.active? + merge_request.merge_params.merge!(params) + + unless merge_request.auto_merge_enabled? + merge_request.auto_merge_enabled = true + merge_request.merge_user = @current_user + merge_request.auto_merge_strategy = AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS + + SystemNoteService.merge_when_pipeline_succeeds(merge_request, @project, @current_user, merge_request.diff_head_commit) + end + + return :failed unless merge_request.save + + :merge_when_pipeline_succeeds + elsif merge_request.actual_head_pipeline.success? + # This can be triggered when a user clicks the auto merge button while + # the tests finish at about the same time + merge_request.merge_async(current_user.id, merge_params) + + :success + else + :failed + end + end + + def process(merge_request) + return unless merge_request.actual_head_pipeline&.success? + return unless merge_request.mergeable? + + merge_request.merge_async(merge_request.merge_user_id, merge_request.merge_params) + end + + def cancel(merge_request) + if merge_request.reset_auto_merge + SystemNoteService.cancel_merge_when_pipeline_succeeds(merge_request, @project, @current_user) + + success + else + error("Can't cancel the automatic merge", 406) + end + end + + def available_for?(merge_request) + merge_request.actual_head_pipeline&.active? + end + end +end diff --git a/app/services/auto_merge_service.rb b/app/services/auto_merge_service.rb new file mode 100644 index 00000000000..a3a780ff388 --- /dev/null +++ b/app/services/auto_merge_service.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +class AutoMergeService < BaseService + STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS = 'merge_when_pipeline_succeeds'.freeze + STRATEGIES = [STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS].freeze + + class << self + def all_strategies + STRATEGIES + end + + def get_service_class(strategy) + return unless all_strategies.include?(strategy) + + "::AutoMerge::#{strategy.camelize}Service".constantize + end + end + + def execute(merge_request, strategy) + service = get_service_instance(strategy) + + return :failed unless service&.available_for?(merge_request) + + service.execute(merge_request) + end + + def process(merge_request) + return unless merge_request.auto_merge_enabled? + + get_service_instance(merge_request.auto_merge_strategy).process(merge_request) + end + + def cancel(merge_request) + return error("Can't cancel the automatic merge", 406) unless merge_request.auto_merge_enabled? + + get_service_instance(merge_request.auto_merge_strategy).cancel(merge_request) + end + + def available_strategies(merge_request) + self.class.all_strategies.select do |strategy| + get_service_instance(strategy).available_for?(merge_request) + end + end + + private + + def get_service_instance(strategy) + self.class.get_service_class(strategy)&.new(project, current_user, params) + end +end diff --git a/app/services/merge_requests/merge_when_pipeline_succeeds_service.rb b/app/services/merge_requests/merge_when_pipeline_succeeds_service.rb deleted file mode 100644 index 973e5b64e88..00000000000 --- a/app/services/merge_requests/merge_when_pipeline_succeeds_service.rb +++ /dev/null @@ -1,47 +0,0 @@ -# frozen_string_literal: true - -module MergeRequests - class MergeWhenPipelineSucceedsService < MergeRequests::BaseService - # Marks the passed `merge_request` to be merged when the pipeline succeeds or - # updates the params for the automatic merge - def execute(merge_request) - merge_request.merge_params.merge!(params) - - # The service is also called when the merge params are updated. - already_approved = merge_request.merge_when_pipeline_succeeds? - - unless already_approved - merge_request.merge_when_pipeline_succeeds = true - merge_request.merge_user = @current_user - - SystemNoteService.merge_when_pipeline_succeeds(merge_request, @project, @current_user, merge_request.diff_head_commit) - end - - merge_request.save - end - - # Triggers the automatic merge of merge_request once the pipeline succeeds - def trigger(pipeline) - return unless pipeline.success? - - pipeline_merge_requests(pipeline) do |merge_request| - next unless merge_request.merge_when_pipeline_succeeds? - next unless merge_request.mergeable? - - merge_request.merge_async(merge_request.merge_user_id, merge_request.merge_params) - end - end - - # Cancels the automatic merge - def cancel(merge_request) - if merge_request.merge_when_pipeline_succeeds? && merge_request.open? - merge_request.reset_merge_when_pipeline_succeeds - SystemNoteService.cancel_merge_when_pipeline_succeeds(merge_request, @project, @current_user) - - success - else - error("Can't cancel the automatic merge", 406) - end - end - end -end diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb index 3abea1ad1ae..08130a531ee 100644 --- a/app/services/merge_requests/refresh_service.rb +++ b/app/services/merge_requests/refresh_service.rb @@ -24,7 +24,7 @@ module MergeRequests reload_merge_requests outdate_suggestions refresh_pipelines_on_merge_requests - reset_merge_when_pipeline_succeeds + cancel_auto_merge mark_pending_todos_done cache_merge_requests_closing_issues @@ -142,8 +142,10 @@ module MergeRequests end end - def reset_merge_when_pipeline_succeeds - merge_requests_for_source_branch.each(&:reset_merge_when_pipeline_succeeds) + def cancel_auto_merge + merge_requests_for_source_branch.each do |merge_request| + AutoMergeService.new(project, current_user).cancel(merge_request) + end end def mark_pending_todos_done diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index 55546432ce4..6a0f3000ffb 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -89,7 +89,7 @@ module MergeRequests merge_request.update(merge_error: nil) if merge_request.head_pipeline && merge_request.head_pipeline.active? - MergeRequests::MergeWhenPipelineSucceedsService.new(project, current_user).execute(merge_request) + AutoMergeService.new(project, current_user).execute(merge_request, AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS) else merge_request.merge_async(current_user.id, {}) end diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index f797c0f11c6..5aa804666f0 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -238,7 +238,7 @@ class NotificationService merge_request, current_user, :merged_merge_request_email, - skip_current_user: !merge_request.merge_when_pipeline_succeeds? + skip_current_user: !merge_request.auto_merge_enabled? ) end diff --git a/app/workers/pipeline_success_worker.rb b/app/workers/pipeline_success_worker.rb index 4f349ed922c..ce6c88c85c1 100644 --- a/app/workers/pipeline_success_worker.rb +++ b/app/workers/pipeline_success_worker.rb @@ -9,9 +9,10 @@ class PipelineSuccessWorker # rubocop: disable CodeReuse/ActiveRecord def perform(pipeline_id) Ci::Pipeline.find_by(id: pipeline_id).try do |pipeline| - MergeRequests::MergeWhenPipelineSucceedsService - .new(pipeline.project, nil) - .trigger(pipeline) + pipeline.all_merge_requests.preload(:merge_user).each do |merge_request| + AutoMergeService.new(pipeline.project, merge_request.merge_user) + .process(merge_request) + end end end # rubocop: enable CodeReuse/ActiveRecord |