From 909c6cddc12a61df5f2b55b26281c41f5caeb83b Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Thu, 12 Dec 2024 22:22:38 +0200 Subject: [PATCH 1/3] Fix no owners for subfolders --- pytest.ini | 3 +- webhook_server_container/libs/github_api.py | 25 ++- webhook_server_container/tests/conftest.py | 114 +++++++++++++ ...hub_api.py => test_pull_request_owners.py} | 155 ++++++------------ .../tests/test_pull_request_size.py | 42 +++++ 5 files changed, 226 insertions(+), 113 deletions(-) create mode 100644 webhook_server_container/tests/conftest.py rename webhook_server_container/tests/{test_github_api.py => test_pull_request_owners.py} (82%) create mode 100644 webhook_server_container/tests/test_pull_request_size.py diff --git a/pytest.ini b/pytest.ini index fcf52b9a..c6ea1623 100644 --- a/pytest.ini +++ b/pytest.ini @@ -1,3 +1,4 @@ [pytest] addopts = - --cov-config=pyproject.toml --cov-report=html --cov-report=term --cov=github_webhook_server + --pdbcls=IPython.terminal.debugger:TerminalPdb + --cov-config=pyproject.toml --cov-report=html --cov-report=term --cov=webhook_server_container diff --git a/webhook_server_container/libs/github_api.py b/webhook_server_container/libs/github_api.py index f22a2150..61720500 100644 --- a/webhook_server_container/libs/github_api.py +++ b/webhook_server_container/libs/github_api.py @@ -2057,25 +2057,34 @@ def owners_data_for_changed_files(self) -> dict[str, dict[str, Any]]: changed_folders = {Path(cf).parent for cf in self.changed_files} - changed_folder_match: list[str] = [] + changed_folder_match: list[Path] = [] - require_root_approvers: bool = False + require_root_approvers: bool | None = None for owners_dir, owners_data in self.all_approvers_and_reviewers.items(): + if owners_dir == ".": + continue + _owners_dir = Path(owners_dir) for changed_folder in changed_folders: - if _owners_dir == changed_folder or _owners_dir.parents == changed_folders: + if changed_folder == _owners_dir or _owners_dir in changed_folder.parents: data[owners_dir] = owners_data - changed_folder_match.append(owners_dir) - if not require_root_approvers: + changed_folder_match.append(_owners_dir) + if require_root_approvers is None: require_root_approvers = owners_data.get("root-approvers", True) - if [_folder for _folder in changed_folders if str(_folder) not in changed_folder_match]: + if require_root_approvers or require_root_approvers is None: data["."] = self.all_approvers_and_reviewers.get(".", {}) - if require_root_approvers: - data["."] = self.all_approvers_and_reviewers.get(".", {}) + else: + for _folder in changed_folders: + for _changed_path in changed_folder_match: + if _folder == _changed_path or _changed_path in _folder.parents: + continue + else: + data["."] = self.all_approvers_and_reviewers.get(".", {}) + break return data diff --git a/webhook_server_container/tests/conftest.py b/webhook_server_container/tests/conftest.py new file mode 100644 index 00000000..e11fa8d2 --- /dev/null +++ b/webhook_server_container/tests/conftest.py @@ -0,0 +1,114 @@ +import pytest +from starlette.datastructures import Headers + +from simple_logger.logger import logging +import yaml +from webhook_server_container.libs.github_api import ProcessGithubWehook +import os + +ALL_CHANGED_FILES = [ + "OWNERS", + "folder1/OWNERS", + "code/file.py", + "README.md", + "folder2/lib.py", + "folder/folder4/another_file.txt", + "folder5/file", +] + + +class Tree: + def __init__(self, path: str): + self.type = "blob" + self.path = path + + @property + def tree(self): + trees = [] + for _path in [ + "OWNERS", + "folder1/OWNERS", + "folder2/OWNERS", + "folder/folder4/OWNERS", + "folder5/OWNERS", + ]: + trees.append(Tree(_path)) + return trees + + +class ContentFile: + def __init__(self, content: str): + self.content = content + + @property + def decoded_content(self): + return self.content + + +class Repository: + def __init__(self): + self.name = "test-repo" + + def get_git_tree(self, sha: str, recursive: bool): + return Tree("") + + def get_contents(self, path: str): + owners_data = yaml.dump({ + "approvers": ["root_approver1", "root_approver2"], + "reviewers": ["root_reviewer1", "root_reviewer2"], + }) + + folder1_owners_data = yaml.dump({ + "approvers": ["folder1_approver1", "folder1_approver2"], + "reviewers": ["folder1_reviewer1", "folder1_reviewer2"], + }) + + folder4_owners_data = yaml.dump({ + "approvers": ["folder4_approver1", "folder4_approver2"], + "reviewers": ["folder4_reviewer1", "folder4_reviewer2"], + }) + + folder5_owners_data = yaml.dump({ + "root-approvers": False, + "approvers": ["folder5_approver1", "folder5_approver2"], + "reviewers": ["folder5_reviewer1", "folder5_reviewer2"], + }) + if path == "OWNERS": + return ContentFile(owners_data) + + elif path == "folder1/OWNERS": + return ContentFile(folder1_owners_data) + + elif path == "folder2/OWNERS": + return ContentFile(yaml.dump({})) + + elif path == "folder/folder4/OWNERS": + return ContentFile(folder4_owners_data) + + elif path == "folder": + return ContentFile(yaml.dump({})) + + elif path == "folder5/OWNERS": + return ContentFile(folder5_owners_data) + + +@pytest.fixture(scope="function") +def process_github_webhook(mocker, request): + base_import_path = "webhook_server_container.libs.github_api" + os.environ["WEBHOOK_SERVER_DATA_DIR"] = "webhook_server_container/tests/manifests" + + mocker.patch(f"{base_import_path}.get_repository_github_app_api", return_value=True) + mocker.patch("github.AuthenticatedUser", return_value=True) + mocker.patch(f"{base_import_path}.get_api_with_highest_rate_limit", return_value=("API", "TOKEN")) + mocker.patch(f"{base_import_path}.get_github_repo_api", return_value=Repository()) + + process_github_webhook = ProcessGithubWehook( + {"repository": {"name": Repository().name}}, Headers({"X-GitHub-Event": "test-event"}), logging.getLogger() + ) + process_github_webhook.pull_request_branch = "main" + if hasattr(request, "param") and request.param: + process_github_webhook.changed_files = request.param[0] + else: + process_github_webhook.changed_files = ALL_CHANGED_FILES + + return process_github_webhook diff --git a/webhook_server_container/tests/test_github_api.py b/webhook_server_container/tests/test_pull_request_owners.py similarity index 82% rename from webhook_server_container/tests/test_github_api.py rename to webhook_server_container/tests/test_pull_request_owners.py index c8fd2234..5fbc57ec 100644 --- a/webhook_server_container/tests/test_github_api.py +++ b/webhook_server_container/tests/test_pull_request_owners.py @@ -1,54 +1,8 @@ import pytest -from starlette.datastructures import Headers -from simple_logger.logger import logging -from stringcolor.ops import os import yaml -from webhook_server_container.libs.github_api import ProcessGithubWehook -from webhook_server_container.utils.constants import APPROVED_BY_LABEL_PREFIX, SIZE_LABEL_PREFIX - -ALL_CHANGED_FILES = [ - "OWNERS", - "folder1/OWNERS", - "code/file.py", - "README.md", - "folder2/lib.py", - "folder/folder4/another_file.txt", - "folder5/file", -] - - -class Label: - def __init__(self, name: str): - self.name = name - - -class Tree: - def __init__(self, path: str): - self.type = "blob" - self.path = path - - @property - def tree(self): - trees = [] - for _path in [ - "OWNERS", - "folder1/OWNERS", - "folder2/OWNERS", - "folder/folder4/OWNERS", - "folder5/OWNERS", - ]: - trees.append(Tree(_path)) - return trees - - -class ContentFile: - def __init__(self, content: str): - self.content = content - - @property - def decoded_content(self): - return self.content +from webhook_server_container.tests.conftest import ContentFile, Tree +from webhook_server_container.utils.constants import APPROVED_BY_LABEL_PREFIX class Repository: @@ -98,43 +52,6 @@ def get_contents(self, path: str): return ContentFile(folder5_owners_data) -class PullRequest: - def __init__(self, additions: int, deletions: int, labels: list[str] | None = None): - self.additions = additions - self.deletions = deletions - self.labels = labels or [] - - @property - def lables(self) -> list[Label]: - _lables = [] - for label in self.labels: - _lables.append(Label(label)) - - return _lables - - -@pytest.fixture(scope="function") -def process_github_webhook(mocker, request): - base_import_path = "webhook_server_container.libs.github_api" - os.environ["WEBHOOK_SERVER_DATA_DIR"] = "webhook_server_container/tests/manifests" - - mocker.patch(f"{base_import_path}.get_repository_github_app_api", return_value=True) - mocker.patch("github.AuthenticatedUser", return_value=True) - mocker.patch(f"{base_import_path}.get_api_with_highest_rate_limit", return_value=("API", "TOKEN")) - mocker.patch(f"{base_import_path}.get_github_repo_api", return_value=Repository()) - - process_github_webhook = ProcessGithubWehook( - {"repository": {"name": Repository().name}}, Headers({"X-GitHub-Event": "test-event"}), logging.getLogger() - ) - process_github_webhook.pull_request_branch = "main" - if hasattr(request, "param") and request.param: - process_github_webhook.changed_files = request.param[0] - else: - process_github_webhook.changed_files = ALL_CHANGED_FILES - - return process_github_webhook - - @pytest.fixture(scope="function") def all_approvers_and_reviewers(process_github_webhook): process_github_webhook.all_approvers_and_reviewers = { @@ -185,25 +102,6 @@ def all_approvers_reviewers(process_github_webhook): process_github_webhook.all_reviewers.sort() -@pytest.mark.parametrize( - "additions, deletions, expected_label", - [ - (0, 0, "XS"), - (18, 1, "XS"), - (48, 1, "S"), - (98, 1, "M"), - (298, 1, "L"), - (498, 1, "XL"), - (1000, 1, "XXL"), - ], -) -def test_get_size_thresholds(process_github_webhook, additions, deletions, expected_label): - process_github_webhook.pull_request = PullRequest(additions=additions, deletions=deletions) - result = process_github_webhook.get_size() - - assert result == f"{SIZE_LABEL_PREFIX}{expected_label}" - - def test_get_all_approvers_and_reviewers(process_github_webhook, all_approvers_and_reviewers): process_github_webhook.repository = Repository() read_owners_result = process_github_webhook.get_all_approvers_and_reviewers() @@ -514,3 +412,52 @@ def test_check_pr_not_approved_folder_with_no_owners_and_folder_without_root_app ] expected_approvers.sort() assert missing_approvers == expected_approvers + + +@pytest.mark.parametrize( + "process_github_webhook", + [ + pytest.param([ + [ + "folder1/sub_folder1/file", + ] + ]) + ], + indirect=True, +) +def test_check_pr_not_approved_subfolder_with_owners(process_github_webhook, all_approvers_and_reviewers): + process_github_webhook.all_approvers = process_github_webhook.get_all_approvers() + check_if_pr_approved = process_github_webhook._check_if_pr_approved( + labels=[ + f"{APPROVED_BY_LABEL_PREFIX}root_approver1", + ] + ) + missing_approvers = [appr.strip() for appr in check_if_pr_approved.split(":")[-1].strip().split(",")] + missing_approvers.sort() + expected_approvers = [ + "folder1_approver1", + "folder1_approver2", + ] + expected_approvers.sort() + assert missing_approvers == expected_approvers + + +@pytest.mark.parametrize( + "process_github_webhook", + [ + pytest.param([ + [ + "folder5/sub_folder5/file", + ] + ]) + ], + indirect=True, +) +def test_check_pr_approved_subfolder_with_owners_no_root_approvers(process_github_webhook, all_approvers_and_reviewers): + process_github_webhook.all_approvers = process_github_webhook.get_all_approvers() + check_if_pr_approved = process_github_webhook._check_if_pr_approved( + labels=[ + f"{APPROVED_BY_LABEL_PREFIX}folder5_approver1", + ] + ) + assert check_if_pr_approved == "" diff --git a/webhook_server_container/tests/test_pull_request_size.py b/webhook_server_container/tests/test_pull_request_size.py new file mode 100644 index 00000000..c0cf8145 --- /dev/null +++ b/webhook_server_container/tests/test_pull_request_size.py @@ -0,0 +1,42 @@ +import pytest + +from webhook_server_container.utils.constants import SIZE_LABEL_PREFIX + + +class Label: + def __init__(self, name: str): + self.name = name + + +class PullRequest: + def __init__(self, additions: int, deletions: int, labels: list[str] | None = None): + self.additions = additions + self.deletions = deletions + self.labels = labels or [] + + @property + def lables(self) -> list[Label]: + _lables = [] + for label in self.labels: + _lables.append(Label(label)) + + return _lables + + +@pytest.mark.parametrize( + "additions, deletions, expected_label", + [ + (0, 0, "XS"), + (18, 1, "XS"), + (48, 1, "S"), + (98, 1, "M"), + (298, 1, "L"), + (498, 1, "XL"), + (1000, 1, "XXL"), + ], +) +def test_get_size_thresholds(process_github_webhook, additions, deletions, expected_label): + process_github_webhook.pull_request = PullRequest(additions=additions, deletions=deletions) + result = process_github_webhook.get_size() + + assert result == f"{SIZE_LABEL_PREFIX}{expected_label}" From 0a680b3692be5451fd8b1ab77002c72106e9bf6c Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Thu, 12 Dec 2024 22:35:57 +0200 Subject: [PATCH 2/3] Address comments --- webhook_server_container/tests/test_pull_request_size.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/webhook_server_container/tests/test_pull_request_size.py b/webhook_server_container/tests/test_pull_request_size.py index c0cf8145..827f5c90 100644 --- a/webhook_server_container/tests/test_pull_request_size.py +++ b/webhook_server_container/tests/test_pull_request_size.py @@ -16,11 +16,7 @@ def __init__(self, additions: int, deletions: int, labels: list[str] | None = No @property def lables(self) -> list[Label]: - _lables = [] - for label in self.labels: - _lables.append(Label(label)) - - return _lables + return [Label(label) for label in self.labels] @pytest.mark.parametrize( From 34325d6c14647ec22c99308fa28a85d793054457 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Thu, 12 Dec 2024 22:41:21 +0200 Subject: [PATCH 3/3] Set coverage to 30% --- pyproject.toml | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index c6d145e8..a94db36d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,8 +1,8 @@ [tool.coverage.run] -omit = [ "tests/*" ] +omit = ["tests/*"] [tool.coverage.report] -fail_under = 0 +fail_under = 30 skip_empty = true [tool.coverage.html] @@ -14,8 +14,8 @@ line-length = 120 fix = true output-format = "grouped" - [tool.ruff.format] - exclude = [ ".git", ".venv", ".mypy_cache", ".tox", "__pycache__" ] +[tool.ruff.format] +exclude = [".git", ".venv", ".mypy_cache", ".tox", "__pycache__"] [tool.mypy] check_untyped_defs = true @@ -27,10 +27,10 @@ show_error_codes = true warn_unused_ignores = true [tool.hatch.build.targets.wheel] -packages = [ "webhook_server_container" ] +packages = ["webhook_server_container"] [tool.uv] -dev-dependencies = [ "ipdb>=0.13.13", "ipython>=8.12.3" ] +dev-dependencies = ["ipdb>=0.13.13", "ipython>=8.12.3"] [project] name = "github-webhook-server" @@ -41,7 +41,7 @@ readme = "README.md" license = "Apache-2.0" classifiers = [ "Programming Language :: Python :: 3", - "Operating System :: OS Independent" + "Operating System :: OS Independent", ] dependencies = [ "build>=1.2.2.post1", @@ -62,23 +62,23 @@ dependencies = [ "string-color>=1.2.3", "timeout-sampler>=0.0.46", "uvicorn>=0.31.0", - "uwsgi>=2.0.27" + "uwsgi>=2.0.27", ] - [[project.authors]] - name = "Meni Yakove" - email = " myakove@gmail.com" +[[project.authors]] +name = "Meni Yakove" +email = " myakove@gmail.com" - [[project.authors]] - name = "Ruth Netser" - email = "ruth.netser@gmail.com" +[[project.authors]] +name = "Ruth Netser" +email = "ruth.netser@gmail.com" - [project.urls] - homepage = "https://github.com/myakove/github-webhook-server" - repository = "https://github.com/myakove/github-webhook-server" - Download = "https://quay.io/repository/myakove/github-webhook-server" - "Bug Tracker" = "https://github.com/myakove/github-webhook-server/issues" +[project.urls] +homepage = "https://github.com/myakove/github-webhook-server" +repository = "https://github.com/myakove/github-webhook-server" +Download = "https://quay.io/repository/myakove/github-webhook-server" +"Bug Tracker" = "https://github.com/myakove/github-webhook-server/issues" [build-system] -requires = [ "hatchling" ] +requires = ["hatchling"] build-backend = "hatchling.build"