Skip to content

Investigation: Support merge train in quick actions

Shinya Maeda requested to merge support-merge-train-in-quick-action into master

What does this MR do?

This MR is about checking code health around merge strategy and required refactoring for achieving #32336 (closed).

Issues:

  • AutoMergeService(MWPS/MT/ATMTWPS) is a super set of MergeService (merge immediately). We should have a single service/interface to automatically choose the preferred merge method for the merge request.
  • Devs are trying to support MWPS only with a specific parameter such as merge_when_pipeline_succeeds=true, which doesn't scale well. We should simplify the interface to support all merge strategy in a single interface (hiding the complexity under the MergeScheduleService/MergeStrategies domain)
  • There are multiple paths to merge the code:
    1. Merge by clicking a button on MR (UI) - It supports all merge strategies.
    2. Merge by slash command - It supports only immediate merge and MWPS.
    3. Merge by API - It supports only immediate merge and MWPS.
    4. Merge by push option - It supports only immediate merge and MWPS.
  • The problem is that one path supports all of the merge strategies but the other paths don't support. Each path has own regulation. There are no centralized logic.
  • Each path clears merge_error before the process. We should generalize it. merge_error should be a single source of truth for merge failure on merge strategies.
  • Each path has common validation before the process. We should generalize it.
  • Users/FE should be able to specify a specific merge strategy from available_merge_strategies.
  • BE should expose preferred_merge_strategy to reduce the FE complexity.
  • MergeService should automatically choose a preferred_merge_strategy if merge_strategy is not specified.
  • Probably better to save merge (immediately) into merge_request.merge_params for tracking purpose. i.e. merge_params[:merge_strategy] == :merge means the MR was merged immdiately.

Current Architecture:

  • MergeRequests::MergeService ... The service for merging MRs immediately.
    • MergeRequests::FfMergeService ... The extension to merge as FastForward.
  • AutoMergeService ... The main service to handle all of the auto merge logic.
  • AutoMerge::XXXService ... An individual service for a specific auto merge strategy.

Usage Example:

if immediately_mergeable
  ::MergeRequests::MergeService
    .new(merge_request.target_project, current_user, merge_params)
    .execute(merge_request)
elsif automatically_mergeable
  AutoMergeService.new(merge_request.target_project, current_user, merge_params)
    .execute(merge_request, AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS)
end

Next Architecture:

  • MergeRequests::MergeService ... The main service for merging MRs. It automatically handles the complexity of all of the merge strategies.
  • MergeRequests::MergeStrategies::Immediate::MergeService ... The service for merging MRs immediately.
  • MergeRequests::MergeStrategies::Immediate::FfMergeService ... The extension to merge as FastForward.
  • MergeRequests::MergeStrategies::Auto::MergeWhenPipelineSucceedsService ... One of the auto merge strategy.
  • MergeRequests::MergeStrategies::Auto::MergeTrain ... One of the auto merge strategy.
  • MergeRequests::MergeStrategies::Auto::AddToMergeTrainWhenPipelineSucceeds ... One of the auto merge strategy.

Goal:

  • Support all merge strategies in all paths.

Breakdown:

  • Introduce a unified architecture
  • Applies the new architecture to 1. Merge by clicking a button on MR (UI)
  • Applies the new architecture to 1. Merge by slash command
  • Applies the new architecture to 1. Merge by API
  • Applies the new architecture to 1. Merge by push option

Related #32336 (closed)

Screenshots

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team
Edited by Shinya Maeda

Merge request reports