Merge "Save superfluous api requests in check run reporting"

tests/fakegithub.py

@@ -650,6 +650,9 @@ class FakeGithubSession(object):
         # Handle check run creation
         match = re.match(r'.*/repos/(.*)/check-runs$', url)
         if match:
+            if self.client._data.fail_check_run_creation:
+                return FakeResponse('Internal server error', 500)
+
             org, reponame = match.groups()[0].split('/', 1)
             repo = self.client._data.repos.get((org, reponame))
 
@@ -709,6 +712,32 @@ class FakeGithubSession(object):
 
         return FakeResponse(None, 404)
 
+    def patch(self, url, data=None, headers=None, params=None, json=None):
+
+        # Handle check run update
+        match = re.match(r'.*/repos/(.*)/check-runs/(.*)$', url)
+        if match:
+            org, reponame = match.groups()[0].split('/', 1)
+            check_run_id = match.groups()[1]
+            repo = self.client._data.repos.get((org, reponame))
+
+            # Find the specified check run
+            check_runs = [
+                check_run
+                for commit in repo._commits.values()
+                for check_run in commit._check_runs
+                if check_run.id == check_run_id
+            ]
+            check_run = check_runs[0]
+
+            check_run.update(json['conclusion'],
+                             json['completed_at'],
+                             json['output'],
+                             json['details_url'],
+                             json['external_id'],
+                             json['actions'])
+            return FakeResponse(check_run.as_dict(), 200)
+
     def get_repo(self, request, params=None):
         org, project, request = request.split('/', 2)
         project_name = '{}/{}'.format(org, project)
@@ -736,6 +765,7 @@ class FakeGithubData(object):
         self.pull_requests = pull_requests
         self.repos = {}
         self.reports = []
+        self.fail_check_run_creation = False
 
 
 class FakeGithubClient(object):

tests/unit/test_github_driver.py

@@ -1912,19 +1912,22 @@ class TestGithubAppDriver(ZuulGithubAppTestCase):
         project = "org/project3"
         github = self.fake_github.getGithubClient(None)
 
+        # Make check run creation fail
+        github._data.fail_check_run_creation = True
+
         # pipeline reports pull request status both on start and success
         self.executor_server.hold_jobs_in_build = True
         A = self.fake_github.openFakePullRequest(project, "master", "A")
         self.fake_github.emitEvent(A.getPullRequestOpenedEvent())
         self.waitUntilSettled()
 
-        # We should have a pending check for the head sha
+        # We should have no pending check for the head sha
         commit = github.repo_from_project(project)._commits.get(A.head_sha)
         check_runs = commit.check_runs()
-        self.assertEqual(1, len(check_runs))
+        self.assertEqual(0, len(check_runs))
 
-        # Delete this check_run to simulate a failed check_run creation
-        commit._check_runs = []
+        # Make check run creation work again
+        github._data.fail_check_run_creation = False
 
         # Now run the build and check if the update of the check_run could
         # still be accomplished.
@@ -1974,7 +1977,7 @@ class TestGithubAppDriver(ZuulGithubAppTestCase):
         self.assertEqual(0, len(check_runs))
 
         expected_warning = (
-            "Failed to update check run tenant-one/checks-api-reporting: "
+            "Failed to create check run tenant-one/checks-api-reporting: "
             "403 Resource not accessible by integration"
         )
         self.assertEqual(2, len(A.comments))

zuul/driver/github/githubconnection.py

@@ -1947,13 +1947,27 @@ class GithubConnection(CachedBranchConnection):
         pull_request.remove_label(label)
         log.debug("Removed label %s from %s#%s", label, proj, pr_number)
 
+    def _create_or_update_check(self, github, project_name, check_run_id,
+                                **kwargs):
+        if check_run_id:
+            # Update an existing check run
+            url = github.session.build_url(
+                'repos', project_name, 'check-runs', str(check_run_id))
+            resp = github.session.patch(url, json=kwargs)
+        else:
+            # Create a new check run
+            url = github.session.build_url(
+                'repos', project_name, 'check-runs')
+            resp = github.session.post(url, json=kwargs)
+        resp.raise_for_status()
+        return resp.json().get('id')
+
     def updateCheck(self, project, pr_number, sha, status, completed, context,
                     details_url, message, file_comments, external_id,
-                    zuul_event_id=None):
+                    zuul_event_id=None, check_run_id=None):
         log = get_annotated_logger(self.log, zuul_event_id)
         github = self.getGithubClient(project, zuul_event_id=zuul_event_id)
         self._append_accept_header(github, PREVIEW_CHECKS_ACCEPT)
-        owner, proj = project.split("/")
 
         # Always provide an empty list of actions by default
         actions = []
@@ -1977,7 +1991,7 @@ class GithubConnection(CachedBranchConnection):
                     context
                 )
             )
-            return errors
+            return None, errors
 
         output = {"title": "Summary", "summary": message}
 
@@ -2000,83 +2014,34 @@ class GithubConnection(CachedBranchConnection):
             # conclusion, as the status will always be "completed".
             conclusion = status
 
