diff --git a/bot/code_review_bot/report/__init__.py b/bot/code_review_bot/report/__init__.py index ed79bb1ec..a0158fa34 100644 --- a/bot/code_review_bot/report/__init__.py +++ b/bot/code_review_bot/report/__init__.py @@ -4,10 +4,10 @@ import structlog +from code_review_bot.report.builderrors import BuildErrorsReporter from code_review_bot.report.github import GithubReporter from code_review_bot.report.lando import LandoReporter from code_review_bot.report.mail import MailReporter -from code_review_bot.report.mail_builderrors import BuildErrorsReporter from code_review_bot.report.phabricator import PhabricatorReporter logger = structlog.get_logger(__name__) diff --git a/bot/code_review_bot/report/mail_builderrors.py b/bot/code_review_bot/report/builderrors.py similarity index 60% rename from bot/code_review_bot/report/mail_builderrors.py rename to bot/code_review_bot/report/builderrors.py index fd57ace18..0dfbeaf83 100644 --- a/bot/code_review_bot/report/mail_builderrors.py +++ b/bot/code_review_bot/report/builderrors.py @@ -4,9 +4,10 @@ import structlog -from code_review_bot import taskcluster +from code_review_bot import Issue, taskcluster from code_review_bot.report.base import Reporter -from code_review_bot.revisions import PhabricatorRevision +from code_review_bot.revisions import GithubRevision, PhabricatorRevision +from code_review_bot.sources.github import GithubClient logger = structlog.get_logger(__name__) @@ -22,25 +23,43 @@ class BuildErrorsReporter(Reporter): """ - Send an email to the author of the revision in case there are build errors + In case there are build errors, notify the author of the revision: + * By email in case of a Phabricator revision + * In a PR thread in case of a Github revision """ def __init__(self, configuration): # Load TC services self.notify = taskcluster.get_service("notify") - + self.github_client = GithubClient.from_configuration(configuration) logger.info("BuildErrorsReporter report enabled.") - def publish(self, issues, revision, task_failures, links, reviewers): + def publish_github(self, revision: GithubRevision, build_errors: list[Issue]): """ - Send an email to the author of the revision + Comment directly on the Pull Request, as a comment mentioning the author """ - if not isinstance(revision, PhabricatorRevision): - logger.info( - "Skipping build error reporting, only available for Phabricator revisions" + if not self.github_client: + logger.error( + "Github API client is not initialized, skipping Github reporting" ) return + messages = [f"Hello @{revision.pull_request.user.login},"] + messages.append( + f"[Code Review bot](https://github.com/mozilla/code-review) detected {len(build_errors)} build errors when analyzing this Pull Request:" + ) + for issue in build_errors: + messages.append(issue.as_error()) + + content = "\n".join(messages) + self.github_client.publish_comment(revision, content) + + def publish_phabricator( + self, revision: PhabricatorRevision, build_errors: list[Issue] + ): + """ + Notify by email the author of the last commit for this revision + """ assert ( revision.phabricator_id and revision.phabricator_phid ), "PhabricatorRevision must have a Phabricator ID and PHID" @@ -56,12 +75,6 @@ def publish(self, issues, revision, task_failures, links, reviewers): ) return - build_errors = [issue for issue in issues if issue.is_build_error()] - - if not build_errors: - logger.info("No build errors encountered.") - return - content = EMAIL_HEADER.format( build_errors=len(build_errors), phabricator_id=revision.phabricator_id, @@ -82,7 +95,7 @@ def publish(self, issues, revision, task_failures, links, reviewers): logger.info("Send build error email", to=commit["author"]["email"]) - # Since we nw know that there is an "author" field we assume that we have "email" + # Since we now know that there is an "author" field we assume that we have "email" self.notify.email( { "address": commit["author"]["email"], @@ -93,3 +106,21 @@ def publish(self, issues, revision, task_failures, links, reviewers): "content": content, } ) + + def publish(self, issues, revision, task_failures, links, reviewers): + build_errors = [issue for issue in issues if issue.is_build_error()] + + if not build_errors: + logger.info("No build errors encountered.") + return + + if isinstance(revision, GithubRevision): + self.publish_github(revision, build_errors) + + elif isinstance(revision, PhabricatorRevision): + self.publish_phabricator(revision, build_errors) + + else: + raise NotImplementedError( + "Only Github and Phabricator revisions are supported" + ) diff --git a/bot/code_review_bot/report/github.py b/bot/code_review_bot/report/github.py index 9a3498b4b..e81031f3e 100644 --- a/bot/code_review_bot/report/github.py +++ b/bot/code_review_bot/report/github.py @@ -19,12 +19,7 @@ def __init__(self, configuration={}, *args, **kwargs): if not configuration.get(key): raise Exception(f"Missing github reporter configuration key {key}") - # Setup github App secret from the configuration - self.github_client = GithubClient( - client_id=configuration["client_id"], - private_key=configuration["private_key_pem"], - installation_id=configuration["installation_id"], - ) + self.github_client = GithubClient.from_configuration(configuration) self.analyzers_skipped = configuration.get("analyzers_skipped", []) assert isinstance( @@ -40,6 +35,11 @@ def publish(self, issues, revision, task_failures, notices, reviewers): "Skipping github reporting, only available for Github revisions" ) return + if not self.github_client: + logger.error( + "Github API client is not initialized, skipping Github reporting" + ) + return if reviewers: logger.warn( diff --git a/bot/code_review_bot/revisions/github.py b/bot/code_review_bot/revisions/github.py index 21e4bb3de..249b34758 100644 --- a/bot/code_review_bot/revisions/github.py +++ b/bot/code_review_bot/revisions/github.py @@ -93,6 +93,7 @@ def as_dict(self): def pull_request(self): from code_review_bot.sources.github import GithubClient + # Setup Github API client, needed to get PR information reporter_conf = next( ( reporter @@ -101,14 +102,9 @@ def pull_request(self): ), None, ) - # A github reporter configuration is required to perform a github Pull Request analysis - assert reporter_conf, "Github reporter secrets must be set to access information about the pull request" - client = GithubClient( - client_id=reporter_conf["client_id"], - private_key=reporter_conf["private_key_pem"], - installation_id=reporter_conf["installation_id"], - ) - return client.get_pull_request(self) + assert reporter_conf, "Github reporter secrets must be set to access information about the Pull Request" + github_client = GithubClient.from_configuration(reporter_conf) + return github_client.get_pull_request(self) def serialize(self): """ diff --git a/bot/code_review_bot/sources/github.py b/bot/code_review_bot/sources/github.py index 7c3244aaa..d85461fb7 100644 --- a/bot/code_review_bot/sources/github.py +++ b/bot/code_review_bot/sources/github.py @@ -17,6 +17,11 @@ logger = structlog.get_logger(__name__) +# Github does not provide an exact limit for the body content on its API. +# We prefer using a safe interval here, based on https://github.com/dead-claudia/github-limits#issue-comments +GITHUB_COMMENT_LIMIT = 65536 + + class ReviewEvent(enum.Enum): """ Review action you want to perform. @@ -50,16 +55,37 @@ def __init__(self, client_id: str, private_key: str, installation_id: str): self.review_comments = [] + @classmethod + def from_configuration(cls, configuration: dict): + """Setup github App secrets from the configuration""" + if not all( + configuration.get(key) + for key in ("client_id", "private_key_pem", "installation_id") + ): + logger.warning( + "Missing github reporter configuration key. Github API client was not initialized" + ) + return + return cls( + client_id=configuration["client_id"], + private_key=configuration["private_key_pem"], + installation_id=configuration["installation_id"], + ) + @lru_cache def get_pull_request(self, revision: GithubRevision): repo = self.api.get_repo(revision.repo_name) return repo.get_pull(revision.pull_number) def _build_review_comment(self, issue): + message = issue.message + if len(message) > GITHUB_COMMENT_LIMIT: + message = message[: GITHUB_COMMENT_LIMIT - 1] + "…" + return ReviewComment( path=issue.path, line=issue.line, - body=issue.message, + body=message, ) def publish_comment( @@ -73,6 +99,10 @@ def publish_comment( assert isinstance(revision, GithubRevision), "Only for github revisions" pull_request = self.get_pull_request(revision) + + if len(message) > GITHUB_COMMENT_LIMIT: + message = message[: GITHUB_COMMENT_LIMIT - 1] + "…" + pull_request.create_issue_comment(body=message) def publish_review( diff --git a/bot/tests/conftest.py b/bot/tests/conftest.py index 57a7d5c1d..f4fa520f8 100644 --- a/bot/tests/conftest.py +++ b/bot/tests/conftest.py @@ -336,6 +336,7 @@ def mock_github(mock_config): json={ "url": "https://api.github.com/repos/owner/repo-name/pulls/1", "issue_url": "https://api.github.com/repos/owner/repo-name/pulls/1", + "user": {"login": "test_user"}, }, ) responses.add( diff --git a/bot/tests/test_reporter_builderrors.py b/bot/tests/test_reporter_builderrors.py new file mode 100644 index 000000000..123c90b5f --- /dev/null +++ b/bot/tests/test_reporter_builderrors.py @@ -0,0 +1,99 @@ +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, You can obtain one at http://mozilla.org/MPL/2.0/. + +import json +from pathlib import Path +from textwrap import dedent + +import responses +from conftest import FIXTURES_DIR +from responses import matchers + +from code_review_bot.report.builderrors import BuildErrorsReporter + +MAIL_CONTENT_BUILD_ERRORS = """ +# [Code Review bot](https://github.com/mozilla/code-review) found 2 build errors on [D51](https://phabricator.test/D51) + + +**Message**: ```Unidentified symbol``` +**Location**: some/file/path:0 + + +**Message**: ```Unidentified symbol``` +**Location**: some/file/path:1 +""" + + +def test_builderrors_taskcluster( + log, mock_config, mock_clang_tidy_issues, mock_revision, mock_taskcluster_config +): + """ + Test builderrors sending email to the author of the Phabricator revision + """ + + def _check_email(request): + payload = json.loads(request.body) + + assert payload["subject"] == "Code Review bot found 2 build errors on D51" + assert payload["address"] == "test@mozilla.com" + assert payload["content"] == MAIL_CONTENT_BUILD_ERRORS + + return (200, {}, "") # ack + + # Add mock taskcluster email to check output + responses.add_callback( + responses.POST, + "http://taskcluster.test/api/notify/v1/email", + callback=_check_email, + ) + + # Publish email + conf = {"emails": ["test@mozilla.com"]} + r = BuildErrorsReporter(conf) + + r.publish(mock_clang_tidy_issues, mock_revision, [], [], []) + + assert log.has("Send build error email", to="test@mozilla.com") + + +def test_builderrors_github( + log, + mock_config, + mock_clang_tidy_issues, + mock_github_revision, + mock_taskcluster_config, +): + """ + Test builderrors commenting on the github Pull Request to mention build errors + """ + mock_taskcluster_config.secrets = { + "REPORTERS": [ + { + "reporter": "github", + "client_id": "client_id", + "private_key_pem": (Path(FIXTURES_DIR) / "private_key.pem").read_text(), + "installation_id": 123456789, + } + ] + } + + responses.add( + responses.POST, + "https://api.github.com:443/repos/owner/repo-name/pulls/1/comments", + match=[ + matchers.json_params_matcher( + { + "body": dedent(""" + Hello @test_user, + [Code Review bot](https://github.com/mozilla/code-review) detected 1 build errors when analyzing this Pull Request: + + **Message**: ```Some Error Message``` + **Location**: dom/animation/Animation.cpp:57 + """).lstrip() + } + ) + ], + ) + r = BuildErrorsReporter({}) + r.publish(mock_clang_tidy_issues, mock_github_revision, [], [], []) diff --git a/bot/tests/test_reporter_mail.py b/bot/tests/test_reporter_mail.py index 650ad8416..58e1af4de 100644 --- a/bot/tests/test_reporter_mail.py +++ b/bot/tests/test_reporter_mail.py @@ -34,19 +34,6 @@ """ -MAIL_CONTENT_BUILD_ERRORS = """ -# [Code Review bot](https://github.com/mozilla/code-review) found 2 build errors on [D51](https://phabricator.test/D51) - - -**Message**: ```Unidentified symbol``` -**Location**: some/file/path:0 - - -**Message**: ```Unidentified symbol``` -**Location**: some/file/path:1 -""" - - def test_conf(mock_config, mock_taskcluster_config): """ Test mail reporter configuration @@ -137,36 +124,3 @@ def _check_email(request): "nb_errors": 2, } ] - - -def test_mail_builderrors( - log, mock_config, mock_clang_tidy_issues, mock_revision, mock_taskcluster_config -): - """ - Test mail_builderrors sending through Taskcluster - """ - from code_review_bot.report.mail_builderrors import BuildErrorsReporter - - def _check_email(request): - payload = json.loads(request.body) - - assert payload["subject"] == "Code Review bot found 2 build errors on D51" - assert payload["address"] == "test@mozilla.com" - assert payload["content"] == MAIL_CONTENT_BUILD_ERRORS - - return (200, {}, "") # ack - - # Add mock taskcluster email to check output - responses.add_callback( - responses.POST, - "http://taskcluster.test/api/notify/v1/email", - callback=_check_email, - ) - - # Publish email - conf = {"emails": ["test@mozilla.com"]} - r = BuildErrorsReporter(conf) - - r.publish(mock_clang_tidy_issues, mock_revision, [], [], []) - - assert log.has("Send build error email", to="test@mozilla.com")