diff --git a/bot/code_review_bot/report/github.py b/bot/code_review_bot/report/github.py index 735ee807e..9a3498b4b 100644 --- a/bot/code_review_bot/report/github.py +++ b/bot/code_review_bot/report/github.py @@ -42,7 +42,10 @@ def publish(self, issues, revision, task_failures, notices, reviewers): return if reviewers: - raise NotImplementedError + logger.warn( + f"These reviewer groups should be assigned, but it's not yet possible on Github: {', '.join(reviewers)}" + ) + # Avoid publishing a patch from a de-activated analyzer publishable_issues = [ issue @@ -51,19 +54,29 @@ def publish(self, issues, revision, task_failures, notices, reviewers): and issue.analyzer.name not in self.analyzers_skipped ] + # Remove any earlier review to get a clean state + nb_dismissed = self.github_client.cleanup_pr(revision) + if publishable_issues: # Publish a review summarizing detected, unresolved and closed issues - message = f"{len(issues)} issues have been found in this revision" - event = ReviewEvent.RequestChanges + self.github_client.publish_review( + issues=publishable_issues, + revision=revision, + message=f"{len(publishable_issues)} issues have been found in this revision", + event=ReviewEvent.RequestChanges, + ) else: - # Simply approve the pull request - logger.info("No publishable issue, approving the pull request") - message = None - event = ReviewEvent.Approved + # Publish a comment, mentioning if previous issues were cleared up + logger.info( + "No publishable issue, posting a standalone comment", + nb_dismissed=nb_dismissed, + ) + if nb_dismissed > 0: + message = "Previous issues have been fixed. This pull request is :ok:" + else: + message = "No new issues detected. This pull request is :ok:" - self.github_client.publish_review( - issues=publishable_issues, - revision=revision, - message=message, - event=event, - ) + self.github_client.publish_comment( + revision=revision, + message=message, + ) diff --git a/bot/code_review_bot/sources/github.py b/bot/code_review_bot/sources/github.py index 84e2d0130..7c3244aaa 100644 --- a/bot/code_review_bot/sources/github.py +++ b/bot/code_review_bot/sources/github.py @@ -5,6 +5,7 @@ # file, You can obtain one at http://mozilla.org/MPL/2.0/. import enum +from functools import lru_cache import structlog from github import Auth, GithubIntegration @@ -49,6 +50,7 @@ def __init__(self, client_id: str, private_key: str, installation_id: str): self.review_comments = [] + @lru_cache def get_pull_request(self, revision: GithubRevision): repo = self.api.get_repo(revision.repo_name) return repo.get_pull(revision.pull_number) @@ -60,6 +62,19 @@ def _build_review_comment(self, issue): body=issue.message, ) + def publish_comment( + self, + revision: GithubRevision, + message: str | None = None, + ): + """ + Publish a comment on a pull request + """ + assert isinstance(revision, GithubRevision), "Only for github revisions" + + pull_request = self.get_pull_request(revision) + pull_request.create_issue_comment(body=message) + def publish_review( self, issues: list[Issue], @@ -70,12 +85,7 @@ def publish_review( """ Publish a review from a list of publishable issues, requesting changes to the author. """ - - if not isinstance(revision, GithubRevision): - logger.warning( - f"Revision must originate from Github in order to publish a review, skipping {revision}." - ) - return + assert isinstance(revision, GithubRevision), "Only for github revisions" repo = self.api.get_repo(revision.repo_name) pull_request = repo.get_pull(revision.pull_number) @@ -95,3 +105,40 @@ def publish_review( event=event.value, **attrs, ) + + def cleanup_pr(self, revision: GithubRevision): + """ + Dismiss previous reviews from the bot + """ + assert isinstance(revision, GithubRevision), "Only for github revisions" + + pr = self.get_pull_request(revision) + + nb = 0 + for review in pr.get_reviews(): + # Only process our own reviews + if review.user.login != "mozilla-code-review[bot]": + continue + + # Only process active reviews + if review.state == "DISMISSED": + continue + + try: + review.dismiss("This review is now deprecated.") + logger.info( + "Dismissed previous Github review from the bot", + review=review.id, + submitted=review.submitted_at, + ) + nb += 1 + except Exception as e: + logger.warn( + "Failed to dismiss previous Github review from the bot", + review=review.id, + submitted=review.submitted_at, + error=e, + ) + raise # trashme + + return nb diff --git a/bot/tests/conftest.py b/bot/tests/conftest.py index 4b4e680ee..57a7d5c1d 100644 --- a/bot/tests/conftest.py +++ b/bot/tests/conftest.py @@ -333,7 +333,10 @@ def mock_github(mock_config): responses.add( responses.GET, "https://api.github.com:443/repos/owner/repo-name/pulls/1", - json={"url": "https://api.github.com/repos/owner/repo-name/pulls/1"}, + json={ + "url": "https://api.github.com/repos/owner/repo-name/pulls/1", + "issue_url": "https://api.github.com/repos/owner/repo-name/pulls/1", + }, ) responses.add( responses.GET, diff --git a/bot/tests/test_reporter_github.py b/bot/tests/test_reporter_github.py index 1160cefe2..85fa75fad 100644 --- a/bot/tests/test_reporter_github.py +++ b/bot/tests/test_reporter_github.py @@ -68,12 +68,20 @@ def test_github_review( ) assert issue_coverage.is_publishable() + # Mock to publish a new review responses.add( responses.POST, "https://api.github.com:443/repos/owner/repo-name/pulls/1/reviews", json={}, ) + # Mock to list existing reviews + responses.add( + responses.GET, + "https://api.github.com:443/repos/owner/repo-name/pulls/1/reviews", + json=[], + ) + reporter.publish([issue_clang_tidy, issue_coverage], revision, [], [], []) assert [(call.request.method, call.request.url) for call in responses.calls] == [ ("GET", "https://github.com/owner/repo-name/pull/1.diff"), @@ -84,6 +92,18 @@ def test_github_review( ), ("GET", "https://api.github.com:443/repos/owner/repo-name"), ("GET", "https://api.github.com:443/repos/owner/repo-name/pulls/1"), + ( + "GET", + "https://api.github.com:443/repos/owner/repo-name/pulls/1/reviews", + ), + ( + "GET", + "https://api.github.com:443/repos/owner/repo-name", + ), + ( + "GET", + "https://api.github.com:443/repos/owner/repo-name/pulls/1", + ), ( "GET", "https://api.github.com:443/repos/owner/repo-name/commits/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", @@ -110,7 +130,7 @@ def test_github_review( } -def test_github_review_approve( +def test_github_review_cleanup( monkeypatch, mock_github, mock_config, @@ -120,7 +140,7 @@ def test_github_review_approve( mock_task, mock_backend_secret, ): - """In case no issue is found, the pull request is approved""" + """In case no issue is found, previous reviews are dismissed""" revision = Revision.from_try_task(mock_try_task, mock_github_decision_task, None) revision.lines = {} revision.files = ["test.txt", "test.cpp", "another_test.cpp"] @@ -134,8 +154,27 @@ def test_github_review_approve( ) responses.add( - responses.POST, + responses.GET, "https://api.github.com:443/repos/owner/repo-name/pulls/1/reviews", + json=[ + {"id": 1, "user": {"login": "a-moz-developer"}}, + { + "id": 2, + "user": {"login": "mozilla-code-review[bot]"}, + "pull_request_url": "https://api.github.com/repos/owner/repo-name/pulls/2", + }, + ], + ) + + responses.add( + responses.PUT, + "https://api.github.com:443/repos/owner/repo-name/pulls/2/reviews/2/dismissals", + json={}, + ) + + responses.add( + responses.POST, + "https://api.github.com:443/repos/owner/repo-name/pulls/1/comments", json={}, ) @@ -149,15 +188,18 @@ def test_github_review_approve( ), ("GET", "https://api.github.com:443/repos/owner/repo-name"), ("GET", "https://api.github.com:443/repos/owner/repo-name/pulls/1"), + ("GET", "https://api.github.com:443/repos/owner/repo-name/pulls/1/reviews"), ( - "GET", - "https://api.github.com:443/repos/owner/repo-name/commits/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "PUT", + "https://api.github.com:443/repos/owner/repo-name/pulls/2/reviews/2/dismissals", + ), + ( + "POST", + "https://api.github.com:443/repos/owner/repo-name/pulls/1/comments", ), - ("POST", "https://api.github.com:443/repos/owner/repo-name/pulls/1/reviews"), ] - review_creation = responses.calls[-1] - assert json.loads(review_creation.request.body) == { - "comments": [], - "commit_id": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", - "event": "APPROVE", + + # Check published comment + assert json.loads(responses.calls[-1].request.body) == { + "body": "Previous issues have been fixed. This pull request is :ok:" }