-            # Unless something went wrong during the start reporting of this
-            # change (e.g. the check_run creation failed), there should already
-            # be a check_run available. If not we will create one.
-            check_runs = []
-
-            repository = github.repository(owner, proj)
-            try:
-                check_runs = [
-                    c for c in repository.commit(sha).check_runs()
-                    if c.name == context
-                ]
-            except github3.exceptions.GitHubException as exc:
-                log.error(
-                    "Could not retrieve existing check runs for %s#%s on "
-                    "sha %s: %s",
-                    project, pr_number, sha, str(exc),
-                )
-
-            if not check_runs:
-                log.debug(
-                    "Could not find check run %s for %s#%s on sha %s. "
-                    "Creating a new one",
-                    context, project, pr_number, sha,
+            if not check_run_id:
+                log.debug("Could not find check run %s for %s#%s on sha %s. "
+                          "Creating a new one", context, project, pr_number,
+                          sha)
+                action = 'create'
+                arguments = dict(
+                    name=context,
+                    head_sha=sha,
+                    conclusion=conclusion,
+                    completed_at=completed_at,
+                    output=output,
+                    details_url=details_url,
+                    external_id=external_id,
+                    actions=actions,
                 )
-
-                try:
-                    check_run = repository.create_check_run(
-                        name=context,
-                        head_sha=sha,
-                        conclusion=conclusion,
-                        completed_at=completed_at,
-                        output=output,
-                        details_url=details_url,
-                        external_id=external_id,
-                        actions=actions,
-                    )
-                except github3.exceptions.GitHubException as exc:
-                    # TODO (felix): Should we retry the check_run creation?
-                    log.error(
-                        "Failed to create check run %s for %s#%s on sha %s: "
-                        "%s",
-                        context, project, pr_number, sha, str(exc)
-                    )
-                    errors.append(
-                        "Failed to create check run {}: {}".format(
-                            context, str(exc)
-                        )
-                    )
             else:
-                check_run = check_runs[0]
-                log.debug(
-                    "Updating existing check run %s for %s#%s on sha %s "
-                    "with status %s",
-                    context, project, pr_number, sha, status,
+                log.debug("Updating existing check run %s for %s#%s on sha %s "
+                          "with status %s", context, project, pr_number, sha,
+                          status)
+                action = 'update'
+                arguments = dict(
+                    conclusion=conclusion,
+                    completed_at=completed_at,
+                    output=output,
+                    details_url=details_url,
+                    external_id=external_id,
+                    actions=actions,
                 )
-
-                try:
-                    check_run.update(
-                        conclusion=conclusion,
-                        completed_at=completed_at,
-                        output=output,
-                        details_url=details_url,
-                        external_id=external_id,
-                        actions=actions,
-                    )
-                except github3.exceptions.GitHubException as exc:
-                    log.error(
-                        "Failed to update check run %s for %s#%s on sha %s: "
-                        "%s",
-                        context, project, pr_number, sha, str(exc),
-                    )
-                    errors.append(
-                        "Failed to update check run {}: {}".format(
-                            context, str(exc)
-                        )
-                    )
-
         else:
             # Add an abort/dequeue action to running check runs
             actions.append(
@@ -2091,35 +2056,33 @@ class GithubConnection(CachedBranchConnection):
                 }
             )
 
-            # Report the start of a check run
-            try:
-                data = dict(
-                    name=context,
-                    head_sha=sha,
-                    status=status,
-                    output=output,
-                    details_url=details_url,
-                    external_id=external_id,
-                    actions=actions,
-                )
+            action = 'create'
+            arguments = dict(
+                name=context,
+                head_sha=sha,
+                status=status,
+                output=output,
+                details_url=details_url,
+                external_id=external_id,
+                actions=actions,
+            )
 
-                url = github.session.build_url('repos', project,
-                                               'check-runs')
-                resp = github.session.post(url, json=data)
-                resp.raise_for_status()
+        # Create/update the check run
+        try:
+            check_run_id = self._create_or_update_check(
+                github,
+                project,
+                check_run_id,
+                **arguments,
+            )
 
-            except requests.exceptions.HTTPError as exc:
-                log.error(
-                    "Failed to create check run %s for %s#%s on sha %s: %s",
-                    context, project, pr_number, sha, str(exc),
-                )
-                errors.append(
-                    "Failed to update check run {}: {}".format(
-                        context, str(exc)
-                    )
-                )
+        except requests.exceptions.HTTPError as exc:
+            log.error("Failed to %s check run %s for %s#%s on sha %s: %s",
+                      action, context, project, pr_number, sha, str(exc))
+            errors.append("Failed to {} check run {}: {}".format(
+                action, context, str(exc)))
 
-        return errors
+        return check_run_id, errors
 
     def _buildAnnotationsFromComments(self, file_comments):
         annotations = []

zuul/driver/github/githubreporter.py

@@ -245,7 +245,8 @@ class GithubReporter(BaseReporter):
             }
         )
 
-        return self.connection.updateCheck(
+        state = item.dynamic_state[self.connection.connection_name]
+        check_run_id, errors = self.connection.updateCheck(
             project,
             pr_number,
             sha,
@@ -257,8 +258,14 @@ class GithubReporter(BaseReporter):
             file_comments,
             external_id,
             zuul_event_id=item.event,
+            check_run_id=state.get('check_run_id')
         )
 
+        if check_run_id:
+            state['check_run_id'] = check_run_id
+
+        return errors
+
     def setLabels(self, item):
         log = get_annotated_logger(self.log, item.event)
         project = item.change.project.name

zuul/model.py

@@ -14,7 +14,7 @@
 # under the License.
 
 import abc
-from collections import OrderedDict
+from collections import OrderedDict, defaultdict
 import copy
 import json
 import logging
@@ -2218,6 +2218,10 @@ class QueueItem(object):
         self._cached_sql_results = {}
         self.event = event  # The trigger event that lead to this queue item
 
+        # Additional container for connection specifig information to be used
+        # by reporters throughout the lifecycle
+        self.dynamic_state = defaultdict(dict)
+
     def annotateLogger(self, logger):
         """Return an annotated logger with the trigger event"""
         return get_annotated_logger(logger, self.event)