Merge "Handle missing diff_refs attribute"

4 files changed, 130 insertions, 14 deletions

diff --git a/tests/base.py b/tests/base.py
index dcd316fab..fd927a92c 100644
--- a/tests/base.py
+++ b/tests/base.py
@@ -2118,6 +2118,21 @@ class FakeGitlabConnection(gitlabconnection.GitlabConnection):
         yield
         self._test_web_server.options['community_edition'] = False
 
+    @contextmanager
+    def enable_delayed_complete_mr(self, complete_at):
+        self._test_web_server.options['delayed_complete_mr'] = complete_at
+        yield
+        self._test_web_server.options['delayed_complete_mr'] = 0
+
+    @contextmanager
+    def enable_uncomplete_mr(self):
+        self._test_web_server.options['uncomplete_mr'] = True
+        orig = self.gl_client.get_mr_wait_factor
+        self.gl_client.get_mr_wait_factor = 0.1
+        yield
+        self.gl_client.get_mr_wait_factor = orig
+        self._test_web_server.options['uncomplete_mr'] = False
+
 
 class GitlabChangeReference(git.Reference):
     _common_path_default = "refs/merge-requests"
diff --git a/tests/fakegitlab.py b/tests/fakegitlab.py
index 294887af0..e4a3e1ac8 100644
--- a/tests/fakegitlab.py
+++ b/tests/fakegitlab.py
@@ -21,6 +21,7 @@ import re
 import socketserver
 import threading
 import urllib.parse
+import time
 
 from git.util import IterableList
 
@@ -32,12 +33,17 @@ class GitlabWebServer(object):
         self.merge_requests = merge_requests
         self.fake_repos = defaultdict(lambda: IterableList('name'))
         # A dictionary so we can mutate it
-        self.options = dict(community_edition=False)
+        self.options = dict(
+            community_edition=False,
+            delayed_complete_mr=0,
+            uncomplete_mr=False)
+        self.stats = {"get_mr": 0}
 
     def start(self):
         merge_requests = self.merge_requests
         fake_repos = self.fake_repos
         options = self.options
+        stats = self.stats
 
         class Server(http.server.SimpleHTTPRequestHandler):
             log = logging.getLogger("zuul.test.GitlabWebServer")
@@ -146,6 +152,7 @@ class GitlabWebServer(object):
                 self.wfile.write(data)
 
             def get_mr(self, project, mr):
+                stats["get_mr"] += 1
                 mr = self._get_mr(project, mr)
                 data = {
                     'target_branch': mr.branch,
@@ -162,13 +169,20 @@ class GitlabWebServer(object):
                     'labels': mr.labels,
                     'merged_at': mr.merged_at.strftime('%Y-%m-%dT%H:%M:%S.%fZ')
                     if mr.merged_at else mr.merged_at,
-                    'diff_refs': {
+                    'merge_status': mr.merge_status,
+                }
+                if options['delayed_complete_mr'] and \
+                        time.monotonic() < options['delayed_complete_mr']:
+                    diff_refs = None
+                elif options['uncomplete_mr']:
+                    diff_refs = None
+                else:
+                    diff_refs = {
                         'base_sha': mr.base_sha,
                         'head_sha': mr.sha,
                         'start_sha': 'c380d3acebd181f13629a25d2e2acca46ffe1e00'
-                    },
-                    'merge_status': mr.merge_status,
-                }
+                    }
+                data['diff_refs'] = diff_refs
                 self.send_data(data)
 
             def get_mr_approvals(self, project, mr):
diff --git a/tests/unit/test_gitlab_driver.py b/tests/unit/test_gitlab_driver.py
index 6c4d4eeb9..e04a3b5d6 100644
--- a/tests/unit/test_gitlab_driver.py
+++ b/tests/unit/test_gitlab_driver.py
@@ -126,6 +126,27 @@ class TestGitlabDriver(ZuulTestCase):
         self.assertTrue(A.approved)
 
     @simple_layout('layouts/basic-gitlab.yaml', driver='gitlab')
+    def test_merge_request_opened_imcomplete(self):
+
+        now = time.monotonic()
+        complete_at = now + 3
+        with self.fake_gitlab.enable_delayed_complete_mr(complete_at):
+            description = "This is the\nMR description."
+            A = self.fake_gitlab.openFakeMergeRequest(
+                'org/project', 'master', 'A', description=description)
+            self.fake_gitlab.emitEvent(
+                A.getMergeRequestOpenedEvent(), project='org/project')
+            self.waitUntilSettled()
+
+            self.assertEqual('SUCCESS',
+                             self.getJobFromHistory('project-test1').result)
+
+            self.assertEqual('SUCCESS',
+                             self.getJobFromHistory('project-test2').result)
+
+        self.assertTrue(self.fake_gitlab._test_web_server.stats["get_mr"] > 2)
+
+    @simple_layout('layouts/basic-gitlab.yaml', driver='gitlab')
     def test_merge_request_updated(self):
 
         A = self.fake_gitlab.openFakeMergeRequest('org/project', 'master', 'A')
@@ -407,7 +428,7 @@ class TestGitlabDriver(ZuulTestCase):
 
     @simple_layout('layouts/basic-gitlab.yaml', driver='gitlab')
     def test_pull_request_with_dyn_reconf(self):
