Follow-up from "MWCP requires a successful pipeline if project has ci"
The following discussion from !146006 (merged) should be addressed:
-
@drew started a discussion: (+5 comments) So using
!merge_request.actual_head_pipeline_success?
here is, I think, only needed for the case when CI is currently running and we need to to set MWPS to true.Otherwise,
return unless merge_request.mergeable?
runs through theCheckCiStatusService
, which you're modifying above to trigger if MWPS is true and then checks#mergeable_ci_state?
, which has the more nuanced version of "pipeline success":def mergeable_ci_state? return true unless only_allow_merge_if_pipeline_succeeds? return false unless actual_head_pipeline return true if project.allow_merge_on_skipped_pipeline?(inherit_group_setting: true) && actual_head_pipeline.skipped? actual_head_pipeline.success? end
So, I think we need it in this class, because I think we need it for deciding to set MWPS. But I don't think we need it for this. Does that make sense?
@drew: Okay @marc_shaw, I wrote out a big comment explaining exactly the trace through this workflow, and in doing so I came to understand how this is going to work
😅 It's almost redundant, and it would have been in a world where we only used the project-wide CI requirement. Because at the top of
mergeable_ci_state?
, we check for the setting that we've now decided we can't rely on people to have configured.def mergeable_ci_state? return true unless only_allow_merge_if_pipeline_succeeds?
So adding the check here makes the MWCP service require this independently of the setting. Grand grand grand.
🙇 However, I also currently have a very wide view of this logic in my head, and am somewhat upset at how dispersed throughout the app we've managed to make this > logic. So I'm going to create an unscheduled follow-up issue from this discussion. As we make change to this feature and the merge workflow, let's try to work > this check back into the mergability logic of the MergeRequest itself