Merge when pipeline succeeeds is not respected when new discussions are opened
Summary
I noticed this MR: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/14399 and made a couple of comments on it while MWPS was set but before the pipeline was green.
This created "new unresolved discussions", which led to the MWPS flag being ignored when the pipeline succeeded.
Once the discussions were resolved (no further changes to the code being made), the MWPS status was shown again. However, the merge was not initiated
Steps to reproduce
Set an MR to MWPS, then create some new discussions before the pipeline completes. Resolve the discussions after hte pipeline completes
What is the current bug behavior?
The MWPS flag is left untouched but not respected, and manual action is required to get it to merge.
In my case, I had to press "cancel automatic merge", then "merge" again once the pipeline was green.
What is the expected correct behavior?
I'm not 100% certain.
First, we could merge it anyway despite the new discussions. Anyone can create a discussion, but the MWPS flag is between reviewer and reviewee - it's not entirely reasonable that any third party with permissions to create a discussion can block the merge.
If we do want discussions to block merge, one option is just to remove the MWPS flag when a new discussion is created. This prevents the nasty "stalled MWPS" state we currently see once the pipeline completes.
Another option is to initiate the merge when the final discussion is resolved, if the pipeline is green and MWPS is set. This will lead to the merge event that was skipped when the pipeline was green being reinstated.
I don't like the last option - it doesn't gel with how I treat discussions at all well, and could lead to code being merged unexpectedly. Overall, I think I'd prefer the first behaviour. It's what I expected would happen when I started peppering an MR I had no real involvement in with comments! :)
/cc @zj