[ISV-5736] Approved label is used instead of PR reviews (#804)

* [ISV-5736] Approved label is used instead of PR reviews

Author: mantomas

ansible/roles/operator-pipeline/templates/openshift/tasks/merge-pr.yml

@@ -63,31 +63,16 @@ spec:
           exit 0
         fi
 
-        # To avoid issuing too many GH API requests, fetch all PR info we need
-        # in a single call upfront and process the result later
-        gh pr view "$(params.git_pr_url)" --json isDraft,reviews,author >/tmp/pr.json
+        # we are not using GH reviews for approval, so the only thing we need to check is the presence
+        # of 'approved' label (added by reviewer through '/approve' comment or by the pipeline)
+        gh pr view "$(params.git_pr_url)" --json labels >/tmp/pr.json
+        IS_APPROVED=$(jq -r '[.labels[].name == "approved"] | any' /tmp/pr.json || echo "false")
 
 
-        # Extract all reviews and return one line per reviewer containing three space separated fields:
-        #   $state $authorAssociation $author
-        # where
-        #   $state is the outcome of the most recent review by the author and can be APPROVED, CHANGES_REQUESTED,
-        #     DISMISSED, COMMENTED or PENDING
-        #   $authorAssociation is the role of the author in the repository; for possible values see
-        #     https://docs.github.com/en/graphql/reference/enums#commentauthorassociation
-        #   $author is the GitHub handle of the reviewer
-        # Example:
-        #   APPROVED MEMBER rh-operator-bundle-bot
-        #   PENDING NONE randomuser
-        jq -r '[.reviews[]|{author:.author.login,authorAssociation,state,submittedAt}]|group_by(.author)|map(sort_by(.submittedAt)|.[-1]|"\(.state) \(.authorAssociation) \(.author)")[]' \
-          /tmp/pr.json | tee /tmp/reviews.txt
-
-
-        # Check if a PR author is a member matches a bot name
-        if [[ "$(jq -r '.author.login' /tmp/pr.json)" == "rh-operator-bundle-bot" ]]; then
-            echo "Author of the PR is known bot, merge is allowed"
-        elif ! grep "^APPROVED MEMBER " /tmp/reviews.txt ; then
-        # Do not merge if we do not have approval from the bot or any other repo member
+        if [ "$IS_APPROVED" = "true" ]; then
+            echo "PR is approved and can be merged"
+        else
+            # Do not merge if we do not have approval
             echo -n "Skipping merge: PR is not approved." | tee "$(workspaces.output.path)/merge_error.txt"
             echo -n "false" > "$(results.pr_merged.path)"
             exit 0

operator-pipeline-images/operatorcert/entrypoints/check_permissions.py

@@ -7,8 +7,14 @@
 import urllib
 from typing import Any
 
-from github import Auth, Github, UnknownObjectException
+from github import (
+    Auth,
+    Github,
+    UnknownObjectException,
+    GithubException,
+)
 from operatorcert import pyxis
+from operatorcert.github import add_labels_to_pull_request, parse_github_issue_url
 from operatorcert.logger import setup_logger
 from operatorcert.operator_repo import Operator
 from operatorcert.operator_repo import Repo as OperatorRepo
@@ -213,7 +219,11 @@ def check_permissions(self) -> bool:
             return True
         if self.is_partner():
             return self.check_permission_for_partner()
-        return self.check_permission_for_community()
+        if self.check_permission_for_community():
+            return True
+        # Enable PR approval on forked repository
+        # and approves rh-operator-bundle-bot PRs
+        return self.pr_owner_can_write()
 
     def is_org_member(self) -> bool:
         """
@@ -248,6 +258,35 @@ def is_org_member(self) -> bool:
         )
         return False
 
+    def pr_owner_can_write(self) -> bool:
+        """
+        Check if the pull request owner has write permissions for the repository
+
+        Returns:
+            bool: A boolean value indicating if the user
+                  has write permissions for the repository
+        """
+        LOGGER.info(
+            "Checking if the pull request owner has write permissions for the repository"
+        )
+        github_auth = Auth.Token(os.environ.get("GITHUB_TOKEN") or "")
+        github = Github(auth=github_auth)
+        repo = github.get_repo(self.github_repo_name)
+        try:
+            permission = repo.get_collaborator_permission(self.pr_owner)
+        except GithubException:
+            LOGGER.info(
+                "Cannot get permissions for the pull request owner '%s'", self.pr_owner
+            )
+            return False
+        if permission in ["admin", "write"]:
+            LOGGER.info(
+                "Pull request owner '%s' has write permissions for the repository",
+                self.pr_owner,
+            )
+            return True
+        return False
+
     def check_permission_for_partner(self) -> bool:
         """
         Check if the pull request owner has permissions to submit a PR for the for
@@ -328,22 +367,21 @@ def check_permission_for_community(self) -> bool:
 
     def request_review_from_maintainers(self) -> None:
         """
-        Request review from repository global maintainers.
-        The maintainers are tagged in the PR as reviewers.
+        Request review from the operator maintainers by adding a comment to the PR.
         """
         LOGGER.info(
             "Requesting a review from maintainers for the pull request: %s",
             self.pull_request_url,
         )
+        maintainers_with_at = ", ".join(map(lambda x: f"@{x}", self.maintainers))
+        comment_text = (
+            "This PR requires a review from repository maintainers.\n"
+            f"{maintainers_with_at}: please review the PR and approve it with an "
+            "`approved` label if the pipeline is still running or merge the PR "
+            "directly after review if the pipeline already passed successfully."
+        )
         run_command(
-            [
-                "gh",
-                "pr",
-                "edit",
-                self.pull_request_url,
-                "--add-reviewer",
-                ",".join(self.maintainers),
-            ]
+            ["gh", "pr", "comment", self.pull_request_url, "--body", comment_text]
         )
 
     def request_review_from_owners(self) -> None:
@@ -461,6 +499,21 @@ def check_permissions(
     return all(is_approved)
 
 
+def add_label_approved(pull_request_url: str) -> None:
+    """
+    Add 'approved' label to the pull request
+
+    Args:
+        pull_request_url (str): URL to the pull request
+    """
+    repository_name, pr_id = parse_github_issue_url(pull_request_url)
+    github_auth = Auth.Token(os.environ.get("GITHUB_TOKEN") or "")
+    github = Github(auth=github_auth)
+    repository = github.get_repo(repository_name)
+    pull_request = repository.get_pull(pr_id)
+    add_labels_to_pull_request(pull_request, ["approved"])
+
+
 def main() -> None:
     """
     Main function of the script
@@ -481,7 +534,7 @@ def main() -> None:
     is_approved = check_permissions(base_repo, head_repo, args)
 
     if is_approved:
-        run_command(["gh", "pr", "review", args.pull_request_url, "--approve"])
+        add_label_approved(args.pull_request_url)
 
     # Save the results to a file
     output = {

operator-pipeline-images/tests/entrypoints/test_check_permissions.py

@@ -1,11 +1,11 @@
 from pathlib import Path
-from typing import Any, Optional
+from typing import Any
 from unittest import mock
 from unittest.mock import MagicMock, call, patch
 
 import operatorcert.entrypoints.check_permissions as check_permissions
 import pytest
-from github import UnknownObjectException
+from github import GithubException, UnknownObjectException
 from operatorcert.operator_repo import Repo as OperatorRepo
 from tests.operator_repo import bundle_files, create_files
 
@@ -133,20 +133,41 @@ def _mock_label(name: str) -> MagicMock:
 
 
 @pytest.mark.parametrize(
-    "is_org_member, is_partner, permission_partner, permission_community, permission_partner_called, permission_community_called, expected_result",
+    "is_org_member, is_partner, owner_can_write, permission_partner, permission_community, permission_partner_called, permission_community_called, expected_result",
     [
-        pytest.param(True, False, False, False, False, False, True, id="org member"),
         pytest.param(
-            False, True, True, False, True, False, True, id="partner - approved"
+            True, False, False, False, False, False, False, True, id="org member"
         ),
         pytest.param(
-            False, True, False, False, True, False, False, id="partner - denied"
+            False, True, False, True, False, True, False, True, id="partner - approved"
         ),
         pytest.param(
-            False, False, False, True, False, True, True, id="community - approved"
+            False, True, False, False, False, True, False, False, id="partner - denied"
         ),
         pytest.param(
-            False, False, False, False, False, True, False, id="community - denied"
+            False, False, True, False, False, False, True, True, id="owner - approved"
+        ),
+        pytest.param(
+            False,
+            False,
+            False,
+            False,
+            True,
+            False,
+            True,
+            True,
+            id="community - approved",
+        ),
+        pytest.param(
+            False,
+            False,
+            False,
+            False,
+            False,
+            False,
+            True,
+            False,
+            id="community - denied",
         ),
     ],
 )
@@ -158,14 +179,17 @@ def _mock_label(name: str) -> MagicMock:
 )
 @patch("operatorcert.entrypoints.check_permissions.OperatorReview.is_partner")
 @patch("operatorcert.entrypoints.check_permissions.OperatorReview.is_org_member")
+@patch("operatorcert.entrypoints.check_permissions.OperatorReview.pr_owner_can_write")
 def test_OperatorReview_check_permissions(
+    mock_pr_owner_can_write: MagicMock,
     mock_is_org_member: MagicMock,
     mock_is_partner: MagicMock,
     mock_check_permission_for_partner: MagicMock,
     mock_check_permission_for_community: MagicMock,
     review_community: check_permissions.OperatorReview,
     is_org_member: bool,
     is_partner: bool,
+    owner_can_write: bool,
     permission_partner: bool,
     permission_community: bool,
     permission_partner_called: bool,
@@ -174,6 +198,7 @@ def test_OperatorReview_check_permissions(
 ) -> None:
     mock_is_org_member.return_value = is_org_member
     mock_is_partner.return_value = is_partner
+    mock_pr_owner_can_write.return_value = owner_can_write
     mock_check_permission_for_partner.return_value = permission_partner
     mock_check_permission_for_community.return_value = permission_community
     assert review_community.check_permissions() == expected_result
@@ -216,6 +241,29 @@ def test_OperatorReview_is_org_member(
     assert review_community.is_org_member() == False
 
 
+@patch("operatorcert.entrypoints.check_permissions.Github")
+@patch("operatorcert.entrypoints.check_permissions.Auth.Token")
+def test_OperatorReview_pr_owner_can_write(
+    mock_token: MagicMock,
+    mock_github: MagicMock,
+    review_community: check_permissions.OperatorReview,
+) -> None:
+    mock_repo = MagicMock()
+    mock_github.return_value.get_repo.return_value = mock_repo
+
+    # User can write to the repository
+    mock_repo.get_collaborator_permission.return_value = "write"
+    assert review_community.pr_owner_can_write() == True
+
+    # User cannot write to the repository
+    mock_repo.get_collaborator_permission.return_value = "read"
+    assert review_community.pr_owner_can_write() == False
+
+    # Cannot get collaborator permission
+    mock_repo.get_collaborator_permission.side_effect = GithubException(404, "", {})
+    assert review_community.pr_owner_can_write() == False
+
+
 @pytest.mark.parametrize(
     ["project", "valid"],
     [
@@ -331,10 +379,13 @@ def test_OperatorReview_request_review_from_maintainers(
         [
             "gh",
             "pr",
-            "edit",
+            "comment",
             review_community.pull_request_url,
-            "--add-reviewer",
-            "maintainer1,maintainer2",
+            "--body",
+            "This PR requires a review from repository maintainers.\n"
+            f"@maintainer1, @maintainer2: please review the PR and approve it with an "
+            "`approved` label if the pipeline is still running or merge the PR "
+            "directly after review if the pipeline already passed successfully.",
         ]
     )
 
@@ -446,8 +497,27 @@ def test_check_permissions(
     )
 
 
+@patch("operatorcert.entrypoints.check_permissions.Github")
+@patch("operatorcert.entrypoints.check_permissions.Auth.Token")
+@patch("operatorcert.entrypoints.check_permissions.add_labels_to_pull_request")
+def test_add_label_approved(
+    mock_add_labels: MagicMock,
+    mock_token: MagicMock,
+    mock_github: MagicMock,
+) -> None:
+    mock_repo = MagicMock()
+    mock_github.return_value.get_repo.return_value = mock_repo
+    mock_pull = MagicMock()
+    mock_repo.get_pull.return_value = mock_pull
+
+    check_permissions.add_label_approved("https://github.com/my-org/repo-123/pulls/1")
+    mock_github.return_value.get_repo.assert_has_calls([call("my-org/repo-123")])
+    mock_repo.get_pull.assert_called_once_with(1)
+    mock_add_labels.assert_called_once_with(mock_pull, ["approved"])
+
+
 @patch("operatorcert.entrypoints.check_permissions.json.dump")
-@patch("operatorcert.entrypoints.check_permissions.run_command")
+@patch("operatorcert.entrypoints.check_permissions.add_label_approved")
 @patch("operatorcert.entrypoints.check_permissions.check_permissions")
 @patch("operatorcert.entrypoints.check_permissions.OperatorRepo")
 @patch("operatorcert.entrypoints.check_permissions.setup_logger")
@@ -457,7 +527,7 @@ def test_main(
     mock_setup_logger: MagicMock,
     mock_operator_repo: MagicMock,
     mock_check_permissions: MagicMock,
-    mock_run_command: MagicMock,
+    mock_add_label_approved: MagicMock,
     mock_json_dump: MagicMock,
     monkeypatch: Any,
     tmp_path: Path,
@@ -480,9 +550,7 @@ def test_main(
     check_permissions.main()
 
     mock_check_permissions.assert_called_once_with(base_repo, head_repo, args)
-    mock_run_command.assert_called_once_with(
-        ["gh", "pr", "review", args.pull_request_url, "--approve"]
-    )
+    mock_add_label_approved.assert_called_once_with(args.pull_request_url)
 
     expected_output = {
         "approved": mock_check_permissions.return_value,