From 42954161386beca6e2c9536b64095c1606858a79 Mon Sep 17 00:00:00 2001 From: Maurizio Porrato Date: Fri, 1 Nov 2024 11:00:03 +0000 Subject: [PATCH] [ISV-4528] Automatically merge approved PRs --- .../entrypoints/check_permissions.py | 58 ++++++++-- .../entrypoints/test_check_permissions.py | 101 +++++++++++++++--- 2 files changed, 133 insertions(+), 26 deletions(-) diff --git a/operator-pipeline-images/operatorcert/entrypoints/check_permissions.py b/operator-pipeline-images/operatorcert/entrypoints/check_permissions.py index 46f81dbe..3a890a52 100644 --- a/operator-pipeline-images/operatorcert/entrypoints/check_permissions.py +++ b/operator-pipeline-images/operatorcert/entrypoints/check_permissions.py @@ -167,6 +167,34 @@ def github_repo_org(self) -> str: """ return urllib.parse.urlparse(self.pull_request_url).path.split("/")[1] + @property + def github_repo_name(self) -> str: + """ + The name of the github repo the pull request belongs to + + Returns: + str: the github repo name in the form "organization_name/repo_name" + """ + return "/".join( + urllib.parse.urlparse(self.pull_request_url).path.split("/")[1:3] + ) + + @property + def pr_labels(self) -> set[str]: + """ + The set of labels applied to the pull request + + Returns: + set[str]: the labels applied to the pull request + """ + github_auth = Auth.Token(os.environ.get("GITHUB_TOKEN") or "") + github = Github(auth=github_auth) + pr_no = int(urllib.parse.urlparse(self.pull_request_url).path.split("/")[-1]) + return { + x.name + for x in github.get_repo(self.github_repo_name).get_pull(pr_no).get_labels() + } + def check_permissions(self) -> bool: """ Check if the pull request owner has permissions to submit a PR for the operator @@ -271,20 +299,27 @@ def check_permission_for_community(self) -> bool: f"{self.operator} does not have any reviewers in the base repository " "or is brand new." ) - if self.pr_owner not in self.reviewers: + + if self.pr_owner in self.reviewers: LOGGER.info( - "Pull request owner %s is not in the list of reviewers %s", + "Pull request owner %s can submit PR for operator %s", self.pr_owner, - self.reviewers, + self.operator.operator_name, ) - self.request_review_from_owners() - return False + return True + + LOGGER.info("Checking if the pull request is approved by a reviewer") + if "approved" in self.pr_labels: + LOGGER.info("Pull request is approved by a reviewer") + return True + LOGGER.info( - "Pull request owner %s can submit PR for operator %s", + "Pull request owner %s is not in the list of reviewers %s", self.pr_owner, - self.operator.operator_name, + self.reviewers, ) - return True + self.request_review_from_owners() + return False def request_review_from_maintainers(self) -> None: """ @@ -313,10 +348,11 @@ def request_review_from_owners(self) -> None: reviewers_with_at = ", ".join(map(lambda x: f"@{x}", self.reviewers)) comment_text = ( "Author of the PR is not listed as one of the reviewers in ci.yaml.\n" - "Please review the PR and approve it with \`/lgtm\` comment.\n" # pylint: disable=anomalous-backslash-in-string + "Please review the PR and approve it with an \`/approve\` comment.\n" # pylint: disable=anomalous-backslash-in-string f"{reviewers_with_at} \n\n" - "Consider adding author of the PR to the ci.yaml file if you want automated " - "approval for a followup submissions." + "Consider adding the author of the PR to the list of reviewers in" + "the ci.yaml file if you want automated merge without explicit" + "approval." ) run_command( diff --git a/operator-pipeline-images/tests/entrypoints/test_check_permissions.py b/operator-pipeline-images/tests/entrypoints/test_check_permissions.py index cea50ef4..541dce82 100644 --- a/operator-pipeline-images/tests/entrypoints/test_check_permissions.py +++ b/operator-pipeline-images/tests/entrypoints/test_check_permissions.py @@ -1,5 +1,5 @@ from pathlib import Path -from typing import Any +from typing import Any, Optional from unittest import mock from unittest.mock import MagicMock, call, patch @@ -105,6 +105,33 @@ def test_OperatorReview_github_repo_org( assert review_community.github_repo_org == "my-org" +def test_OperatorReview_github_repo_name( + review_community: check_permissions.OperatorReview, +) -> None: + assert review_community.github_repo_name == "my-org/repo-123" + + +@patch("operatorcert.entrypoints.check_permissions.Github") +@patch("operatorcert.entrypoints.check_permissions.Auth.Token") +def test_OperatorReview_pr_labels( + mock_token: MagicMock, + mock_github: MagicMock, + review_community: check_permissions.OperatorReview, +) -> None: + repo = MagicMock() + pull = MagicMock() + + def _mock_label(name: str) -> MagicMock: + m = MagicMock() + m.name = name + return m + + pull.get_labels.return_value = [_mock_label("foo"), _mock_label("bar")] + repo.get_pull.return_value = pull + mock_github.return_value.get_repo.return_value = repo + assert review_community.pr_labels == {"foo", "bar"} + + @pytest.mark.parametrize( "is_org_member, is_partner, permission_partner, permission_community, permission_partner_called, permission_community_called, expected_result", [ @@ -225,6 +252,39 @@ def test_OperatorReview_check_permission_for_partner( review_partner.check_permission_for_partner() +@pytest.mark.parametrize( + ["reviewers", "labels", "owners_review", "result"], + [ + pytest.param( + [], + set(), + False, + check_permissions.MaintainersReviewNeeded, + id="No reviewers", + ), + pytest.param( + ["owner", "foo"], + set(), + False, + True, + id="author is reviewer", + ), + pytest.param( + ["foo", "bar"], + set(), + True, + False, + id="author is not reviewer", + ), + pytest.param( + ["bar", "baz"], + {"approved"}, + False, + True, + id="author is not reviewer, PR approved", + ), + ], +) @patch( "operatorcert.entrypoints.check_permissions.OperatorReview.request_review_from_owners" ) @@ -232,24 +292,33 @@ def test_OperatorReview_check_permission_for_partner( "operatorcert.entrypoints.check_permissions.OperatorReview.reviewers", new_callable=mock.PropertyMock, ) +@patch( + "operatorcert.entrypoints.check_permissions.OperatorReview.pr_labels", + new_callable=mock.PropertyMock, +) def test_OperatorReview_check_permission_for_community( + mock_labels: MagicMock, mock_reviewers: MagicMock, mock_review_from_owners: MagicMock, review_community: check_permissions.OperatorReview, + reviewers: list[str], + labels: set[str], + owners_review: bool, + result: bool | type, ) -> None: - mock_reviewers.return_value = [] - with pytest.raises(check_permissions.MaintainersReviewNeeded): - review_community.check_permission_for_community() - - mock_reviewers.return_value = ["user1", "user2"] + mock_reviewers.return_value = reviewers + mock_labels.return_value = labels - assert review_community.check_permission_for_community() == False - mock_review_from_owners.assert_called_once() + if isinstance(result, bool): + assert review_community.check_permission_for_community() == result + else: + with pytest.raises(result): + review_community.check_permission_for_community() - mock_review_from_owners.reset_mock() - mock_reviewers.return_value = ["owner"] - assert review_community.check_permission_for_community() == True - mock_review_from_owners.assert_not_called() + if owners_review: + mock_review_from_owners.assert_called_once() + else: + mock_review_from_owners.assert_not_called() @patch("operatorcert.entrypoints.check_permissions.run_command") @@ -284,9 +353,11 @@ def test_OperatorReview_request_review_from_owners( review_community.pull_request_url, "--body", "Author of the PR is not listed as one of the reviewers in ci.yaml.\n" - "Please review the PR and approve it with \\`/lgtm\\` comment.\n" - "@user1, @user2 \n\nConsider adding author of the PR to the ci.yaml " - "file if you want automated approval for a followup submissions.", + "Please review the PR and approve it with an \\`/approve\\` comment.\n" + "@user1, @user2 \n\n" + "Consider adding the author of the PR to the list of reviewers in" + "the ci.yaml file if you want automated merge without explicit" + "approval.", ] )