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 | |
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
35 files changed, 478 insertions, 175 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 diff --git a/changelogs/unreleased/abstract-auto-merge.yml b/changelogs/unreleased/abstract-auto-merge.yml new file mode 100644 index 00000000000..d3069a3e500 --- /dev/null +++ b/changelogs/unreleased/abstract-auto-merge.yml @@ -0,0 +1,5 @@ +--- +title: Refactor and abstract Auto Merge Processes +merge_request: 28595 +author: +type: other diff --git a/config/routes/project.rb b/config/routes/project.rb index 840dc4c7844..d44ff62bc2a 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -208,7 +208,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do member do get :commit_change_content post :merge - post :cancel_merge_when_pipeline_succeeds + post :cancel_auto_merge get :pipeline_status get :ci_environments_status post :toggle_subscription diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index ce85772e4ed..955624404f1 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -386,9 +386,8 @@ module API ) if merge_when_pipeline_succeeds && merge_request.head_pipeline && merge_request.head_pipeline.active? - ::MergeRequests::MergeWhenPipelineSucceedsService - .new(merge_request.target_project, current_user, merge_params) - .execute(merge_request) + AutoMergeService.new(merge_request.target_project, current_user, merge_params) + .execute(merge_request, AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS) else ::MergeRequests::MergeService .new(merge_request.target_project, current_user, merge_params) @@ -429,11 +428,9 @@ module API post ':id/merge_requests/:merge_request_iid/cancel_merge_when_pipeline_succeeds' do merge_request = find_project_merge_request(params[:merge_request_iid]) - unauthorized! unless merge_request.can_cancel_merge_when_pipeline_succeeds?(current_user) + unauthorized! unless merge_request.can_cancel_auto_merge?(current_user) - ::MergeRequests::MergeWhenPipelineSucceedsService - .new(merge_request.target_project, current_user) - .cancel(merge_request) + AutoMergeService.new(merge_request.target_project, current_user).cancel(merge_request) end desc 'Rebase the merge request against its target branch' do diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index f4a18dcba51..f8c0ab55eb4 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -429,8 +429,9 @@ describe Projects::MergeRequestsController do it 'sets the MR to merge when the pipeline succeeds' do service = double(:merge_when_pipeline_succeeds_service) + allow(service).to receive(:available_for?) { true } - expect(MergeRequests::MergeWhenPipelineSucceedsService) + expect(AutoMerge::MergeWhenPipelineSucceedsService) .to receive(:new).with(project, anything, anything) .and_return(service) expect(service).to receive(:execute).with(merge_request) @@ -713,9 +714,9 @@ describe Projects::MergeRequestsController do end end - describe 'POST cancel_merge_when_pipeline_succeeds' do + describe 'POST cancel_auto_merge' do subject do - post :cancel_merge_when_pipeline_succeeds, + post :cancel_auto_merge, params: { format: :json, namespace_id: merge_request.project.namespace.to_param, @@ -725,14 +726,15 @@ describe Projects::MergeRequestsController do xhr: true end - it 'calls MergeRequests::MergeWhenPipelineSucceedsService' do - mwps_service = double + it 'calls AutoMergeService' do + auto_merge_service = double - allow(MergeRequests::MergeWhenPipelineSucceedsService) + allow(AutoMergeService) .to receive(:new) - .and_return(mwps_service) + .and_return(auto_merge_service) - expect(mwps_service).to receive(:cancel).with(merge_request) + allow(auto_merge_service).to receive(:available_strategies).with(merge_request) + expect(auto_merge_service).to receive(:cancel).with(merge_request) subject end diff --git a/spec/factories/merge_requests.rb b/spec/factories/merge_requests.rb index e8df5094b83..0b6a43b13a9 100644 --- a/spec/factories/merge_requests.rb +++ b/spec/factories/merge_requests.rb @@ -95,7 +95,8 @@ FactoryBot.define do end trait :merge_when_pipeline_succeeds do - merge_when_pipeline_succeeds true + auto_merge_enabled true + auto_merge_strategy AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS merge_user { author } end diff --git a/spec/features/merge_request/user_merges_when_pipeline_succeeds_spec.rb b/spec/features/merge_request/user_merges_when_pipeline_succeeds_spec.rb index d4ad11b3585..e7b92dc5535 100644 --- a/spec/features/merge_request/user_merges_when_pipeline_succeeds_spec.rb +++ b/spec/features/merge_request/user_merges_when_pipeline_succeeds_spec.rb @@ -74,11 +74,12 @@ describe 'Merge request > User merges when pipeline succeeds', :js do source_project: project, title: 'Bug NS-04', author: user, - merge_user: user, - merge_params: { force_remove_source_branch: '1' }) + merge_user: user) end before do + merge_request.merge_params['force_remove_source_branch'] = '0' + merge_request.save! click_link "Cancel automatic merge" end @@ -102,11 +103,11 @@ describe 'Merge request > User merges when pipeline succeeds', :js do context 'when merge when pipeline succeeds is enabled' do let(:merge_request) do - create(:merge_request_with_diffs, :simple, source_project: project, - author: user, - merge_user: user, - title: 'MepMep', - merge_when_pipeline_succeeds: true) + create(:merge_request_with_diffs, :simple, :merge_when_pipeline_succeeds, + source_project: project, + author: user, + merge_user: user, + title: 'MepMep') end let!(:build) do create(:ci_build, pipeline: pipeline) diff --git a/spec/features/merge_request/user_sees_merge_widget_spec.rb b/spec/features/merge_request/user_sees_merge_widget_spec.rb index 93ddde623fe..1477307ed7b 100644 --- a/spec/features/merge_request/user_sees_merge_widget_spec.rb +++ b/spec/features/merge_request/user_sees_merge_widget_spec.rb @@ -314,7 +314,8 @@ describe 'Merge request > User sees merge widget', :js do context 'view merge request with MWPS enabled but automatically merge fails' do before do merge_request.update( - merge_when_pipeline_succeeds: true, + auto_merge_enabled: true, + auto_merge_strategy: AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS, merge_user: merge_request.author, merge_error: 'Something went wrong' ) diff --git a/spec/frontend/vue_mr_widget/stores/get_state_key_spec.js b/spec/frontend/vue_mr_widget/stores/get_state_key_spec.js index b356ea85cad..9797549498b 100644 --- a/spec/frontend/vue_mr_widget/stores/get_state_key_spec.js +++ b/spec/frontend/vue_mr_widget/stores/get_state_key_spec.js @@ -31,7 +31,7 @@ describe('getStateKey', () => { expect(bound()).toEqual('notAllowedToMerge'); - context.mergeWhenPipelineSucceeds = true; + context.autoMergeEnabled = true; expect(bound()).toEqual('mergeWhenPipelineSucceeds'); diff --git a/spec/javascripts/vue_mr_widget/components/states/mr_widget_ready_to_merge_spec.js b/spec/javascripts/vue_mr_widget/components/states/mr_widget_ready_to_merge_spec.js index 368c997d318..daca51c6156 100644 --- a/spec/javascripts/vue_mr_widget/components/states/mr_widget_ready_to_merge_spec.js +++ b/spec/javascripts/vue_mr_widget/components/states/mr_widget_ready_to_merge_spec.js @@ -80,7 +80,7 @@ describe('ReadyToMerge', () => { it('should have default data', () => { expect(vm.mergeWhenBuildSucceeds).toBeFalsy(); expect(vm.useCommitMessageWithDescription).toBeFalsy(); - expect(vm.setToMergeWhenPipelineSucceeds).toBeFalsy(); + expect(vm.setToAutoMerge).toBeFalsy(); expect(vm.showCommitMessageEditor).toBeFalsy(); expect(vm.isMakingRequest).toBeFalsy(); expect(vm.isMergingImmediately).toBeFalsy(); @@ -91,17 +91,17 @@ describe('ReadyToMerge', () => { }); describe('computed', () => { - describe('shouldShowMergeWhenPipelineSucceedsText', () => { + describe('shouldShowAutoMergeText', () => { it('should return true with active pipeline', () => { vm.mr.isPipelineActive = true; - expect(vm.shouldShowMergeWhenPipelineSucceedsText).toBeTruthy(); + expect(vm.shouldShowAutoMergeText).toBeTruthy(); }); it('should return false with inactive pipeline', () => { vm.mr.isPipelineActive = false; - expect(vm.shouldShowMergeWhenPipelineSucceedsText).toBeFalsy(); + expect(vm.shouldShowAutoMergeText).toBeFalsy(); }); }); @@ -325,7 +325,7 @@ describe('ReadyToMerge', () => { vm.handleMergeButtonClick(true); setTimeout(() => { - expect(vm.setToMergeWhenPipelineSucceeds).toBeTruthy(); + expect(vm.setToAutoMerge).toBeTruthy(); expect(vm.isMakingRequest).toBeTruthy(); expect(eventHub.$emit).toHaveBeenCalledWith('MRWidgetUpdateRequested'); @@ -345,7 +345,7 @@ describe('ReadyToMerge', () => { vm.handleMergeButtonClick(false, true); setTimeout(() => { - expect(vm.setToMergeWhenPipelineSucceeds).toBeFalsy(); + expect(vm.setToAutoMerge).toBeFalsy(); expect(vm.isMakingRequest).toBeTruthy(); expect(eventHub.$emit).toHaveBeenCalledWith('FailedToMerge', undefined); @@ -363,7 +363,7 @@ describe('ReadyToMerge', () => { vm.handleMergeButtonClick(); setTimeout(() => { - expect(vm.setToMergeWhenPipelineSucceeds).toBeFalsy(); + expect(vm.setToAutoMerge).toBeFalsy(); expect(vm.isMakingRequest).toBeTruthy(); expect(vm.initiateMergePolling).toHaveBeenCalled(); diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index c72b6e9033d..43c61c48fe5 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -1038,14 +1038,28 @@ describe MergeRequest do end end - describe "#reset_merge_when_pipeline_succeeds" do + describe "#auto_merge_strategy" do + subject { merge_request.auto_merge_strategy } + + let(:merge_request) { create(:merge_request, :merge_when_pipeline_succeeds) } + + it { is_expected.to eq('merge_when_pipeline_succeeds') } + + context 'when auto merge is disabled' do + let(:merge_request) { create(:merge_request) } + + it { is_expected.to be_nil } + end + end + + describe "#reset_auto_merge" do let(:merge_if_green) do create :merge_request, merge_when_pipeline_succeeds: true, merge_user: create(:user), merge_params: { "should_remove_source_branch" => "1", "commit_message" => "msg" } end it "sets the item to false" do - merge_if_green.reset_merge_when_pipeline_succeeds + merge_if_green.reset_auto_merge merge_if_green.reload expect(merge_if_green.merge_when_pipeline_succeeds).to be_falsey diff --git a/spec/presenters/merge_request_presenter_spec.rb b/spec/presenters/merge_request_presenter_spec.rb index 0e1aed42cc5..6408b0bd748 100644 --- a/spec/presenters/merge_request_presenter_spec.rb +++ b/spec/presenters/merge_request_presenter_spec.rb @@ -207,25 +207,25 @@ describe MergeRequestPresenter do end end - describe '#cancel_merge_when_pipeline_succeeds_path' do + describe '#cancel_auto_merge_path' do subject do described_class.new(resource, current_user: user) - .cancel_merge_when_pipeline_succeeds_path + .cancel_auto_merge_path end context 'when can cancel mwps' do it 'returns path' do - allow(resource).to receive(:can_cancel_merge_when_pipeline_succeeds?) + allow(resource).to receive(:can_cancel_auto_merge?) .with(user) .and_return(true) - is_expected.to eq("/#{resource.project.full_path}/merge_requests/#{resource.iid}/cancel_merge_when_pipeline_succeeds") + is_expected.to eq("/#{resource.project.full_path}/merge_requests/#{resource.iid}/cancel_auto_merge") end end context 'when cannot cancel mwps' do it 'returns nil' do - allow(resource).to receive(:can_cancel_merge_when_pipeline_succeeds?) + allow(resource).to receive(:can_cancel_auto_merge?) .with(user) .and_return(false) diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 5c94a87529b..9f9180bc8c9 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -1473,7 +1473,7 @@ describe API::MergeRequests do end it "enables merge when pipeline succeeds if the pipeline is active" do - allow_any_instance_of(MergeRequest).to receive(:head_pipeline).and_return(pipeline) + allow_any_instance_of(MergeRequest).to receive_messages(head_pipeline: pipeline, actual_head_pipeline: pipeline) allow(pipeline).to receive(:active?).and_return(true) put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/merge", user), params: { merge_when_pipeline_succeeds: true } @@ -1484,7 +1484,7 @@ describe API::MergeRequests do end it "enables merge when pipeline succeeds if the pipeline is active and only_allow_merge_if_pipeline_succeeds is true" do - allow_any_instance_of(MergeRequest).to receive(:head_pipeline).and_return(pipeline) + allow_any_instance_of(MergeRequest).to receive_messages(head_pipeline: pipeline, actual_head_pipeline: pipeline) allow(pipeline).to receive(:active?).and_return(true) project.update_attribute(:only_allow_merge_if_pipeline_succeeds, true) @@ -1950,7 +1950,7 @@ describe API::MergeRequests do describe 'POST :id/merge_requests/:merge_request_iid/cancel_merge_when_pipeline_succeeds' do before do - ::MergeRequests::MergeWhenPipelineSucceedsService.new(merge_request.target_project, user).execute(merge_request) + ::AutoMergeService.new(merge_request.target_project, user).execute(merge_request, AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS) end it 'removes the merge_when_pipeline_succeeds status' do diff --git a/spec/serializers/merge_request_widget_entity_spec.rb b/spec/serializers/merge_request_widget_entity_spec.rb index b89898f26f7..a27c22191f4 100644 --- a/spec/serializers/merge_request_widget_entity_spec.rb +++ b/spec/serializers/merge_request_widget_entity_spec.rb @@ -297,4 +297,50 @@ describe MergeRequestWidgetEntity do end end end + + describe 'auto merge' do + context 'when auto merge is enabled' do + let(:resource) { create(:merge_request, :merge_when_pipeline_succeeds) } + + it 'returns auto merge related information' do + expect(subject[:auto_merge_enabled]).to be_truthy + expect(subject[:auto_merge_strategy]).to eq('merge_when_pipeline_succeeds') + end + end + + context 'when auto merge is not enabled' do + let(:resource) { create(:merge_request) } + + it 'returns auto merge related information' do + expect(subject[:auto_merge_enabled]).to be_falsy + expect(subject[:auto_merge_strategy]).to be_nil + end + end + + context 'when head pipeline is running' do + before do + create(:ci_pipeline, :running, project: project, + ref: resource.source_branch, + sha: resource.diff_head_sha) + resource.update_head_pipeline + end + + it 'returns available auto merge strategies' do + expect(subject[:available_auto_merge_strategies]).to eq(%w[merge_when_pipeline_succeeds]) + end + end + + context 'when head pipeline is finished' do + before do + create(:ci_pipeline, :success, project: project, + ref: resource.source_branch, + sha: resource.diff_head_sha) + resource.update_head_pipeline + end + + it 'returns available auto merge strategies' do + expect(subject[:available_auto_merge_strategies]).to be_empty + end + end + end end diff --git a/spec/services/merge_requests/merge_when_pipeline_succeeds_service_spec.rb b/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb index 96f61f3f103..a20bf8e17e4 100644 --- a/spec/services/merge_requests/merge_when_pipeline_succeeds_service_spec.rb +++ b/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -describe MergeRequests::MergeWhenPipelineSucceedsService do +describe AutoMerge::MergeWhenPipelineSucceedsService do let(:user) { create(:user) } let(:project) { create(:project, :repository) } @@ -21,6 +21,27 @@ describe MergeRequests::MergeWhenPipelineSucceedsService do described_class.new(project, user, commit_message: 'Awesome message') end + describe "#available_for?" do + subject { service.available_for?(mr_merge_if_green_enabled) } + + let(:pipeline_status) { :running } + + before do + create(:ci_pipeline, pipeline_status, ref: mr_merge_if_green_enabled.source_branch, + sha: mr_merge_if_green_enabled.diff_head_sha, + project: mr_merge_if_green_enabled.source_project) + mr_merge_if_green_enabled.update_head_pipeline + end + + it { is_expected.to be_truthy } + + context 'when the head piipeline succeeded' do + let(:pipeline_status) { :success } + + it { is_expected.to be_falsy } + end + end + describe "#execute" do let(:merge_request) do create(:merge_request, target_project: project, source_project: project, @@ -30,8 +51,7 @@ describe MergeRequests::MergeWhenPipelineSucceedsService do context 'first time enabling' do before do allow(merge_request) - .to receive(:head_pipeline) - .and_return(pipeline) + .to receive_messages(head_pipeline: pipeline, actual_head_pipeline: pipeline) service.execute(merge_request) end @@ -39,7 +59,7 @@ describe MergeRequests::MergeWhenPipelineSucceedsService do it 'sets the params, merge_user, and flag' do expect(merge_request).to be_valid expect(merge_request.merge_when_pipeline_succeeds).to be_truthy - expect(merge_request.merge_params).to eq commit_message: 'Awesome message' + expect(merge_request.merge_params).to include commit_message: 'Awesome message' expect(merge_request.merge_user).to be user end @@ -54,8 +74,8 @@ describe MergeRequests::MergeWhenPipelineSucceedsService do let(:build) { create(:ci_build, ref: mr_merge_if_green_enabled.source_branch) } before do - allow(mr_merge_if_green_enabled).to receive(:head_pipeline) - .and_return(pipeline) + allow(mr_merge_if_green_enabled) + .to receive_messages(head_pipeline: pipeline, actual_head_pipeline: pipeline) allow(mr_merge_if_green_enabled).to receive(:mergeable?) .and_return(true) @@ -72,7 +92,7 @@ describe MergeRequests::MergeWhenPipelineSucceedsService do end end - describe "#trigger" do + describe "#process" do let(:merge_request_ref) { mr_merge_if_green_enabled.source_branch } let(:merge_request_head) do project.commit(mr_merge_if_green_enabled.source_branch).id @@ -86,8 +106,11 @@ describe MergeRequests::MergeWhenPipelineSucceedsService do end it "merges all merge requests with merge when the pipeline succeeds enabled" do + allow(mr_merge_if_green_enabled) + .to receive_messages(head_pipeline: triggering_pipeline, actual_head_pipeline: triggering_pipeline) + expect(MergeWorker).to receive(:perform_async) - service.trigger(triggering_pipeline) + service.process(mr_merge_if_green_enabled) end end @@ -99,7 +122,7 @@ describe MergeRequests::MergeWhenPipelineSucceedsService do it 'does not merge request' do expect(MergeWorker).not_to receive(:perform_async) - service.trigger(old_pipeline) + service.process(mr_merge_if_green_enabled) end end @@ -111,7 +134,7 @@ describe MergeRequests::MergeWhenPipelineSucceedsService do it 'does not merge request' do expect(MergeWorker).not_to receive(:perform_async) - service.trigger(unrelated_pipeline) + service.process(mr_merge_if_green_enabled) end end @@ -125,8 +148,11 @@ describe MergeRequests::MergeWhenPipelineSucceedsService do end it 'merges the associated merge request' do + allow(mr_merge_if_green_enabled) + .to receive_messages(head_pipeline: pipeline, actual_head_pipeline: pipeline) + expect(MergeWorker).to receive(:perform_async) - service.trigger(pipeline) + service.process(mr_merge_if_green_enabled) end end end diff --git a/spec/services/auto_merge_service_spec.rb b/spec/services/auto_merge_service_spec.rb new file mode 100644 index 00000000000..d0eefed3150 --- /dev/null +++ b/spec/services/auto_merge_service_spec.rb @@ -0,0 +1,140 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe AutoMergeService do + set(:project) { create(:project) } + set(:user) { create(:user) } + let(:service) { described_class.new(project, user) } + + describe '.all_strategies' do + subject { described_class.all_strategies } + + it 'returns all strategies' do + is_expected.to eq(AutoMergeService::STRATEGIES) + end + end + + describe '#available_strategies' do + subject { service.available_strategies(merge_request) } + + let(:merge_request) { create(:merge_request) } + let(:pipeline_status) { :running } + + before do + create(:ci_pipeline, pipeline_status, ref: merge_request.source_branch, + sha: merge_request.diff_head_sha, + project: merge_request.source_project) + + merge_request.update_head_pipeline + end + + it 'returns available strategies' do + is_expected.to include('merge_when_pipeline_succeeds') + end + + context 'when the head piipeline succeeded' do + let(:pipeline_status) { :success } + + it 'returns available strategies' do + is_expected.to be_empty + end + end + end + + describe '.get_service_class' do + subject { described_class.get_service_class(strategy) } + + let(:strategy) { AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS } + + it 'returns service instance' do + is_expected.to eq(AutoMerge::MergeWhenPipelineSucceedsService) + end + + context 'when strategy is not present' do + let(:strategy) { } + + it 'returns nil' do + is_expected.to be_nil + end + end + end + + describe '#execute' do + subject { service.execute(merge_request, strategy) } + + let(:merge_request) { create(:merge_request) } + let(:pipeline_status) { :running } + let(:strategy) { AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS } + + before do + create(:ci_pipeline, pipeline_status, ref: merge_request.source_branch, + sha: merge_request.diff_head_sha, + project: merge_request.source_project) + + merge_request.update_head_pipeline + end + + it 'delegates to a relevant service instance' do + expect_next_instance_of(AutoMerge::MergeWhenPipelineSucceedsService) do |service| + expect(service).to receive(:execute).with(merge_request) + end + + subject + end + + context 'when the head piipeline succeeded' do + let(:pipeline_status) { :success } + + it 'returns failed' do + is_expected.to eq(:failed) + end + end + end + + describe '#process' do + subject { service.process(merge_request) } + + let(:merge_request) { create(:merge_request, :merge_when_pipeline_succeeds) } + + it 'delegates to a relevant service instance' do + expect_next_instance_of(AutoMerge::MergeWhenPipelineSucceedsService) do |service| + expect(service).to receive(:process).with(merge_request) + end + + subject + end + + context 'when auto merge is not enabled' do + let(:merge_request) { create(:merge_request) } + + it 'returns nil' do + is_expected.to be_nil + end + end + end + + describe '#cancel' do + subject { service.cancel(merge_request) } + + let(:merge_request) { create(:merge_request, :merge_when_pipeline_succeeds) } + + it 'delegates to a relevant service instance' do + expect_next_instance_of(AutoMerge::MergeWhenPipelineSucceedsService) do |service| + expect(service).to receive(:cancel).with(merge_request) + end + + subject + end + + context 'when auto merge is not enabled' do + let(:merge_request) { create(:merge_request) } + + it 'returns error' do + expect(subject[:message]).to eq("Can't cancel the automatic merge") + expect(subject[:status]).to eq(:error) + expect(subject[:http_status]).to eq(406) + end + end + end +end diff --git a/spec/services/merge_requests/push_options_handler_service_spec.rb b/spec/services/merge_requests/push_options_handler_service_spec.rb index f7a39bb42d5..54b9c6dae38 100644 --- a/spec/services/merge_requests/push_options_handler_service_spec.rb +++ b/spec/services/merge_requests/push_options_handler_service_spec.rb @@ -76,10 +76,11 @@ describe MergeRequests::PushOptionsHandlerService do shared_examples_for 'a service that can set the merge request to merge when pipeline succeeds' do subject(:last_mr) { MergeRequest.last } - it 'sets merge_when_pipeline_succeeds' do + it 'sets auto_merge_enabled' do service.execute - expect(last_mr.merge_when_pipeline_succeeds).to eq(true) + expect(last_mr.auto_merge_enabled).to eq(true) + expect(last_mr.auto_merge_strategy).to eq(AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS) end it 'sets merge_user to the user' do diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index 7258428589f..6ba67c7165c 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -23,7 +23,8 @@ describe MergeRequests::RefreshService do source_branch: 'master', target_branch: 'feature', target_project: @project, - merge_when_pipeline_succeeds: true, + auto_merge_enabled: true, + auto_merge_strategy: AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS, merge_user: @user) @another_merge_request = create(:merge_request, @@ -31,7 +32,8 @@ describe MergeRequests::RefreshService do source_branch: 'master', target_branch: 'test', target_project: @project, - merge_when_pipeline_succeeds: true, + auto_merge_enabled: true, + auto_merge_strategy: AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS, merge_user: @user) @fork_merge_request = create(:merge_request, @@ -83,7 +85,7 @@ describe MergeRequests::RefreshService do expect(@merge_request.notes).not_to be_empty expect(@merge_request).to be_open - expect(@merge_request.merge_when_pipeline_succeeds).to be_falsey + expect(@merge_request.auto_merge_enabled).to be_falsey expect(@merge_request.diff_head_sha).to eq(@newrev) expect(@fork_merge_request).to be_open expect(@fork_merge_request.notes).to be_empty @@ -292,7 +294,7 @@ describe MergeRequests::RefreshService do expect(@merge_request.notes).not_to be_empty expect(@merge_request).to be_open - expect(@merge_request.merge_when_pipeline_succeeds).to be_falsey + expect(@merge_request.auto_merge_enabled).to be_falsey expect(@merge_request.diff_head_sha).to eq(@newrev) expect(@fork_merge_request).to be_open expect(@fork_merge_request.notes).to be_empty diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index ba4c9ce60f3..fbfcd95e204 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -217,8 +217,9 @@ describe MergeRequests::UpdateService, :mailer do head_pipeline_of: merge_request ) - expect(MergeRequests::MergeWhenPipelineSucceedsService).to receive(:new).with(project, user) + expect(AutoMerge::MergeWhenPipelineSucceedsService).to receive(:new).with(project, user, {}) .and_return(service_mock) + allow(service_mock).to receive(:available_for?) { true } expect(service_mock).to receive(:execute).with(merge_request) end diff --git a/spec/workers/pipeline_success_worker_spec.rb b/spec/workers/pipeline_success_worker_spec.rb index 4cbe384b47a..b511edfa620 100644 --- a/spec/workers/pipeline_success_worker_spec.rb +++ b/spec/workers/pipeline_success_worker_spec.rb @@ -5,12 +5,13 @@ require 'spec_helper' describe PipelineSuccessWorker do describe '#perform' do context 'when pipeline exists' do - let(:pipeline) { create(:ci_pipeline, status: 'success') } + let(:pipeline) { create(:ci_pipeline, status: 'success', ref: merge_request.source_branch, project: merge_request.source_project) } + let(:merge_request) { create(:merge_request) } it 'performs "merge when pipeline succeeds"' do - expect_any_instance_of( - MergeRequests::MergeWhenPipelineSucceedsService - ).to receive(:trigger) + expect_next_instance_of(AutoMergeService) do |auto_merge| + expect(auto_merge).to receive(:process) + end described_class.new.perform(pipeline.id) end |