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