Adding approvals via our API could raise a race condition when an approval is sent via the API and a commit is pushed at the same time

Existing ZD Ticket: https://gitlab.zendesk.com/agent/tickets/76235

Excerpt from the ticket:

I had a look in the GitLab API to find out what options we have to let a tool automatically approve a merge request. The option there is just

to approve a merge request which brought up the question what happens when there is a race condition between approval and another change to the merge request. Usually when a merge request was approved the approval gets removed as soon as a code change was made.

However what happens when the merge request was modified between Review and giving the approval? I expected that at least in the GitLab API I need to pass the commit id when approving to avoid that. I know that one could require that the CI must succeed building but that isn’t an option here.

@jivanvl So the proposal is to provide the SHA of the "last approved" commit in relation to the Merge Request, as a parameter in the request to the /approve endpoint? And the Merge Request can only be marked as approved if that commit, and the branch associated with the Merge Request, are equivalent?

/cc @smcgivern

@markglenfletcher Basically yeah, from what I understand, this is to avoid a race condition a la how our MR widget does it, but this is for the API

@markglenfletcher Yes, exactly. If the commit is no longer the HEAD of the branch, it should not be given. If you make the commit-id optional it won't break the existing API and it's still possible to approve regardless of the commit in case one doesn't care.

@heiko_boettger:

I know that one could require that the CI must succeed building but that isn’t an option here.

I think this is only for merging, not approving, isn't it?

On the topic of merging, this is actually already a param when merging an MR: https://docs.gitlab.com/ee/api/merge_requests.html#accept-mr

If the sha parameter is passed and does not match the HEAD of the source - you'll get a 409 and the error message 'SHA does not match HEAD of source branch'

I think adding that for approvals would be reasonable. Is that what you're looking for?

@smcgivern Yes, the sha param is missing here. It's exactly what am I looking for.

@victorwu this is fairly straightforward. It's like https://gitlab.com/gitlab-org/gitlab-ce/issues/14139 but for approvals

changed milestone to %9.4

added Deliverable label

assigned to @smcgivern

mentioned in merge request !2370 (merged)

closed via merge request !2370 (merged)

mentioned in commit ecb9df99