Skip to content

Rearchitect MR widget mergeability logic

Problem to solve

The MR widget feature that shows the mergeability status today has logic that is implemented partially in the backend and partially in the frontend. Because the MR widget JSON sent by the backend to the frontend contains a lot of information, the frontend, overtime has used this data to make additional computations. This has resulted in faster frontend iterations (since it didn't require a backend change to provided the needed data) but has caused business logic to leak into the frontend.

Overtime, some logic has diverged between frontend and backend, causing bugs to emerge:

In addition to that, the mergeability status of the MR widget is an old and complex code (both on the frontend as well as the backend) where the responsibility is shared across multiple groups.

Goal

  • We want to have a MR widget that receives all the data from the backend, where no extra computations are needed in the frontend, except for basic UI representations.
  • The backend represents the SSOT for business logic around mergeability status.
  • The code ownership of each component at play is clearly defined

Details

Possible architecture

  • The MergeRequest#mergeable? method could run a sequence of mergeability checks, one after the other. The returning value could be a success result object, if all the checks passed, or a failure result object, containing details of the first failing check. This code is generic and abstracted from any implementation details. Maintainers of Merge Request core logic would be responsible for this.
  • Each mergeability check is a concrete class that runs a specific check. Each check is maintained by the team responsible for the domain logic. E.g. ~"group::continuous integration" would be responsible for the check against the MR head pipeline, according to the CI/CD project settings. groupcode review would be responsible for the check that ensures all discussions are resolved, etc.
  • Instead of the backend returning a mergeable: true/false to the frontend, we could return a richer object that the frontend could use to display the status and related actions. An rough idea could be:
{
  merge_strategy: nil,
  error: {
    origin: 'Ci::MergeAllowedByPipeline',
    message: 'The merge request cannot be merged because the pipeline is failed',
    actions: 'Retry the pipeline or push a new commit to fix the failure'
  }
}
{
  merge_strategy: {
    name: 'merge_when_pipeline_succeeds', # other values: merge, merge_train, merge_train_when_pipeline_succeed, nil
    description: _('Merge when pipeline succeeds')
  },
  error: nil,
}
{
  merge_strategy: {
    name: 'merge', # when project setting `pipeline_must_succeed: false` we allow merge with a red button
    description: _('Merge'),
    warning: _('The pipeline failed'),
  },
  error: nil,
}

This JSON result should satisfy the Merge button design principles - cc @pedroms

Benefits for the frontend

TODO:

Benefits overall

  • We could extend the mergeability checks in the future by adding new check classes in the backend, without any frontend change
  • mergeability UX is identical whether we are using the UI, API, etc since all business logic is consolidated in the backend
  • most of the logic would not require feature specs since it could be unit tested
  • TBC

References

Recording of initial discussion: https://gitlab.zoom.us/rec/share/kiXdfIm0OlrnLvUGtl2ioXllvd4xcJAqszRNlnUYI5stDCjJL6xx1yewuSQQShs9.ICqoHXpmeHe-z61f

This page may contain information related to upcoming products, features and functionality. It is important to note that the information presented is for informational purposes only, so please do not rely on the information for purchasing or planning purposes. Just like with all projects, the items mentioned on the page are subject to change or delay, and the development, release, and timing of any products, features, or functionality remain at the sole discretion of GitLab Inc.

Edited by 🤖 GitLab Bot 🤖