Publish a comment to the Github PR mentioning the build errors

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__)

bot/code_review_bot/report/mail_builderrors.py → 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"
+            )

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(

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):
         """

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(

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(

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, [], [], [])

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")