diff --git a/webhook_server_container/libs/github_api.py b/webhook_server_container/libs/github_api.py index f22a2150..c2f1c4f2 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,29 +2058,51 @@ 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} - - changed_folder_match: list[str] = [] - require_root_approvers: bool = False + for changed_folder in changed_folders: + changed_folder_str = str(changed_folder) + _owners_data = self._get_owner_data_for_changed_folder(_changed_folder=changed_folder_str) + data[changed_folder_str] = _owners_data - 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) - - if [_folder for _folder in changed_folders if str(_folder) not in changed_folder_match]: - data["."] = self.all_approvers_and_reviewers.get(".", {}) + if not require_root_approvers: + require_root_approvers = _owners_data.get("root-approvers", True) if require_root_approvers: data["."] = self.all_approvers_and_reviewers.get(".", {}) - 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): + return owners_data[_changed_folder] + else: + # 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.""" try: diff --git a/webhook_server_container/tests/test_github_api.py b/webhook_server_container/tests/test_github_api.py index c8fd2234..85b93935 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,8 +222,15 @@ 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": {}, + "folder2": { + "approvers": ["root_approver1", "root_approver2"], + "reviewers": ["root_reviewer1", "root_reviewer2"], + }, "folder/folder4": { "approvers": ["folder4_approver1", "folder4_approver2"], "reviewers": ["folder4_reviewer1", "folder4_reviewer2"], @@ -241,276 +248,249 @@ 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", - ] - ]) + f"{APPROVED_BY_LABEL_PREFIX}root_approver1", + ], + [ + "folder1_approver1", + "folder1_approver2", + "folder4_approver1", + "folder4_approver2", + "folder5_approver1", + "folder5_approver2", + ], + id="test_pr_not_approved_root_approvals", + ), + pytest.param( + [ + 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_pr_not_approved_partial_approved", + ), ], - indirect=True, ) -def test_check_pr_approved_specific_folder_with_root_approvers(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}folder1_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 - -@pytest.mark.parametrize( - "process_github_webhook", - [ - pytest.param([ - [ - "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 == "" + 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_multiple_owners", + ), + pytest.param( + [ + "file_in_root", + "folder2", + "folder/another_file.txt", + ], + [ + f"{APPROVED_BY_LABEL_PREFIX}root_approver1", + ], + id="test_check_pr_root_approval_multiple_folders_no_owners", + ), + pytest.param( + [ + "folder5/file.py", + ], + [ + f"{APPROVED_BY_LABEL_PREFIX}folder5_approver1", + ], + id="test_check_pr_approved_folder_with_owner_no_root_approvers", + ), + pytest.param( [ "folder_with_no_owners/file", - ] - ]) - ], - indirect=True, -) -def test_check_pr_not_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}folder5_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", + ], + id="test_check_pr_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_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_multiple_folder_root_approvers", + ), ], - indirect=True, + indirect=["process_github_webhook"], ) -def test_check_pr_approved_specific_folder_without_owners(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", - ] - ) + 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_requires_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", - ] - ]) - ], - indirect=True, -) -def test_check_pr_approved_folder_with_no_owners_and_folder_without_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}root_approver1", - f"{APPROVED_BY_LABEL_PREFIX}folder5_approver1", - ] - ) - assert check_if_pr_approved == "" - - -@pytest.mark.parametrize( - "process_github_webhook", - [ - pytest.param([ + ], + [ + f"{APPROVED_BY_LABEL_PREFIX}folder5_approver1", + ], + [ + "root_approver1", + "root_approver2", + ], + 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=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_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() - 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) 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