gitlab: merge request approval: specify the commit

Avoid approving a merge request which has been updated in the
meantime. It can happen when pipeline.dequeue-on-new-patchset
is disabled.

Change-Id: I6565a1af57b9267022fb3d11002d2d9ffab7a43b

tests/base.py

@@ -1693,7 +1693,11 @@ class FakeGitlabAPIClient(gitlabconnection.GitlabAPIClient):
         match = re.match(
             r'.+/projects/(.+)/merge_requests/(\d+)/approve$', url)
         if match:
+            assert 'sha' in params
             mr = self._get_mr(match)
+            if params['sha'] != mr.sha:
+                return {'message': 'SHA does not match HEAD of source '
+                        'branch: <new_sha>'}, 409, "", "POST"
             mr.approved = True
 
         match = re.match(

tests/unit/test_gitlab_driver.py

@@ -161,6 +161,27 @@ class TestGitlabDriver(ZuulTestCase):
         zuulvars = job.parameters['zuul']
         self.assertEqual('check-approval', zuulvars['pipeline'])
 
+    @simple_layout('layouts/basic-gitlab.yaml', driver='gitlab')
+    def test_merge_request_updated_during_build(self):
+
+        A = self.fake_gitlab.openFakeMergeRequest('org/project', 'master', 'A')
+        self.fake_gitlab.emitEvent(A.getMergeRequestOpenedEvent())
+        old = A.sha
+        A.addCommit()
+        new = A.sha
+        self.assertNotEqual(old, new)
+        self.waitUntilSettled()
+
+        self.assertEqual(2, len(self.history))
+        # MR must not be approved: tested commit isn't current commit
+        self.assertFalse(A.approved)
+
+        self.fake_gitlab.emitEvent(A.getMergeRequestUpdatedEvent())
+        self.waitUntilSettled()
+
+        self.assertEqual(4, len(self.history))
+        self.assertTrue(A.approved)
+
     @simple_layout('layouts/basic-gitlab.yaml', driver='gitlab')
     def test_merge_request_labeled(self):
 

zuul/driver/gitlab/gitlabconnection.py

@@ -321,12 +321,12 @@ class GitlabAPIClient():
         return resp[0]
 
     # https://docs.gitlab.com/ee/api/merge_request_approvals.html#approve-merge-request
-    def approve_mr(self, project_name, number, approve=True,
+    def approve_mr(self, project_name, number, patchset, approve=True,
                    zuul_event_id=None):
         approve = 'approve' if approve else 'unapprove'
         path = "/projects/%s/merge_requests/%s/%s" % (
             quote_plus(project_name), number, approve)
-        params = {}
+        params = {'sha': patchset} if approve else {}
         resp = self.post(
             self.baseurl + path, params=params,
             zuul_event_id=zuul_event_id)
@@ -336,8 +336,14 @@ class GitlabAPIClient():
             # approve and unapprove endpoint could return code 401 whether the
             # actual state of the Merge Request approval. Two call on approve
             # endpoint the second call return 401.
-            if resp[1] != 401:
+            # 409 is returned when current HEAD of the merge request doesn't
+            # match the 'sha' parameter.
+            if resp[1] not in (401, 409):
                 raise
+            elif approve == 'approve' and resp[1] == 409:
+                log = get_annotated_logger(self.log, zuul_event_id)
+                log.error('Fail to approve the merge request: %s' % resp[0])
+                return
         return resp[0]
 
     # https://docs.gitlab.com/ee/api/merge_request_approvals.html#get-configuration-1
@@ -563,12 +569,14 @@ class GitlabConnection(BaseConnection):
             project_name, number, message, zuul_event_id=event)
         log.info("Commented on MR %s#%s", project_name, number)
 
-    def approveMR(self, project_name, number, approve, event=None):
+    def approveMR(self, project_name, number, patchset, approve, event=None):
         log = get_annotated_logger(self.log, event)
-        self.gl_client.approve_mr(
-            project_name, number, approve, zuul_event_id=event)
-        log.info(
-            "Set approval: %s on MR %s#%s", approve, project_name, number)
+        result = self.gl_client.approve_mr(
+            project_name, number, patchset, approve, zuul_event_id=event)
+        if result:
+            log.info(
+                "Set approval: %s on MR %s#%s (%s)", approve,
+                project_name, number, patchset)
 
     def getChangesDependingOn(self, change, projects, tenant):
         """ Reverse lookup of MR depending on this one

zuul/driver/gitlab/gitlabreporter.py

@@ -68,10 +68,11 @@ class GitlabReporter(BaseReporter):
         log = get_annotated_logger(self.log, item.event)
         project = item.change.project.name
         mr_number = item.change.number
+        patchset = item.change.patchset
         log.debug('Reporting change %s, params %s, approval: %s',
                   item.change, self.config, self._approval)
-        self.connection.approveMR(project, mr_number, self._approval,
-                                  event=item.event)
+        self.connection.approveMR(project, mr_number, patchset,
+                                  self._approval, event=item.event)
 
     def mergeMR(self, item):
         project = item.change.project.name