Option to turn on Self Approval of Merge Requests for a Master

Description

With reference to this https://gitlab.com/gitlab-org/gitlab-ee/issues/388, a customer proposing:

Before it was required that two persons give the approval and it didn’t matter if one of the approvers was the creator of a merge request. Since the approvers where two masters it was ideal for us. A normal developer needed two approvals where as a master could give his own approval.

Proposal

It would be great if there would be an exception for masters, so that we could always add a master to the list of approvers. Also I like to suggest that GitLab should support hooks in such places, I can imagine that it’s not easy to satisfy all the needs of different customers and that might really help here. If the hook is missing the default rule (at the point of project creation) is applied, otherwise the hook is used, so you can keep gitlab easy to setup for all others. The hook might get the following input parameters: project, source branch, target branch, existingApprovers, newApprover and just return a boolean.

Links / references

https://gitlab.zendesk.com/agent/tickets/36476 https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/560

Added feature proposal label

Added ~113221 label

@regisF @smcgivern (as you tackled https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/560) Can you take a look at this proposal?

/cc @vsizov too as he is the approvals king

The main thing that worries me is how complicated the logic for figuring out who a valid approver is anyway. That's not to say we shouldn't do it, but this code's already pretty hairy.

I'm also not sure how the hook would look here, but maybe that's just a lack of imagination.

@SeanPackham As an alternative, why not providing the ability to install a hook somewhere on the server? Such a hook could allow each customer to define it's own logic. The hook could have the arguments projectid, source-branch, target-branch, userid of the creator. The result of the script would be a list of the allowed approvers.

Yeah, for sure, the approvals feature now is very complex part of project. After https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/743 it become even more complex.

@abuango This issue contains two proposals actually: Self-approvals and Hooks. Maybe it's worth to create two separate issues for them.

@heiko_boettger I think you mentioned the wrong Sean :)

@heiko_boettger so would the hook run inside the context of the GitLab application, or outside? If outside, where would it get the user IDs? Apologies if these are dumb questions, I just want to understand what it would look like.

@smcgivern well I though about a custom_hook folder somewhere on the server similar to the git hooks where the hook-scripts can be placed. It could be just a shell script taking the arguments I proposed (may be adding the url of the gitlab server as well) and inside the script I would use the gitlab api to access more information such as getting the user list of the project, group or globally. To make it simpler it might make sense to use the mail-addresses instead of the ids . That way one could just write a simple script printing the content of a hardcoded textfile containing the mail addresses or the more complicated stuff as listing all users of the project and filtering out the merge request owners unless they have the role master. However I currently do not know if there is the possibility to get the effective role (inherited from the group) of a user on a project via the gitlab api, this might be avoided by sending the list of users on the project via stdin. One thing to consider is the influence of performance on the gui, it is definitely slower stating a process and query all this information via the gitlab api. I could also live with implementing a ruby script somewhere.

@vsizov The hook is just an idea, I think the original request to make an exception for masters came from me because I thought is easier to implement than a hook.

@heiko_boettger thanks for the explanation!

@vsizov what do you think?

I like the idea of having hook, but I think hook should be a web call and not shell script. The reason is that it's scalable and it's relatively safe. UPDATE: And you don't have to have access to server.

Wouldn't that be slow, if we have to call that on every MR view?

It will be slow, but we could implement the config option if we need to check the hook. On the other hand we would have to implement more UI with Web Hook. So I'm not certain about it.

The Shell Hook I don't like either, because it's hard to scale if you have more then one application server.

@vsizov Today I again was asked about this issue. The change to disallow the owner as an approver broke our workflow. Reducing the number of approvals is not an option since a change from a non-master needs to be approved by two masters. Is there a quick fix for that?

@heiko_boettger

The change to disallow the owner as an approver broke our workflow

Did we change that? AFAIK the only author of MR can't be an approver which is reasonable.

@vsizov Sorry for the confusion, when I wrote owner I meant the author of the merge request and this behaviour was changed by !560 (merged) introduced in 8.10. In issue #388 (closed) there is a nice table and some points above it which also describe the scenario of two masters. The currently implemented solution of magically reducing the number of approvers in the cases where a merge would otherwise not be possible "just" needs to be extended a bit.

I agree it's reasonable but it doesn't change anything about the fact that a master now need to have the approval of some there developer. And my coworkers are starting to complain everytime one of the masters is in vacation. Since I could imagine that every one has different needs on how approval should work, I guess a custom hook would be the best.

Hi, just want to ask if there is any progress In this area.

That's not scheduled yet.

We just had another customer ask for this https://gitlab.zendesk.com/agent/tickets/49001

hey, thanks @dewetblomerus,

this looks like it can solve our problem (for allowing users to create merge requests that they themselves can close), but from what @smcgivern said, i can tell that's it's not a very trivial solution and it could take some time to develope..

my suggestion was not that complex and might get some customers off your back quickly: just add a flag in the project settings that allows a merge request to set a number of approvers lower than the default.

@aviv2 'just' adding a project flag is also not that simple https://about.gitlab.com/handbook/product/#convention-over-configuration

I'd love to come up with some kind of 'unified theory of approvals' that would cover all the cases people want out of this. @victorwu, maybe one for you to think about too! There's a surprising amount of complexity in approving MRs.

Added ~238305 label

And another customer https://gitlab.zendesk.com/agent/tickets/49510

hey De Wet, this is a dead link.. perhaps it was deleted?

thanks, aviv

Hi @aviv2, these links are currently internal only for GitLab support requests.

Just noting that this would be exceptionally useful for us as well to have this implemented.

Customer requested this in a ticket here: https://gitlab.zendesk.com/agent/tickets/50830 customer tag was already in place.

Since !560 (merged) was merged gitlab is also standing in our way in this regard. If person X creates a MR with commits from person Y, why can X not be an approver? The assumption that the person creating the MR is the same as the person doing the changes is plain wrong. Gitlab should not force any workflow here.

@t-b that's a very good point! We are consistent in using the MR creator as the 'author', but you're right that this doesn't always hold. (In fact, the commits can be from someone who doesn't even have an account on the GitLab instance.) I don't have a good suggestion for you at the moment, but I did want to acknowledge that.

Would love to see movement on this, it'd help eliminate some serious frustrations on our team's workflow as well.

Having this option would be awesome as well. Being the manager of our development team, I should be able to merge any requests I choose including my own if we so choose. I understand the point of this feature is to allow for code review which is great but having some flexibility when needed would be awesome.

^ Same for my team as well. It's great to require another teammate's approval 95% of the time. But in similar instances to when the "merge immediately" option is useful, being able to approve my own MR and merge is important for time-sensitive fixes.

changed milestone to %Next 2-3 months

added Discussion label

changed milestone to %Next 3-6 months

mentioned in issue #3349