Support file comments via Github checks API

Zuul is already providing these file comments via the zuul_return value.
So far, the Github reporter wasn't able to use those, but with the help
of the checks API we can add so called "annotations" to each check run.

Change-Id: Iff10172f95dc0430bec8e4dafb9a6c09bbe06077

releasenotes/notes/github-checks-annotations-0b3e417b167e58d2.yaml

@@ -0,0 +1,10 @@
+---
+features:
+  - |
+    The Github driver can now report file comments via Github checks API.
+    If a github reporter is configured to use the checks API, file comments
+    provided via ``zuul_return`` will automatically be reported to the pull
+    request in Github.
+
+    For more details on how to provide file comments from Zuul, see the
+    documentation of the :ref:`return_values`.

tests/fakegithub.py

@@ -136,7 +136,8 @@ class FakeCommit(object):
     def set_check_run(self, name, details_url, output, status, conclusion,
                       completed_at, app):
         check_run = FakeCheckRun(
-            name, details_url, output, status, conclusion, completed_at, app)
+            name, details_url, output, status, conclusion, completed_at, app
+        )
         # Always insert a check_run to the front of the list to represent the
         # last check_run provided for a commit.
         self._check_runs.insert(0, check_run)

tests/fixtures/config/github-file-comments/git/github_common-config/zuul.yaml

@@ -0,0 +1,28 @@
+- pipeline:
+    name: check
+    manager: independent
+    post-review: true
+    trigger:
+      github:
+        - event: pull_request
+          action: opened
+    start:
+      github:
+        check: in_progress
+        comment: false
+    success:
+      github:
+        check: success
+        comment: false
+    failure:
+      github:
+        check: failure
+        comment: false
+
+- job:
+    name: base
+    parent: null
+    nodeset:
+      nodes:
+        - name: test_node
+          label: test_label

tests/fixtures/config/github-file-comments/git/org_project/README

@@ -0,0 +1 @@
+test

tests/fixtures/config/github-file-comments/git/org_project/playbooks/file-comments-error.yaml

@@ -0,0 +1,24 @@
+- hosts: all
+  tasks:
+    - zuul_return:
+        data:
+          zuul:
+            file_comments:
+              README:
+                - line: 2
+                  message: "Invalid range (only start)"
+                  range:
+                    start_line: 2
+                    start_character: 1
+                - line: 4
+                  message: "Invalid range (only end)"
+                  range:
+                    end_line: 4
+                    end_character: 7
+                - message: "No line provided"
+                - line: 7  # No message provided
+                - line: 9999
+                  message: Line is not part of the file
+              missingfile.txt:
+                - line: 1
+                  message: "Missing file"

tests/fixtures/config/github-file-comments/git/org_project/playbooks/file-comments.yaml

@@ -0,0 +1,23 @@
+- hosts: all
+  tasks:
+    - zuul_return:
+        data:
+          zuul:
+            file_comments:
+              README:
+                - line: 1
+                  message: "Simple line annotation"
+                - line: 6
+                  message: "simple range annotation"
+                  range:
+                    start_line: 4
+                    start_character: 0
+                    end_line: 6
+                    end_character: 10
+                - line: 7
+                  message: "Columns must be part of the same line"
+                  range:
+                    start_line: 7
+                    start_character: 13
+                    end_line: 7
+                    end_character: 26

tests/fixtures/config/github-file-comments/git/org_project/zuul.yaml

@@ -0,0 +1,16 @@
+- job:
+    parent: base
+    name: file-comments
+    run: playbooks/file-comments.yaml
+
+- job:
+    parent: base
+    name: file-comments-error
+    run: playbooks/file-comments-error.yaml
+
+- project:
+    name: org/project
+    check:
+      jobs:
+        - file-comments
+        - file-comments-error

tests/fixtures/config/github-file-comments/main.yaml

@@ -0,0 +1,8 @@
+- tenant:
+    name: tenant-one
+    source:
+      github:
+        config-projects:
+          - github/common-config
+        untrusted-projects:
+          - org/project

tests/unit/test_github_driver.py

@@ -18,6 +18,7 @@ from testtools.matchers import MatchesRegex, StartsWith
 import urllib
 import socket
 import time
+import textwrap
 from unittest import mock, skip
 
 import git
@@ -26,7 +27,8 @@ import github3.exceptions
 from zuul.driver.github.githubconnection import GithubShaCache
 import zuul.rpcclient
 
