Explicit eligible individual approvers and eligible explicit group approvers.
Implicit eligible project member approvers.
At the merge request level
The number of required approvals.
Explicit eligible individual approvers and eligible explicit group approvers.
Implicit eligible project member approvers.
Who has already approved.
Validations
Should be same as Web UI.
When attempting to override at merge request level, should respect flag that allows for overridability. (This flag itself is part of scope. See below.)
Should respect mr required number needs to be >= project required number.
Should respect who are eligible to be added per existing logic.
Appropriate error messages if failed validations.
Individual approvers
The individual approvers are explicitly set/returned in the Web UI. This should be the same in the API.
Group approvers
A group approver is represented by the group in the system and reflected in the Web UI. So that should be the same in the API. E.g. Group A is a group approver with members X, Y, Z. Via the API, you should not return X, Y, Z as individual approvers. The API should only return Group A as an approver.
Project member approvers
Under certain conditions (see description in https://gitlab.com/gitlab-org/gitlab-ee/issues/4134), project members with developer role or higher is eligible to approve. This should not be exposed via the API, because the API user can infer this on their own.
Also allow the API to return and set the flag (in a project) to override approvals per merge request, i.e. the checkbox in the project settings:
@smcgivern : Overriding project approvals config is a project setting. Do we typically expose these project settings via the API? If yes, is it worth it to do it as part of this issue? Or another issue?
@smcgivern : Also added a few more detailed specs for the API in terms of group approvers and implicit approvers. Those specs are based on my understanding of the GitLab approver system so I just tried to match as closely. Definitely let's update if it doesn't make sense. Feel free to comment or update directly. Thanks.
@DouweM : Do you mind reviewing the description above. @smcgivern is away for this week. I hope to be able to get back to the customer next week in terms of scheduling. But if you could at least verify that the general design direction makes sense that would be a huge help to reduce any risk. Thanks!
@vsizov If I recall correctly, you initially implemented approvers and may have worked on some of the later iterations too. Does the design direction sound right to you?
"Who can approve and who has already approved." => "Who can approve" for project level. I think this is a TYPO
And the part "Project member approvers" is not clear to me, I mean I don't understand why we should make a difference between UI and API. The validation should be consistent for both.
Thanks @DouweM + @vsizov for validating this. Super helpful!!
@vsizov: You're right. At the project level, who has approved doesn't make sense. Updated it and made the wording even more explicit. Thanks!
Regarding the project member approvers section, I mean that in our existing functionality, these are implicit eligible approvers right? So say you are looking at merge request X in project P, and the eligible approvers are:
Explicit approvers
User A
User B
Implicit approvers
All the users in Project P with developer role or higher, minus the merge request author.
So in the API call, if it's asking for a list of eligible approvers, I'm saying we should return:
User A
User B
only. And then we don't have to return anything else. Because the consumer of the API should know the business logic of implicit approvers already.
@mdelaossa could you work on this in 10.6, please? Note that I closed https://gitlab.com/gitlab-org/gitlab-ee/issues/4463 in favour of this, as I think this is a superset of that issue. Please check that issue when you start work on this, just to be sure there's nothing missing, though.
Thanks @smcgivern and @mdelaossa . I also added the screenshot in #4463 (closed) from there to here. Also the description also contains a comment that the API should respect the overridability. I.e. if the flag is not set for a given project, when you try to change the required number or the approvers on a particular merge request, it should return an error. Let me know if you disagree.
@smcgivern@mdelaossa : Just a heads up, the customer (https://gitlab.com/gitlab-org/gitlab-ee/issues/183#note_58093592) who has this high on their priority list already has some scripts mocked out on their end and are ready to test this out as soon as there's any WIP branch/mr. So they might have some comments on naming of variables, and the technical implementation along the way. So look out for any comments from them and feel free to engage directly. Ping myself and @jwoods06 if you need anything. Thanks!
Hello Mario. I'm the customer that Victor is referring to. I spent some time working with the existing merge_request API and am fairly familiar with how it works. We have python scripts that will be using the new enhancements to the API.
Here is an example python script that shows how the 2 new parameters to the PUT merge_request API might be implemented. This is just a suggestion, so if you feel that this is not correct, then that is fine. Its meant to show you how we think it might be implemented..,.
Example python code ...
#!/usr/local/bin/python3
import requests
headers = {'PRIVATE-TOKEN':'TCim54sdsfsdfsdfk86'} # <-example private token created in GitLab
specify merge request URL for project 776, merge request 323
Hmm. The code above got muddied up. Let me try again...
#!/usr/local/bin/python3import requestsheaders = {'PRIVATE-TOKEN':'TCim54sdf342XDRzak86'} # <-example private token created in GitLab# specify merge request URL for project 776, merge request 323update_approvers_url = 'https://junipergit.juniper.net/api/v4/projects/776/merge_requests/323)'# Option 3: Use username list and update required approversrequired_approvers = ['jdoe','jsmith','padams']update_response = requests.put(update_approvers_url,headers=headers,data={'approvers':required_approvers})# Option 2: Alternatively, if it is too difficult to implement the username list to approvers as above,# then you could implement this and require user ids. We could always fetch the user ids via the users api first.required_approvers = [32,54,23]update_response = requests.put(update_approvers_url,headers=headers,data={'approvers':required_approvers})# And finally, we'd need to be able to update the minimum number of approvals neededupdate_response = requests.put(update_approvers_url,headers=headers,data={'approvals_required': 6}
I think your proposed design makes sense, although we might tweak the param names if needed.
On the user ID / username thing, currently we tend to use user IDs for params (like when reassigning an issue), so I think we'd want to be consistent there, but I don't see a problem in allowing any param that accepts a user to accept either:
A numeric user ID.
A username, including the leading '@'. (This is important because you can have a numeric username.)
That would probably be a separate issue to this one, though, to do it across the API consistently. I proposed it in https://gitlab.com/gitlab-org/gitlab-ce/issues/30328#note_42913157, but in that case, there was a better alternative: another option would be that we would accept either approver_ids or approver_username, and just use one of those. Again, I think that would be part of a future improvement to the API.
@jdhayes555@victorwu while thinking about how to organize this I realized that the existing approvals endpoint expands the groups allowed to approve into a list of users (User.where(id: all_approvers_including_groups.map(&:id)).where.not(id: approvals.select(:user_id)))
I'm wary of changing this because it might break functionality for someone using this endpoint. What do you think? We could do a few things:
change the functionality and keep the old behavior behind a param (?expand_groups=1?)
keep this behavior on the MR side and be inconsistent
return only users on the project endpoints for consistency
@mdelaossa : The approvals API is a new external API that is in scope here right? When you say existing approvals endpoint, what do you mean? Something internal?
As for expanding a group into it's constituent users, I think that's used in some internal logic to figure out if the current user is an eligible approver, and may be used in something other places. Not sure how that applies in this situation?
I 100% agree that the API should behave analogously to the WebUI. I'm just wondering what to do with these pre-existing endpoints (basically just asking for the go-ahead to change them so they're compliant with the expected behavior :) )
@mdelaossa thanks, I agree with @victorwu. approvers_left appears to be completely undocumented, so let's change the format of this to be consistent everywhere. We can add an option to expand those approvers if needed.
Actually, I think I was too hasty there. I think approvers_left can differ from the rest of the API if needed, because only a user can actually approve an MR - using groups is just a mechanism for selecting a set of users.
So if it's easier to use a different key name for the configuration checking, which returns both users and groups (as the MR is configured), and keep this like it is, I think that's fine too 👍
ah, that's a good way to look at this. Thanks @smcgivern!
I'll leave that as-is and add another key for configured approvers (or use another endpoint for the actual configuration. Still gotta see what will keep it more tidy)
I'm wondering if this will be backported to earlier versions? I'm implementing something in another project that requires knowledge of whether "reset_approvals_on_push" is set. If this is only available in the API in later versions of GitLab, it'll complicate things somewhat.