Option to disallow author to approve merge request
Description including problem, use cases, benefits, and/or goals
To prevent having to add Approvers amount + 1, because the author can approve themselves, it is suggested to not allow the author of the MR be able to approve a MR.
Proposal
Non-configurable. Simply don't allow the author to approve.
This does not apply to Accepting merge requests, which should be tackled separately.
Links / references
Original issue
Similar to gitlab-org/gitlab-ce#5940 (Don't allow accepting MR with failed build)
Zendesk issue: https://gitlab.zendesk.com/agent/tickets/17096
Strictly do not allow merge request author to accept or approve their own merge request, optionally.
cc/ @JobV
Activity
I think if you enable approvals, the author shouldn't be able to approve it. Let's not make it an option.
In line with: https://gitlab.com/gitlab-org/gitlab-ce/issues/13411
I agree with @JobV it should not be an option. Just don't allow author to approve.
Agree with @JobV.
@DouweM Schedule? By not making it optional I think 8.7 is realistic.
@rspeicher I don't think so, judging by how full 8.7 is already. I'll put it for 8.8.
Milestone changed to 8.8
Milestone changed to 8.9
@dblessing I rewrote the description. Please see if we need to create another for accepting MRs.
mentioned in issue #580
Reassigned to @smcgivern
Started branch
388-option-to-disallow-author-to-approve-merge-requestmentioned in merge request !455 (merged)
Status changed to closed by merge request !455 (merged)
mentioned in merge request !479 (closed)
Milestone changed to %8.10
I implemented this in !455 (merged) but I didn't really think it through properly: both @mvaneijk and @rabbitfang had problems with their workflow on GitLab.com once the RC containing this feature was deployed there.
As I understand it (@mvaneijk and @rabbitfang, please jump in if this is wrong), the problem was like this:
- You have a project with two approvers.
- Every MR must be approved by those two.
- If one of the two approvers creates an MR, it can't be approved, because the author can't approve their own MR and there aren't any approvers left.
- A workaround for this is to edit the MR to remove the author as an allowed approver. The MR can then be approved by anyone with access to the project.
- Another option is to change the project-level approver count to one fewer than it was.
Obviously an MR should never get into a 'stuck' state like this, but I'm not sure what the most intuitive behaviour is. Perhaps we we should disallow the author from being an approver (remove them from the default list if present, don't let them be added to the list), and then just treat them like any other project member?
This would allow the author to approve their own MR if we've run out of approvers, but not special-case the author beyond validating that they aren't in the approvers list. The only caveat to that is that the MR author can edit the approvers list to both add and remove approvers, and maybe in that case we should only let them add, not remove. (Otherwise they could remove all approvers, then approve their own MR.)
Approvals left Approvers left Approver Project member 0 >= 0 ✗ ✗ >= 1 0 ✗ ✓ >= 1 >= 1 ✓ ✗ @JobV I hope that's reasonably clear, I'm not sure I have particularly coherent thoughts about this
@smcgivern With my use case, I am the sole approver in the project, and all MR's require 1 approval. The purpose of that is so I can act as a gatekeeper for contributions by others.
@rabbitfang thanks. Is that so that other people can merge their own MRs, but only you can approve them?
@smcgivern Sort of. Some of the contributors are masters/owner of the project so I do not want them to be able to accidentally merge the MR without me reviewing it.
Since I never really checked: can masters/owners always approve MR's?
@rabbitfang no, there's nothing special about masters or owners when it comes to approvals. What the contributors can do currently is set themselves as an approver of an MR, then approve it themselves. I don't think we have any additional restrictions on setting approvers, hence this issue
@smcgivern I'm not concerned about masters/owners being able to bypass the approval system if they know how and have the intent, but rather about them doing it accidentally, so it is fine how it currently is.
Thanks @rabbitfang, that makes sense. @JobV over to you
I think the change as proposed makes sense:
- disallow the author from being an approver for their own MR
- if no specific approvers are selected, just require N approvals from other projects members
- if no project members are left, require less approvals in total (project members - 1) => so even with 2 approvals required, you can have a project with 2 people that accepts outside contributions and needs you both to sign off on it.
For cases where you want something more related to access of specific people, like what @rabbitfang implies, it makes more sense to use branch permissions.
@smcgivern this makes sense?
Edited by 🚄 Job van der Voort 🚀@JobV it makes sense, as long as we understand that it doesn't work for @rabbitfang's case (which is fine, if there are other ways to accomplish the same thing). As that is a project with one approver but multiple members (I think), it would mean:
- The approver creates an MR.
- They can't approve it, so it's open to all project members.
- If there weren't enough project members left to approve it, we reduce the number of approvals needed. But as there are project members left, the MR still needs approval by one of them.
I'll also try to make the UI when you're creating an MR clearer if you're already an approver (remove you from the list, don't allow you to be added, etc.).
@smcgivern What's the status here? :)
@DouweM sorry, it was in my todos, I was just working on other things. I'm on it today
mentioned in merge request !560 (merged)
mentioned in commit 13cc2966
mentioned in commit 5e921f1a
Milestone changed to %8.11
@smcgivern Can this be closed now?
Derp, yes. This was closed by !560 (merged).
mentioned in issue #966
Mentioned in issue #966
Mentioned in issue #580
marked this issue as related to #3349