-from tests.base import (BaseTestCase, ZuulGithubAppTestCase, ZuulTestCase,
+from tests.base import (AnsibleZuulTestCase, BaseTestCase,
+                        ZuulGithubAppTestCase, ZuulTestCase,
                         simple_layout, random_sha1)
 from tests.base import ZuulWebFixture
 
@@ -1711,3 +1713,98 @@ class TestGithubAppDriver(ZuulGithubAppTestCase):
         self.assertEqual(2, len(A.comments))
         self.assertIn(expected_warning, A.comments[0])
         self.assertIn(expected_warning, A.comments[1])
+
+
+class TestCheckRunAnnotations(ZuulGithubAppTestCase, AnsibleZuulTestCase):
+    """We need Github app authentication and be able to run Ansible jobs"""
+    config_file = 'zuul-github-driver.conf'
+    tenant_config_file = "config/github-file-comments/main.yaml"
+
+    def test_file_comments(self):
+        project = "org/project"
+        github = self.fake_github.getGithubClient(None)
+
+        # The README file must be part of this PR to make the comment function
+        # work. Thus we change it's content to provide some more text.
+        files_dict = {
+            "README": textwrap.dedent(
+                """
+                section one
+                ===========
+
+                here is some text
+                and some more text
+                and a last line of text
+
+                section two
+                ===========
+
+                here is another section
+                with even more text
+                and the end of the section
+                """
+            ),
+        }
+
+        A = self.fake_github.openFakePullRequest(
+            project, "master", "A", files=files_dict
+        )
+        self.fake_github.emitEvent(A.getPullRequestOpenedEvent())
+        self.waitUntilSettled()
+
+        # We should have a pending check for the head sha
+        self.assertIn(
+            A.head_sha, github.repo_from_project(project)._commits.keys())
+        check_runs = self.fake_github.getCommitChecks(project, A.head_sha)
+
+        self.assertEqual(1, len(check_runs))
+        check_run = check_runs[0]
+
+        self.assertEqual("tenant-one/check", check_run["name"])
+        self.assertEqual("completed", check_run["status"])
+        self.assertThat(
+            check_run["output"]["summary"],
+            MatchesRegex(r'.*Build succeeded.*', re.DOTALL)
+        )
+
+        annotations = check_run["output"]["annotations"]
+        self.assertEqual(4, len(annotations))
+
+        self.assertEqual(annotations[0], {
+            "path": "README",
+            "annotation_level": "warning",
+            "message": "Simple line annotation",
+            "start_line": 1,
+            "end_line": 1,
+        })
+
+        # As the columns are not part of the same line, they are ignored in the
+        # annotation. Otherwise Github will complain about the request.
+        self.assertEqual(annotations[1], {
+            "path": "README",
+            "annotation_level": "warning",
+            "message": "simple range annotation",
+            "start_line": 4,
+            "end_line": 6,
+        })
+
+        self.assertEqual(annotations[2], {
+            "path": "README",
+            "annotation_level": "warning",
+            "message": "Columns must be part of the same line",
+            "start_line": 7,
+            "end_line": 7,
+            "start_column": 13,
+            "end_column": 26,
+        })
+
+        # From the invalid/error file comments, only the "line out of file"
+        # should remain. All others are excluded as they would result in
+        # invalid Github requests, making the whole check run update fail.
+        self.assertEqual(annotations[3], {
+            "path": "README",
+            "annotation_level": "warning",
+            "message": "Line is not part of the file",
+            "end_line": 9999,
+            "start_line": 9999
+        })

zuul/driver/gerrit/gerritreporter.py

@@ -35,18 +35,6 @@ class GerritReporter(BaseReporter):
         self._checks_api = action.pop('checks-api', None)
         self._labels = action
 
-    def _getFileComments(self, item):
-        ret = {}
-        for build in item.current_build_set.getBuilds():
-            fc = build.result_data.get('zuul', {}).get('file_comments')
-            if not fc:
-                continue
-            for fn, comments in fc.items():
-                existing_comments = ret.setdefault(fn, [])
-                existing_comments += comments
-        self.addConfigurationErrorComments(item, ret)
-        return ret
-
     def report(self, item):
         """Send a message to gerrit."""
         log = get_annotated_logger(self.log, item.event)
@@ -65,8 +53,7 @@ class GerritReporter(BaseReporter):
                 self.connection.canonical_hostname:
             return
 
-        comments = self._getFileComments(item)
-        self.filterComments(item, comments)
+        comments = self.getFileComments(item)
         if self._create_comment:
             message = self._formatItemReport(item)
         else:

zuul/driver/github/githubconnection.py

@@ -1835,7 +1835,7 @@ class GithubConnection(BaseConnection):
         log.debug("Removed label %s from %s#%s", label, proj, pr_number)
 
     def updateCheck(self, project, pr_number, sha, status, completed, context,
-                    details_url, message, zuul_event_id=None):
+                    details_url, message, file_comments, zuul_event_id=None):
         log = get_annotated_logger(self.log, zuul_event_id)
         github = self.getGithubClient(project, zuul_event_id=zuul_event_id)
         owner, proj = project.split("/")
