From fe3b5e3bae70b0591a19cc8992484790411d2a2b Mon Sep 17 00:00:00 2001 From: Felix Edel Date: Fri, 14 Feb 2020 14:05:44 +0100 Subject: [PATCH] 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 --- ...b-checks-annotations-0b3e417b167e58d2.yaml | 10 ++ tests/fakegithub.py | 3 +- .../git/github_common-config/zuul.yaml | 28 ++++++ .../git/org_project/README | 1 + .../playbooks/file-comments-error.yaml | 24 +++++ .../org_project/playbooks/file-comments.yaml | 23 +++++ .../git/org_project/zuul.yaml | 16 +++ .../config/github-file-comments/main.yaml | 8 ++ tests/unit/test_github_driver.py | 99 ++++++++++++++++++- zuul/driver/gerrit/gerritreporter.py | 15 +-- zuul/driver/github/githubconnection.py | 77 ++++++++++++++- zuul/driver/github/githubreporter.py | 4 + zuul/reporter/__init__.py | 18 ++++ 13 files changed, 309 insertions(+), 17 deletions(-) create mode 100644 releasenotes/notes/github-checks-annotations-0b3e417b167e58d2.yaml create mode 100644 tests/fixtures/config/github-file-comments/git/github_common-config/zuul.yaml create mode 100644 tests/fixtures/config/github-file-comments/git/org_project/README create mode 100644 tests/fixtures/config/github-file-comments/git/org_project/playbooks/file-comments-error.yaml create mode 100644 tests/fixtures/config/github-file-comments/git/org_project/playbooks/file-comments.yaml create mode 100644 tests/fixtures/config/github-file-comments/git/org_project/zuul.yaml create mode 100644 tests/fixtures/config/github-file-comments/main.yaml diff --git a/releasenotes/notes/github-checks-annotations-0b3e417b167e58d2.yaml b/releasenotes/notes/github-checks-annotations-0b3e417b167e58d2.yaml new file mode 100644 index 0000000000..2e744fb279 --- /dev/null +++ b/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`. diff --git a/tests/fakegithub.py b/tests/fakegithub.py index f3ab7e69bc..bed0efe713 100644 --- a/tests/fakegithub.py +++ b/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) diff --git a/tests/fixtures/config/github-file-comments/git/github_common-config/zuul.yaml b/tests/fixtures/config/github-file-comments/git/github_common-config/zuul.yaml new file mode 100644 index 0000000000..bc44471bad --- /dev/null +++ b/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 diff --git a/tests/fixtures/config/github-file-comments/git/org_project/README b/tests/fixtures/config/github-file-comments/git/org_project/README new file mode 100644 index 0000000000..9daeafb986 --- /dev/null +++ b/tests/fixtures/config/github-file-comments/git/org_project/README @@ -0,0 +1 @@ +test diff --git a/tests/fixtures/config/github-file-comments/git/org_project/playbooks/file-comments-error.yaml b/tests/fixtures/config/github-file-comments/git/org_project/playbooks/file-comments-error.yaml new file mode 100644 index 0000000000..a87c1348b8 --- /dev/null +++ b/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" diff --git a/tests/fixtures/config/github-file-comments/git/org_project/playbooks/file-comments.yaml b/tests/fixtures/config/github-file-comments/git/org_project/playbooks/file-comments.yaml new file mode 100644 index 0000000000..5838954e9f --- /dev/null +++ b/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 diff --git a/tests/fixtures/config/github-file-comments/git/org_project/zuul.yaml b/tests/fixtures/config/github-file-comments/git/org_project/zuul.yaml new file mode 100644 index 0000000000..5125c1f0c9 --- /dev/null +++ b/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 diff --git a/tests/fixtures/config/github-file-comments/main.yaml b/tests/fixtures/config/github-file-comments/main.yaml new file mode 100644 index 0000000000..8e4e8eaa2f --- /dev/null +++ b/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 diff --git a/tests/unit/test_github_driver.py b/tests/unit/test_github_driver.py index 3a9cbd0cac..59e1bd43af 100644 --- a/tests/unit/test_github_driver.py +++ b/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 + }) diff --git a/zuul/driver/gerrit/gerritreporter.py b/zuul/driver/gerrit/gerritreporter.py index 52165db322..3f47d39b9e 100644 --- a/zuul/driver/gerrit/gerritreporter.py +++ b/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: diff --git a/zuul/driver/github/githubconnection.py b/zuul/driver/github/githubconnection.py index 0bb1808d79..4a6cb85d5e 100644 --- a/zuul/driver/github/githubconnection.py +++ b/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: diff --git a/zuul/driver/github/githubreporter.py b/zuul/driver/github/githubreporter.py index 93b209242a..2142b79d3a 100644 --- a/zuul/driver/github/githubreporter.py +++ b/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, ) diff --git a/zuul/reporter/__init__.py b/zuul/reporter/__init__.py index a0698ff210..ac5f371d6b 100644 --- a/zuul/reporter/__init__.py +++ b/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