Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ISV-4528] Automatically merge approved PRs #746

Merged
merged 2 commits into from
Nov 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
"""
Expand Down Expand Up @@ -312,11 +347,12 @@ 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
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."
"The author of the PR is not listed as one of the reviewers in ci.yaml.\n"
f"{reviewers_with_at}: please review the PR and approve it with an "
"`/approve` comment.\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."
)

run_command(
Expand Down
102 changes: 86 additions & 16 deletions operator-pipeline-images/tests/entrypoints/test_check_permissions.py
Original file line number Diff line number Diff line change
@@ -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

Expand Down Expand Up @@ -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",
[
Expand Down Expand Up @@ -225,31 +252,73 @@ 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"
)
@patch(
"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")
Expand Down Expand Up @@ -283,10 +352,11 @@ def test_OperatorReview_request_review_from_owners(
"comment",
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.",
"The author of the PR is not listed as one of the reviewers in ci.yaml.\n"
"@user1, @user2: please review the PR and approve it with an `/approve` comment.\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.",
]
)

Expand Down