-
+        path = os.path.join(self.upstream_root, 'org/project')
         zuul_yaml = [
             {'job': {
                 'name': 'project-test3',
@@ -424,11 +445,13 @@ class TestGitlabDriver(ZuulTestCase):
         playbook = "- hosts: all\n  tasks: []"
 
         A = self.fake_gitlab.openFakeMergeRequest(
-            'org/project', 'master', 'A')
+            'org/project', 'master', 'A',
+            base_sha=git.Repo(path).head.object.hexsha)
         A.addCommit(
             {'.zuul.yaml': yaml.dump(zuul_yaml),
              'job.yaml': playbook}
         )
+        A.addCommit({"dummy.file": ""})
         self.fake_gitlab.emitEvent(A.getMergeRequestOpenedEvent())
         self.waitUntilSettled()
 
@@ -440,6 +463,40 @@ class TestGitlabDriver(ZuulTestCase):
                          self.getJobFromHistory('project-test3').result)
 
     @simple_layout('layouts/basic-gitlab.yaml', driver='gitlab')
+    def test_pull_request_with_dyn_reconf_alt(self):
+        with self.fake_gitlab.enable_uncomplete_mr():
+            zuul_yaml = [
+                {'job': {
+                    'name': 'project-test3',
+                    'run': 'job.yaml'
+                }},
+                {'project': {
+                    'check': {
+                        'jobs': [
+                            'project-test3'
+                        ]
+                    }
+                }}
+            ]
+            playbook = "- hosts: all\n  tasks: []"
+            A = self.fake_gitlab.openFakeMergeRequest(
+                'org/project', 'master', 'A')
+            A.addCommit(
+                {'.zuul.yaml': yaml.dump(zuul_yaml),
+                 'job.yaml': playbook}
+            )
+            A.addCommit({"dummy.file": ""})
+            self.fake_gitlab.emitEvent(A.getMergeRequestOpenedEvent())
+            self.waitUntilSettled()
+
+            self.assertEqual('SUCCESS',
+                             self.getJobFromHistory('project-test1').result)
+            self.assertEqual('SUCCESS',
+                             self.getJobFromHistory('project-test2').result)
+            self.assertEqual('SUCCESS',
+                             self.getJobFromHistory('project-test3').result)
+
+    @simple_layout('layouts/basic-gitlab.yaml', driver='gitlab')
     def test_ref_updated_and_tenant_reconfigure(self):
 
         self.waitUntilSettled()
diff --git a/zuul/driver/gitlab/gitlabconnection.py b/zuul/driver/gitlab/gitlabconnection.py
index 1515db8df..da423f085 100644
--- a/zuul/driver/gitlab/gitlabconnection.py
+++ b/zuul/driver/gitlab/gitlabconnection.py
@@ -281,6 +281,7 @@ class GitlabAPIClient():
         self.api_token = api_token
         self.keepalive = keepalive
         self.disable_pool = disable_pool
+        self.get_mr_wait_factor = 2
         self.headers = {'Authorization': 'Bearer %s' % (
             self.api_token)}
 
@@ -342,11 +343,36 @@ class GitlabAPIClient():
 
     # https://docs.gitlab.com/ee/api/merge_requests.html#get-single-mr
     def get_mr(self, project_name, number, zuul_event_id=None):
-        path = "/projects/%s/merge_requests/%s" % (
-            quote_plus(project_name), number)
-        resp = self.get(self.baseurl + path, zuul_event_id=zuul_event_id)
-        self._manage_error(*resp, zuul_event_id=zuul_event_id)
-        return resp[0]
+        log = get_annotated_logger(self.log, zuul_event_id)
+        attempts = 0
+
+        def _get_mr():
+            path = "/projects/%s/merge_requests/%s" % (
+                quote_plus(project_name), number)
+            resp = self.get(self.baseurl + path, zuul_event_id=zuul_event_id)
+            self._manage_error(*resp, zuul_event_id=zuul_event_id)
+            return resp[0]
+
+        # The Gitlab API might not return a complete MR description as
+        # some attributes are updated asynchronously. This loop ensures
+        # we query the API until all async attributes are available or until
+        # a defined delay is reached.
+        while True:
+            attempts += 1
+            mr = _get_mr()
+            # The diff_refs attribute is updated asynchronously
+            if all(map(lambda k: mr.get(k, None), ['diff_refs'])):
+                return mr
+            if attempts > 4:
+                log.warning(
+                    "Fetched MR %s#%s with imcomplete data" % (
+                        project_name, number))
+                return mr
+            wait_delay = attempts * self.get_mr_wait_factor
+            log.info(
+                "Will retry to fetch %s#%s due to imcomplete data "
+                "(in %s seconds) ..." % (project_name, number, wait_delay))
+            time.sleep(wait_delay)
 
     # https://docs.gitlab.com/ee/api/branches.html#list-repository-branches
     def get_project_branches(self, project_name, exclude_unprotected,
@@ -667,8 +693,12 @@ class GitlabConnection(ZKChangeCacheMixin, ZKBranchCacheMixin, BaseConnection):
         change.ref = "refs/merge-requests/%s/head" % change.number
         change.branch = change.mr['target_branch']
         change.is_current_patchset = (change.mr['sha'] == change.patchset)
-        change.base_sha = change.mr['diff_refs'].get('base_sha')
-        change.commit_id = change.mr['diff_refs'].get('head_sha')
+        change.commit_id = event.patch_number
+        diff_refs = change.mr.get("diff_refs", {})
+        if diff_refs:
+            change.base_sha = diff_refs.get('base_sha')
+        else:
+            change.base_sha = None
         change.owner = change.mr['author'].get('username')
         # Files changes are not part of the Merge Request data
         # See api/merge_requests.html#get-single-mr-changes