MergeWhenChecksPassService doesn't account for MergeTrains being enabled
The Problem
During the Merge When Checks Pass rollout, we had a customer report that resolving discussions on a Merge Request was automatically merging immediately, and skipping the merge train MR.
They walked through an example of it live, and we confirmed that turning the flag off fixed the problem.
The Investigation
@drew Did a cursory review of the AutoMerge::MergeWhenChecksPassService
, which inherits directly from the existing AutoMerge::MergeWhenPipelineSucceedsService
. This makes sense, as it's the same behavior with some other questions put in front of the merge action.
The problem (it seems) is that until now when MergeTrains are enabled, we use a totally separate AutoMerge service called AutoMerge::AddToMergeTrainWhenPipelineSucceedsService
. This uses the MergeTrainService
to add the MR to the train, which will get around the merging later. It doesn't use MergeRequest#merge_async
, or directly merge the MR at all. I don't see anything like an AddToMergeTrainWhenChecksPassService
that would use the MergeTrainService
instead of merging directly.
The Upshot
AutoMerge::MergeWhenChecksPassService
looks perfectly built to do what we normally do with MWPS, it just seems to not be aware of MergeTrains at all. So it shouldn't be used when MergeTrains are enabled. It sees that there are no failing checks, it just merges right away.
- We should not allow MWCP to be used if MergeTrains are enabled.
- When should have another, separate service to... ATMTWCP
😉 🚂