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:
- #214742 (closed)
- #35562 (closed)
- #291168 (closed)
- #35135 (closed)
- #212991 (closed)
- #30947 (closed)
- #321001 (closed)
- #322389 (closed)
- &5455 (closed) contains a comprehensive list of current bugs
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
- Most of this code in
ready_to_merge.vue
could be removed -
mr_widget_options.vue
could be cleaned up from CI code -
ready_to_merge.js
files on both Core and EE could be cleaned up as well
TODO:
- Merge Immediately can sometimes be shown as dangerous action.
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.