@@ -1864,6 +1864,12 @@ class GithubConnection(BaseConnection):
 
         output = {"title": "Summary", "summary": message}
 
+        if file_comments:
+            # Build the list of annotations to be applied on the check run
+            output["annotations"] = self._buildAnnotationsFromComments(
+                file_comments
+            )
+
         # Currently, the GithubReporter only supports start and end reporting.
         # During the build no further update will be reported.
         if completed:
@@ -1972,6 +1978,75 @@ class GithubConnection(BaseConnection):
 
         return errors
 
+    def _buildAnnotationsFromComments(self, file_comments):
+        annotations = []
+        for fn, comments in file_comments.items():
+            for comment in comments:
+
+                if "message" not in comment:
+                    # Github doesn't accept anntoations without a message.
+                    # Faking a message doesn't make munch sense to me.
+                    continue
+
+                start_column = None
+                end_column = None
+                start_line = None
+                end_line = None
+
+                if "line" in comment:
+                    start_line = comment.get("line")
+                    end_line = comment.get("line")
+
+                if "range" in comment:
+                    rng = comment["range"]
+                    # Look up the start_ and end_line from the range and use
+                    # the line as fallback
+                    start_line = rng.get("start_line")
+                    end_line = rng.get("end_line")
+
+                    # Github only accepts column parameters if they apply to
+                    # the same line.
+                    if start_line == end_line:
+                        start_column = rng.get("start_character")
+                        end_column = rng.get("end_character")
+
+                # TODO (felix): Make annotation_level configurable via
+                # file_comments in zuul_return. Other reporters like Gerrit
+                # might ignore the field if they don't support it.
+                # Accepted values are "notice", "warning", "failure".
+                # A "failure" annotation won't declare the check run as
+                # failure.
+
+                # A Github check annotation requires at least the following
+                # attributes: "path", "start_line", "end_line", "message" and
+                # "annotation_level"
+                raw_annotation = {
+                    "path": fn,
+                    "annotation_level": "warning",
+                    "message": comment["message"],
+                    "start_line": start_line,
+                    "end_line": end_line,
+                    "start_column": start_column,
+                    "end_column": end_column,
+                }
+
+                # Filter out None values from the annotation. Otherwise
+                # github will complain about column values being None:
+                # "For 'properties/start_column', nil is not an integer."
+                # "For 'properties/end_column', nil is not an integer."
+                annotation = {
+                    k: v for k, v in raw_annotation.items()
+                    if v is not None
+                }
+
+                # Don't provide an annotation without proper start_ and
+                # end_line as this will make the whole check run update fail.
+                if not {"start_line", "end_line"} <= set(annotation):
+                    continue
+
+                annotations.append(annotation)
+        return annotations
+
     def getPushedFileNames(self, event):
         files = set()
         for c in event.commits:

zuul/driver/github/githubreporter.py

@@ -225,6 +225,9 @@ class GithubReporter(BaseReporter):
 
         details_url = item.formatStatusUrl()
 
+        # Check for inline comments that can be reported via checks API
+        file_comments = self.getFileComments(item)
+
         return self.connection.updateCheck(
             project,
             pr_number,
@@ -234,6 +237,7 @@ class GithubReporter(BaseReporter):
             self.context,
             details_url,
             message,
+            file_comments,
             zuul_event_id=item.event,
         )
 

zuul/reporter/__init__.py

@@ -82,6 +82,24 @@ class BaseReporter(object, metaclass=abc.ABCMeta):
                                               end_line=mark.end_line,
                                               end_character=mark.end_column)))
 
+    def _getFileComments(self, item):
+        """Get the file comments from the zuul_return value"""
+        ret = {}
+        for build in item.current_build_set.getBuilds():
+            fc = build.result_data.get("zuul", {}).get("file_comments")
+            if not fc:
+                continue
+            for fn, comments in fc.items():
+                existing_comments = ret.setdefault(fn, [])
+                existing_comments.extend(comments)
+        self.addConfigurationErrorComments(item, ret)
+        return ret
+
+    def getFileComments(self, item):
+        comments = self._getFileComments(item)
+        self.filterComments(item, comments)
+        return comments
+
     def filterComments(self, item, comments):
         """Filter comments for files in change