From d36e54a612908cf9cc756f6d49ac8f5f257c9921 Mon Sep 17 00:00:00 2001 From: dbasunag Date: Tue, 10 Dec 2024 18:45:50 -0500 Subject: [PATCH 1/5] for changed file look for owners in parent folder recursively till owner is found --- webhook_server_container/libs/github_api.py | 36 +++++++++++-------- .../tests/test_github_api.py | 1 + 2 files changed, 23 insertions(+), 14 deletions(-) diff --git a/webhook_server_container/libs/github_api.py b/webhook_server_container/libs/github_api.py index f22a2150..8091c638 100644 --- a/webhook_server_container/libs/github_api.py +++ b/webhook_server_container/libs/github_api.py @@ -2038,7 +2038,8 @@ def get_all_approvers(self) -> list[str]: _approvers: list[str] = [] for list_of_approvers in self.owners_data_for_changed_files().values(): for _approver in list_of_approvers.get("approvers", []): - _approvers.append(_approver) + if _approver not in _approvers: + _approvers.append(_approver) _approvers.sort() return _approvers @@ -2046,8 +2047,9 @@ def get_all_approvers(self) -> list[str]: def get_all_reviewers(self) -> list[str]: _reviewers: list[str] = [] for list_of_reviewers in self.owners_data_for_changed_files().values(): - for _approver in list_of_reviewers.get("reviewers", []): - _reviewers.append(_approver) + for _reviewer in list_of_reviewers.get("reviewers", []): + if _reviewer not in _reviewers: + _reviewers.append(_reviewer) _reviewers.sort() return _reviewers @@ -2056,20 +2058,18 @@ def owners_data_for_changed_files(self) -> dict[str, dict[str, Any]]: data: dict[str, dict[str, Any]] = {} changed_folders = {Path(cf).parent for cf in self.changed_files} - + all_owner_data = self.all_approvers_and_reviewers changed_folder_match: list[str] = [] require_root_approvers: bool = False - - for owners_dir, owners_data in self.all_approvers_and_reviewers.items(): - _owners_dir = Path(owners_dir) - - for changed_folder in changed_folders: - if _owners_dir == changed_folder or _owners_dir.parents == changed_folders: - data[owners_dir] = owners_data - changed_folder_match.append(owners_dir) - if not require_root_approvers: - require_root_approvers = owners_data.get("root-approvers", True) + for changed_folder in changed_folders: + changed_folder_str = str(changed_folder) + _owner_dir = self._get_owner_dir_name_for_changed_folder(changed_folder=changed_folder_str) + _owners_data = all_owner_data[_owner_dir] + data[changed_folder_str] = _owners_data + changed_folder_match.append(_owner_dir) + if not require_root_approvers: + require_root_approvers = _owners_data.get("root-approvers", True) if [_folder for _folder in changed_folders if str(_folder) not in changed_folder_match]: data["."] = self.all_approvers_and_reviewers.get(".", {}) @@ -2079,6 +2079,14 @@ def owners_data_for_changed_files(self) -> dict[str, dict[str, Any]]: return data + def _get_owner_dir_name_for_changed_folder(self, changed_folder: str) -> str: + owner_dirs = self.all_approvers_and_reviewers.keys() + if changed_folder in owner_dirs: + return changed_folder + else: + changed_folder = str(Path(changed_folder).parent) + return self._get_owner_dir_name_for_changed_folder(changed_folder=changed_folder) + def _validate_owners_content(self, content: Any, path: str) -> bool: """Validate OWNERS file content structure.""" try: diff --git a/webhook_server_container/tests/test_github_api.py b/webhook_server_container/tests/test_github_api.py index c8fd2234..0cf413dd 100644 --- a/webhook_server_container/tests/test_github_api.py +++ b/webhook_server_container/tests/test_github_api.py @@ -222,6 +222,7 @@ def test_owners_data_for_changed_files(process_github_webhook, all_approvers_and "approvers": ["folder1_approver1", "folder1_approver2"], "reviewers": ["folder1_reviewer1", "folder1_reviewer2"], }, + "code": {"approvers": ["root_approver1", "root_approver2"], "reviewers": ["root_reviewer1", "root_reviewer2"]}, ".": {"approvers": ["root_approver1", "root_approver2"], "reviewers": ["root_reviewer1", "root_reviewer2"]}, "folder2": {}, "folder/folder4": { From 1264751b3a0ebafa284027268ac5ab9bd99ca6ea Mon Sep 17 00:00:00 2001 From: dbasunag Date: Wed, 11 Dec 2024 18:48:07 -0500 Subject: [PATCH 2/5] if ownersfile is not present aggregate all the owners till root and use it --- webhook_server_container/libs/github_api.py | 44 +- .../tests/test_github_api.py | 426 ++++++++---------- 2 files changed, 225 insertions(+), 245 deletions(-) diff --git a/webhook_server_container/libs/github_api.py b/webhook_server_container/libs/github_api.py index 8091c638..75c96549 100644 --- a/webhook_server_container/libs/github_api.py +++ b/webhook_server_container/libs/github_api.py @@ -2058,34 +2058,46 @@ def owners_data_for_changed_files(self) -> dict[str, dict[str, Any]]: data: dict[str, dict[str, Any]] = {} changed_folders = {Path(cf).parent for cf in self.changed_files} - all_owner_data = self.all_approvers_and_reviewers - changed_folder_match: list[str] = [] - require_root_approvers: bool = False for changed_folder in changed_folders: changed_folder_str = str(changed_folder) - _owner_dir = self._get_owner_dir_name_for_changed_folder(changed_folder=changed_folder_str) - _owners_data = all_owner_data[_owner_dir] + _owners_data = self._get_owner_data_for_changed_folder(_changed_folder=changed_folder_str) data[changed_folder_str] = _owners_data - changed_folder_match.append(_owner_dir) + if not require_root_approvers: require_root_approvers = _owners_data.get("root-approvers", True) - if [_folder for _folder in changed_folders if str(_folder) not in changed_folder_match]: - data["."] = self.all_approvers_and_reviewers.get(".", {}) - if require_root_approvers: data["."] = self.all_approvers_and_reviewers.get(".", {}) - return data - def _get_owner_dir_name_for_changed_folder(self, changed_folder: str) -> str: - owner_dirs = self.all_approvers_and_reviewers.keys() - if changed_folder in owner_dirs: - return changed_folder + def _get_owner_data_for_changed_folder(self, _changed_folder: str) -> dict[str, dict[str, Any | bool]]: + _aggregated_owners: dict[str, dict[str, Any | bool]] = { + "approvers": [], + "reviewers": [], + "root-approvers": True, + } + owners_data = self.all_approvers_and_reviewers + if owners_data.get(_changed_folder): + return owners_data[_changed_folder] else: - changed_folder = str(Path(changed_folder).parent) - return self._get_owner_dir_name_for_changed_folder(changed_folder=changed_folder) + # find all owners files till root and combine them all + while _changed_folder != ".": + _changed_folder = str(Path(_changed_folder).parent) or "." + if owners_data.get(_changed_folder): + _aggregated_owners["approvers"].extend([ + _approver + for _approver in owners_data[_changed_folder].get("approvers", []) + if _approver not in _aggregated_owners["approvers"] + ]) + _aggregated_owners["reviewers"].extend([ + _reviewer + for _reviewer in owners_data[_changed_folder].get("reviewers", []) + if _reviewer not in _aggregated_owners["reviewers"] + ]) + if _changed_folder != ".": + _aggregated_owners["root-approvers"] = owners_data[_changed_folder].get("root-approvers", True) + return _aggregated_owners def _validate_owners_content(self, content: Any, path: str) -> bool: """Validate OWNERS file content structure.""" diff --git a/webhook_server_container/tests/test_github_api.py b/webhook_server_container/tests/test_github_api.py index 0cf413dd..848ee6fb 100644 --- a/webhook_server_container/tests/test_github_api.py +++ b/webhook_server_container/tests/test_github_api.py @@ -128,10 +128,10 @@ def process_github_webhook(mocker, request): ) process_github_webhook.pull_request_branch = "main" if hasattr(request, "param") and request.param: - process_github_webhook.changed_files = request.param[0] + process_github_webhook.changed_files = request.param else: process_github_webhook.changed_files = ALL_CHANGED_FILES - + print(f"changed files: {process_github_webhook.changed_files}") return process_github_webhook @@ -222,9 +222,17 @@ def test_owners_data_for_changed_files(process_github_webhook, all_approvers_and "approvers": ["folder1_approver1", "folder1_approver2"], "reviewers": ["folder1_reviewer1", "folder1_reviewer2"], }, - "code": {"approvers": ["root_approver1", "root_approver2"], "reviewers": ["root_reviewer1", "root_reviewer2"]}, + "code": { + "approvers": ["root_approver1", "root_approver2"], + "reviewers": ["root_reviewer1", "root_reviewer2"], + "root-approvers": True, + }, ".": {"approvers": ["root_approver1", "root_approver2"], "reviewers": ["root_reviewer1", "root_reviewer2"]}, - "folder2": {}, + "folder2": { + "approvers": ["root_approver1", "root_approver2"], + "reviewers": ["root_reviewer1", "root_reviewer2"], + "root-approvers": True, + }, "folder/folder4": { "approvers": ["folder4_approver1", "folder4_approver2"], "reviewers": ["folder4_reviewer1", "folder4_reviewer2"], @@ -242,276 +250,236 @@ def test_all_approvers_reviewers(process_github_webhook, all_approvers_and_revie assert all_reviewers == process_github_webhook.all_reviewers -def test_check_pr_approved(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", - f"{APPROVED_BY_LABEL_PREFIX}root_approver2", - f"{APPROVED_BY_LABEL_PREFIX}folder1_approver1", - f"{APPROVED_BY_LABEL_PREFIX}folder1_approver2", - f"{APPROVED_BY_LABEL_PREFIX}folder4_approver1", - f"{APPROVED_BY_LABEL_PREFIX}folder4_approver2", - f"{APPROVED_BY_LABEL_PREFIX}folder5_approver1", - f"{APPROVED_BY_LABEL_PREFIX}folder5_approver2", - ] - ) - assert check_if_pr_approved == "" - - -def test_check_pr_minimum_approved(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", - f"{APPROVED_BY_LABEL_PREFIX}folder1_approver1", - f"{APPROVED_BY_LABEL_PREFIX}folder4_approver1", - f"{APPROVED_BY_LABEL_PREFIX}folder5_approver1", - ] - ) - assert check_if_pr_approved == "" - - -def test_check_pr_not_approved(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", - "folder4_approver1", - "folder4_approver2", - "folder5_approver1", - "folder5_approver2", - ] - expected_approvers.sort() - assert missing_approvers == expected_approvers - - -def test_check_pr_partial_approved(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", - f"{APPROVED_BY_LABEL_PREFIX}root_approver2", - ] - ) - missing_approvers = [appr.strip() for appr in check_if_pr_approved.split(":")[-1].strip().split(",")] - missing_approvers.sort() - expected_approvers = [ - "folder1_approver1", - "folder1_approver2", - "folder4_approver1", - "folder4_approver2", - "folder5_approver1", - "folder5_approver2", - ] - expected_approvers.sort() - assert missing_approvers == expected_approvers - - @pytest.mark.parametrize( - "process_github_webhook", + "approval_labels", [ - pytest.param([ + pytest.param( [ - "file_in_root", - "folder2", - "folder/folder4/another_file.txt", - ] - ]) - ], - indirect=True, -) -def test_check_pr_approved_specific_folder(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", - f"{APPROVED_BY_LABEL_PREFIX}folder4_approver1", - ] - ) - assert check_if_pr_approved == "" - - -@pytest.mark.parametrize( - "process_github_webhook", - [ - pytest.param([ + f"{APPROVED_BY_LABEL_PREFIX}root_approver1", + f"{APPROVED_BY_LABEL_PREFIX}root_approver2", + f"{APPROVED_BY_LABEL_PREFIX}folder1_approver1", + f"{APPROVED_BY_LABEL_PREFIX}folder1_approver2", + f"{APPROVED_BY_LABEL_PREFIX}folder4_approver1", + f"{APPROVED_BY_LABEL_PREFIX}folder4_approver2", + f"{APPROVED_BY_LABEL_PREFIX}folder5_approver1", + f"{APPROVED_BY_LABEL_PREFIX}folder5_approver2", + ], + id="test_pr_approved_all_approvals", + ), + pytest.param( [ - "file_in_root", - "folder2", - "folder/another_file.txt", - ] - ]) + f"{APPROVED_BY_LABEL_PREFIX}root_approver1", + f"{APPROVED_BY_LABEL_PREFIX}folder1_approver1", + f"{APPROVED_BY_LABEL_PREFIX}folder4_approver1", + f"{APPROVED_BY_LABEL_PREFIX}folder5_approver1", + ], + id="test_pr_approved_minimum_approvals", + ), ], - indirect=True, ) -def test_check_pr_approved_nested_folder_no_owners(process_github_webhook, all_approvers_and_reviewers): +def test_check_pr_approved(process_github_webhook, all_approvers_and_reviewers, approval_labels): 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", - ] - ) + check_if_pr_approved = process_github_webhook._check_if_pr_approved(labels=approval_labels) assert check_if_pr_approved == "" @pytest.mark.parametrize( - "process_github_webhook", + "approval_labels, expected_approval_labels", [ - pytest.param([ + pytest.param( [ - "folder1/file", - ] - ]) - ], - indirect=True, -) -def test_check_pr_approved_specific_folder_with_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}folder1_approver1", - ] - ) - missing_approvers = [appr.strip() for appr in check_if_pr_approved.split(":")[-1].strip().split(",")] - missing_approvers.sort() - expected_approvers = [ - "root_approver1", - "root_approver2", - ] - expected_approvers.sort() - assert missing_approvers == expected_approvers - - -@pytest.mark.parametrize( - "process_github_webhook", - [ - pytest.param([ + f"{APPROVED_BY_LABEL_PREFIX}root_approver1", + ], [ - "folder5/file", - ] - ]) - ], - indirect=True, -) -def test_check_pr_approved_specific_folder_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 == "" - - -@pytest.mark.parametrize( - "process_github_webhook", - [ - pytest.param([ + "folder1_approver1", + "folder1_approver2", + "folder4_approver1", + "folder4_approver2", + "folder5_approver1", + "folder5_approver2", + ], + id="test_pr_approved_root_approvals", + ), + pytest.param( [ - "folder_with_no_owners/file", - ] - ]) + f"{APPROVED_BY_LABEL_PREFIX}root_approver1", + f"{APPROVED_BY_LABEL_PREFIX}root_approver2", + ], + [ + "folder1_approver1", + "folder1_approver2", + "folder4_approver1", + "folder4_approver2", + "folder5_approver1", + "folder5_approver2", + ], + id="test_check_pr_partial_approved", + ), ], - indirect=True, ) -def test_check_pr_not_approved_specific_folder_without_owners(process_github_webhook, all_approvers_and_reviewers): +def test_check_pr_not_approved( + process_github_webhook, all_approvers_and_reviewers, approval_labels, expected_approval_labels +): 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", - ] - ) + check_if_pr_approved = process_github_webhook._check_if_pr_approved(labels=approval_labels) missing_approvers = [appr.strip() for appr in check_if_pr_approved.split(":")[-1].strip().split(",")] missing_approvers.sort() - expected_approvers = [ - "root_approver1", - "root_approver2", - ] - expected_approvers.sort() - assert missing_approvers == expected_approvers + + expected_approval_labels.sort() + assert missing_approvers == expected_approval_labels @pytest.mark.parametrize( - "process_github_webhook", + "process_github_webhook, approved_labels", [ - pytest.param([ + pytest.param( + [ + "file_in_root", + "folder2", + "folder/folder4/another_file.txt", + ], + [ + f"{APPROVED_BY_LABEL_PREFIX}root_approver1", + f"{APPROVED_BY_LABEL_PREFIX}folder4_approver1", + ], + id="test_check_pr_approved_specific_folder", + ), + pytest.param( + [ + "file_in_root", + "folder2", + "folder/another_file.txt", + ], + [ + f"{APPROVED_BY_LABEL_PREFIX}root_approver1", + ], + id="test_check_pr_approved_nested_folder_no_owners", + ), + pytest.param( + [ + "folder5/file.py", + ], + [ + f"{APPROVED_BY_LABEL_PREFIX}folder5_approver1", + ], + id="test_check_pr_approved_specific_folder_no_root_approvers", + ), + pytest.param( [ "folder_with_no_owners/file", - ] - ]) - ], - indirect=True, -) -def test_check_pr_approved_specific_folder_without_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", - ] - ) - assert check_if_pr_approved == "" - - -@pytest.mark.parametrize( - "process_github_webhook", - [ - pytest.param([ + ], + [ + f"{APPROVED_BY_LABEL_PREFIX}root_approver1", + ], + id="test_check_pr_approved_specific_folder_without_owners", + ), + pytest.param( [ "folder_with_no_owners/file", "folder5/file", - ] - ]) + ], + [ + f"{APPROVED_BY_LABEL_PREFIX}root_approver1", + f"{APPROVED_BY_LABEL_PREFIX}folder5_approver1", + ], + id="test_check_pr_approved_folder_with_no_owners_and_folder_without_root_approvers", + ), + pytest.param( + [ + "folder5/test_folder0/test_folder1/test_folder2/file1.txt", + ], + [ + f"{APPROVED_BY_LABEL_PREFIX}folder5_approver1", + ], + id="test_check_pr_approved_deep_nested_folder_specific_approvers", + ), + pytest.param( + [ + "folder5/test_folder0/test_folder1/test_folder2/file1.txt", + ], + [ + f"{APPROVED_BY_LABEL_PREFIX}root_approver1", + ], + id="test_check_pr_approved_deep_nested_folder_root_approvers", + ), + pytest.param( + [ + "folder/folder4/test_folder1/test_folder2/file1.txt", + ], + [ + f"{APPROVED_BY_LABEL_PREFIX}root_approver1", + ], + id="test_check_pr_deep_nested_folder_specific_approvers", + ), + pytest.param( + ["folder/folder4/test_folder1/test_folder2/file1.txt", "folder3/test.py"], + [ + f"{APPROVED_BY_LABEL_PREFIX}root_approver2", + ], + id="test_check_pr_with_deep_nested_folder_root_approvers", + ), ], - indirect=True, + indirect=["process_github_webhook"], ) -def test_check_pr_approved_folder_with_no_owners_and_folder_without_root_approvers( - process_github_webhook, all_approvers_and_reviewers -): +def test_check_pr_approved_with_folders(process_github_webhook, all_approvers_and_reviewers, approved_labels): 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", - f"{APPROVED_BY_LABEL_PREFIX}folder5_approver1", - ] - ) + check_if_pr_approved = process_github_webhook._check_if_pr_approved(labels=approved_labels) assert check_if_pr_approved == "" @pytest.mark.parametrize( - "process_github_webhook", + "process_github_webhook, approval_label, expected_approvers", [ - pytest.param([ + pytest.param( + [ + "folder1/file", + ], + [ + f"{APPROVED_BY_LABEL_PREFIX}folder1_approver1", + ], + [ + "root_approver1", + "root_approver2", + ], + id="test_check_pr_approved_specific_folder_with_root_approvers", + ), + pytest.param( + [ + "folder_with_no_owners/file", + ], + [ + f"{APPROVED_BY_LABEL_PREFIX}folder5_approver1", + ], + [ + "root_approver1", + "root_approver2", + ], + id="test_check_pr_not_approved_specific_folder_without_owners", + ), + pytest.param( [ "folder_with_no_owners/file", "folder5/file", - ] - ]) + ], + [ + f"{APPROVED_BY_LABEL_PREFIX}folder5_approver1", + ], + [ + "root_approver1", + "root_approver2", + ], + id="test_check_pr_not_approved_folder_with_no_owners_and_folder_without_root_approvers", + ), ], - indirect=True, + indirect=["process_github_webhook"], ) -def test_check_pr_not_approved_folder_with_no_owners_and_folder_without_root_approvers( - process_github_webhook, all_approvers_and_reviewers +def test_check_pr_approved_specific_folder_negative( + process_github_webhook, all_approvers_and_reviewers, approval_label, expected_approvers ): 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", - ] - ) + check_if_pr_approved = process_github_webhook._check_if_pr_approved(labels=approval_label) + print(check_if_pr_approved) missing_approvers = [appr.strip() for appr in check_if_pr_approved.split(":")[-1].strip().split(",")] missing_approvers.sort() - expected_approvers = [ - "root_approver1", - "root_approver2", - ] expected_approvers.sort() assert missing_approvers == expected_approvers From 9698a50396cb9a3893ba3982babe8f5ebd90126a Mon Sep 17 00:00:00 2001 From: dbasunag Date: Wed, 11 Dec 2024 18:58:03 -0500 Subject: [PATCH 3/5] trying to resolve type annotation errors --- webhook_server_container/libs/github_api.py | 13 ++++++------- webhook_server_container/tests/test_github_api.py | 1 - 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/webhook_server_container/libs/github_api.py b/webhook_server_container/libs/github_api.py index 75c96549..eec05421 100644 --- a/webhook_server_container/libs/github_api.py +++ b/webhook_server_container/libs/github_api.py @@ -10,7 +10,7 @@ import re import time from concurrent.futures import Future, ThreadPoolExecutor, as_completed -from typing import Any, Callable, Dict, Generator, List, Optional, Set, Tuple +from typing import Any, Callable, Dict, Generator, List, Optional, Set, Tuple, Union from github.CheckRun import CheckRun from stringcolor import cs @@ -2071,12 +2071,11 @@ def owners_data_for_changed_files(self) -> dict[str, dict[str, Any]]: data["."] = self.all_approvers_and_reviewers.get(".", {}) return data - def _get_owner_data_for_changed_folder(self, _changed_folder: str) -> dict[str, dict[str, Any | bool]]: - _aggregated_owners: dict[str, dict[str, Any | bool]] = { - "approvers": [], - "reviewers": [], - "root-approvers": True, - } + def _get_owner_data_for_changed_folder(self, _changed_folder: str) -> dict[str, Union[List[str], bool]]: + _aggregated_owners: dict[str, Union[List[str], bool]] = {} + _aggregated_owners["approvers"] = [] + _aggregated_owners["reviewers"] = [] + _aggregated_owners["root-approvers"] = True owners_data = self.all_approvers_and_reviewers if owners_data.get(_changed_folder): return owners_data[_changed_folder] diff --git a/webhook_server_container/tests/test_github_api.py b/webhook_server_container/tests/test_github_api.py index 848ee6fb..1489166f 100644 --- a/webhook_server_container/tests/test_github_api.py +++ b/webhook_server_container/tests/test_github_api.py @@ -478,7 +478,6 @@ def test_check_pr_approved_specific_folder_negative( ): process_github_webhook.all_approvers = process_github_webhook.get_all_approvers() check_if_pr_approved = process_github_webhook._check_if_pr_approved(labels=approval_label) - print(check_if_pr_approved) missing_approvers = [appr.strip() for appr in check_if_pr_approved.split(":")[-1].strip().split(",")] missing_approvers.sort() expected_approvers.sort() From e46d52dc844ba00772cedd5d55f3bb98b5505b37 Mon Sep 17 00:00:00 2001 From: dbasunag Date: Wed, 11 Dec 2024 19:05:08 -0500 Subject: [PATCH 4/5] fix typing errors --- webhook_server_container/libs/github_api.py | 9 +++------ webhook_server_container/tests/test_github_api.py | 2 -- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/webhook_server_container/libs/github_api.py b/webhook_server_container/libs/github_api.py index eec05421..a0f57f81 100644 --- a/webhook_server_container/libs/github_api.py +++ b/webhook_server_container/libs/github_api.py @@ -10,7 +10,7 @@ import re import time from concurrent.futures import Future, ThreadPoolExecutor, as_completed -from typing import Any, Callable, Dict, Generator, List, Optional, Set, Tuple, Union +from typing import Any, Callable, Dict, Generator, List, Optional, Set, Tuple from github.CheckRun import CheckRun from stringcolor import cs @@ -2071,11 +2071,8 @@ def owners_data_for_changed_files(self) -> dict[str, dict[str, Any]]: data["."] = self.all_approvers_and_reviewers.get(".", {}) return data - def _get_owner_data_for_changed_folder(self, _changed_folder: str) -> dict[str, Union[List[str], bool]]: - _aggregated_owners: dict[str, Union[List[str], bool]] = {} - _aggregated_owners["approvers"] = [] - _aggregated_owners["reviewers"] = [] - _aggregated_owners["root-approvers"] = True + def _get_owner_data_for_changed_folder(self, _changed_folder: str) -> dict[str, Any]: + _aggregated_owners: dict[str, Any] = {"approvers": [], "reviewers": []} owners_data = self.all_approvers_and_reviewers if owners_data.get(_changed_folder): return owners_data[_changed_folder] diff --git a/webhook_server_container/tests/test_github_api.py b/webhook_server_container/tests/test_github_api.py index 1489166f..01331f8d 100644 --- a/webhook_server_container/tests/test_github_api.py +++ b/webhook_server_container/tests/test_github_api.py @@ -225,13 +225,11 @@ def test_owners_data_for_changed_files(process_github_webhook, all_approvers_and "code": { "approvers": ["root_approver1", "root_approver2"], "reviewers": ["root_reviewer1", "root_reviewer2"], - "root-approvers": True, }, ".": {"approvers": ["root_approver1", "root_approver2"], "reviewers": ["root_reviewer1", "root_reviewer2"]}, "folder2": { "approvers": ["root_approver1", "root_approver2"], "reviewers": ["root_reviewer1", "root_reviewer2"], - "root-approvers": True, }, "folder/folder4": { "approvers": ["folder4_approver1", "folder4_approver2"], From 8406cc3498fa44c4db6a0fe1d36146523f38929d Mon Sep 17 00:00:00 2001 From: dbasunag Date: Wed, 11 Dec 2024 20:08:30 -0500 Subject: [PATCH 5/5] minor changes --- webhook_server_container/libs/github_api.py | 8 +++++ .../tests/test_github_api.py | 36 +++++++++++++------ 2 files changed, 33 insertions(+), 11 deletions(-) diff --git a/webhook_server_container/libs/github_api.py b/webhook_server_container/libs/github_api.py index a0f57f81..c2f1c4f2 100644 --- a/webhook_server_container/libs/github_api.py +++ b/webhook_server_container/libs/github_api.py @@ -2072,6 +2072,14 @@ def owners_data_for_changed_files(self) -> dict[str, dict[str, Any]]: return data def _get_owner_data_for_changed_folder(self, _changed_folder: str) -> dict[str, Any]: + # if we find entry for the changed folder in self.all_approvers_and_reviewers, then we return it + # else we start from the parent and go all the way to root and combine all approvers and owners found + # return the same. "root-approvers" would be copied from the owner file found below root + # Example: if a file file1.txt is changed, that is found under + # test_folder0/test_folder_1/test_folder2/test_folder3/, reviewers and + # approvers from test_folder3, test_folder2, test_folder1, test_folder0 and root all would be combined + # to get approvers and reviewers of file1.txt + _aggregated_owners: dict[str, Any] = {"approvers": [], "reviewers": []} owners_data = self.all_approvers_and_reviewers if owners_data.get(_changed_folder): diff --git a/webhook_server_container/tests/test_github_api.py b/webhook_server_container/tests/test_github_api.py index 01331f8d..85b93935 100644 --- a/webhook_server_container/tests/test_github_api.py +++ b/webhook_server_container/tests/test_github_api.py @@ -296,7 +296,7 @@ def test_check_pr_approved(process_github_webhook, all_approvers_and_reviewers, "folder5_approver1", "folder5_approver2", ], - id="test_pr_approved_root_approvals", + id="test_pr_not_approved_root_approvals", ), pytest.param( [ @@ -311,7 +311,7 @@ def test_check_pr_approved(process_github_webhook, all_approvers_and_reviewers, "folder5_approver1", "folder5_approver2", ], - id="test_check_pr_partial_approved", + id="test_pr_not_approved_partial_approved", ), ], ) @@ -340,7 +340,7 @@ def test_check_pr_not_approved( f"{APPROVED_BY_LABEL_PREFIX}root_approver1", f"{APPROVED_BY_LABEL_PREFIX}folder4_approver1", ], - id="test_check_pr_approved_specific_folder", + id="test_check_pr_approved_multiple_owners", ), pytest.param( [ @@ -351,7 +351,7 @@ def test_check_pr_not_approved( [ f"{APPROVED_BY_LABEL_PREFIX}root_approver1", ], - id="test_check_pr_approved_nested_folder_no_owners", + id="test_check_pr_root_approval_multiple_folders_no_owners", ), pytest.param( [ @@ -360,7 +360,7 @@ def test_check_pr_not_approved( [ f"{APPROVED_BY_LABEL_PREFIX}folder5_approver1", ], - id="test_check_pr_approved_specific_folder_no_root_approvers", + id="test_check_pr_approved_folder_with_owner_no_root_approvers", ), pytest.param( [ @@ -369,7 +369,7 @@ def test_check_pr_not_approved( [ f"{APPROVED_BY_LABEL_PREFIX}root_approver1", ], - id="test_check_pr_approved_specific_folder_without_owners", + id="test_check_pr_specific_folder_without_owners", ), pytest.param( [ @@ -407,14 +407,14 @@ def test_check_pr_not_approved( [ f"{APPROVED_BY_LABEL_PREFIX}root_approver1", ], - id="test_check_pr_deep_nested_folder_specific_approvers", + id="test_check_pr_deep_nested_folder_withowners_requires_root_approvers", ), pytest.param( ["folder/folder4/test_folder1/test_folder2/file1.txt", "folder3/test.py"], [ f"{APPROVED_BY_LABEL_PREFIX}root_approver2", ], - id="test_check_pr_with_deep_nested_folder_root_approvers", + id="test_check_pr_with_multiple_folder_root_approvers", ), ], indirect=["process_github_webhook"], @@ -439,7 +439,7 @@ def test_check_pr_approved_with_folders(process_github_webhook, all_approvers_an "root_approver1", "root_approver2", ], - id="test_check_pr_approved_specific_folder_with_root_approvers", + id="test_check_pr_approved_specific_folder_requires_root_approvers", ), pytest.param( [ @@ -466,12 +466,26 @@ def test_check_pr_approved_with_folders(process_github_webhook, all_approvers_an "root_approver1", "root_approver2", ], - id="test_check_pr_not_approved_folder_with_no_owners_and_folder_without_root_approvers", + id="test_check_pr_not_approved_folder_with_no_owners_and_folder_with_root_approvers", + ), + pytest.param( + [ + "folder_with_no_owners/file", + "folder5/file", + ], + [ + f"{APPROVED_BY_LABEL_PREFIX}root_approver1", + ], + [ + "folder5_approver1", + "folder5_approver2", + ], + id="test_check_pr_not_approved_folder_with_no_owners_and_folder_approvers", ), ], indirect=["process_github_webhook"], ) -def test_check_pr_approved_specific_folder_negative( +def test_check_pr_not_approved_specific_folder_negative( process_github_webhook, all_approvers_and_reviewers, approval_label, expected_approvers ): process_github_webhook.all_approvers = process_github_webhook.get_all_approvers()