From b7893149603a5a5ca0006b60144fcc0093a9991c Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Mon, 25 Nov 2024 23:49:35 +0200 Subject: [PATCH 1/5] check only parent path for owners file, make sure root owners are always approvers --- webhook_server_container/libs/github_api.py | 80 ++--- .../tests/test_github_api.py | 300 +++++++++++++++--- 2 files changed, 286 insertions(+), 94 deletions(-) diff --git a/webhook_server_container/libs/github_api.py b/webhook_server_container/libs/github_api.py index 8419e580..2f0ba7c6 100644 --- a/webhook_server_container/libs/github_api.py +++ b/webhook_server_container/libs/github_api.py @@ -15,7 +15,6 @@ from stringcolor import cs from github.Branch import Branch -from github.ContentFile import ContentFile import requests import shortuuid from starlette.datastructures import Headers @@ -203,7 +202,7 @@ def process(self) -> None: self.last_committer = getattr(self.last_commit.committer, "login", self.parent_committer) self.changed_files = self.list_changed_commit_files() self.pull_request_branch = self.pull_request.base.ref - self.approvers_and_reviewers = self.get_approvers_and_reviewers() + self.all_approvers_and_reviewers = self.get_all_approvers_and_reviewers() self.all_approvers = self.get_all_approvers() self.all_reviewers = self.get_all_reviewers() @@ -581,43 +580,15 @@ def _error(_out: str, _err: str) -> None: """ self.send_slack_message(message=message, webhook_url=self.slack_webhook_url) - def get_owners_content(self, folder_path: str = "") -> Dict[str, Any]: - if folder_path: - # Normalize path and check for directory traversal - norm_path = os.path.normpath(folder_path) - if ( - norm_path.startswith("/") - or norm_path.startswith("\\") - or ".." in norm_path - or not all(part.isalnum() or part in "-_" for part in norm_path.split(os.path.sep)) - ): - self.logger.error(f"{self.log_prefix} Invalid folder path: {folder_path}") - return {} - - try: - owners_path = f"{folder_path}/OWNERS" if folder_path else "OWNERS" - owners_content: list[ContentFile] | ContentFile = self.repository.get_contents(owners_path) - if isinstance(owners_content, list): - self.logger.debug(f"{self.log_prefix} Found more than one OWNERS file, using the first one") - owners_content = owners_content[0] - - _content: Dict[str, Any] = yaml.safe_load(owners_content.decoded_content) - self.logger.debug(f"{self.log_prefix} OWNERS file content: {_content}") - return _content - - except UnknownObjectException: - self.logger.error(f"{self.log_prefix} OWNERS file not found") - return {} - @property def root_reviewers(self) -> List[str]: - _reviewers = self.approvers_and_reviewers.get(".", {}).get("reviewers", []) + _reviewers = self.all_approvers_and_reviewers.get(".", {}).get("reviewers", []) self.logger.debug(f"{self.log_prefix} ROOT Reviewers: {_reviewers}") return _reviewers @property def root_approvers(self) -> List[str]: - _approvers = self.approvers_and_reviewers.get(".", {}).get("approvers", []) + _approvers = self.all_approvers_and_reviewers.get(".", {}).get("approvers", []) self.logger.debug(f"{self.log_prefix} ROOT Approvers: {_approvers}") return _approvers @@ -2027,7 +1998,7 @@ def run_podman_command(self, command: str, pipe: bool = False) -> Tuple[bool, st return rc, out, err - def get_approvers_and_reviewers(self) -> dict[str, dict[str, list[str]]]: + def get_all_approvers_and_reviewers(self) -> dict[str, dict[str, list[str]]]: # Dictionary mapping OWNERS file paths to their approvers and reviewers _owners: dict[str, dict[str, list[str]]] = {} @@ -2050,7 +2021,6 @@ def get_approvers_and_reviewers(self) -> dict[str, dict[str, list[str]]]: try: content = yaml.safe_load(_path.decoded_content) if self._validate_owners_content(content, content_path): - # Use Path for consistent path handling parent_path = str(Path(content_path).parent) if not parent_path: parent_path = "." @@ -2069,9 +2039,9 @@ def get_all_approvers(self) -> list[str]: for _approver in list_of_approvers: _approvers.append(_approver) - reviewers = list(set(self.root_approvers + _approvers)) - reviewers.sort() - return reviewers + approvers = list(set(self.root_approvers + _approvers)) + approvers.sort() + return approvers def get_all_reviewers(self) -> list[str]: _reviewers: list[str] = [] @@ -2079,9 +2049,9 @@ def get_all_reviewers(self) -> list[str]: for _approver in list_of_reviewers: _reviewers.append(_approver) - approvers = list(set(self.root_reviewers + _reviewers)) - approvers.sort() - return approvers + reviewers = list(set(self.root_reviewers + _reviewers)) + reviewers.sort() + return reviewers def owners_data_for_changed_files(self) -> dict[str, list[list[str]]]: data: dict[str, list[list[str]]] = {"approvers": [], "reviewers": []} @@ -2089,10 +2059,10 @@ def owners_data_for_changed_files(self) -> dict[str, list[list[str]]]: changed_folders = {Path(cf).parent for cf in self.changed_files} for changed_folder_path in changed_folders: - for owners_dir, owners_data in self.approvers_and_reviewers.items(): + for owners_dir, owners_data in self.all_approvers_and_reviewers.items(): _owners_dir = Path(owners_dir) - if _owners_dir == changed_folder_path or _owners_dir in changed_folder_path.parents: + if _owners_dir == changed_folder_path or _owners_dir == changed_folder_path.parents: _reviewers = owners_data.get("reviewers", []) self.logger.debug(f"{self.log_prefix} Found reviewers for {owners_dir}: {_reviewers}") data["reviewers"].append(_reviewers) @@ -2202,12 +2172,14 @@ def _check_lables_for_can_be_merged(self, labels: list[str]) -> str: def _check_if_pr_approved(self, labels: list[str]) -> str: _pr_approvers: list[str] = [] - all_needed_approvers = [] - for approvers_list in self.owners_data_for_changed_files()["approvers"]: - if approvers_list not in all_needed_approvers: - all_needed_approvers.append(approvers_list) + # all_needed_approvers = [] + required_pr_approvers = self.owners_data_for_changed_files()["approvers"] + + root_approvers = self.all_approvers_and_reviewers.get(".", {}).get("approvers") + if root_approvers: + required_pr_approvers.append(root_approvers) - # all_needed_approvers is [['approver1', 'approver2'], ['approver3', 'approver4']] + # required_pr_approvers is [['approver1', 'approver2'], ['approver3', 'approver4']] # To mark PR as approved we need at least one lgtm/approved from each nested list inside all_needed_approvers approved_by = [] for _label in labels: @@ -2218,15 +2190,19 @@ def _check_if_pr_approved(self, labels: list[str]) -> str: approved_by.append(approved_user) - missing_approvers = self.all_approvers.copy() + missing_approvers: list[str] = [] + for list_of_reviewers in required_pr_approvers: + for _approver in list_of_reviewers: + missing_approvers.append(_approver) + + missing_approvers = list(set(missing_approvers)) - for owners_data in self.approvers_and_reviewers.values(): - _approvers = owners_data.get("approvers", []) - for approver in _approvers: + for approvers in required_pr_approvers: + for approver in approvers: if approver in approved_by: _pr_approvers.append(approver) # Once we found approver in approved_by list, we remove all approvers from missing_approvers list for this owners file - {missing_approvers.remove(_approver) for _approver in _approvers if _approver in missing_approvers} # type: ignore + {missing_approvers.remove(_approver) for _approver in approvers if _approver in missing_approvers} # type: ignore break if missing_approvers: diff --git a/webhook_server_container/tests/test_github_api.py b/webhook_server_container/tests/test_github_api.py index 392f9c4d..4165d0c2 100644 --- a/webhook_server_container/tests/test_github_api.py +++ b/webhook_server_container/tests/test_github_api.py @@ -7,6 +7,16 @@ 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): @@ -21,7 +31,15 @@ def __init__(self, path: str): @property def tree(self): trees = [] - for _path in ["OWNERS", "test1/OWNERS", "code/file.py", "README.md"]: + for _path in [ + "OWNERS", + "folder1/OWNERS", + "code/file.py", + "README.md", + "folder2/OWNERS", + "folder/folder4/OWNERS", + "folder5/OWNERS", + ]: trees.append(Tree(_path)) return trees @@ -43,17 +61,43 @@ def get_git_tree(self, sha: str, recursive: bool): return Tree("") def get_contents(self, path: str): - owners_data = yaml.dump({"approvers": ["approver1", "approver2"], "reviewers": ["reviewer1", "reviewer2"]}) + 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"], + }) - test1_owners_data = yaml.dump({ - "approvers": ["approver3", "approver4"], - "reviewers": ["reviewer3", "reviewer4"], + 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 == "test1/OWNERS": - return ContentFile(test1_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) class PullRequest: @@ -72,7 +116,7 @@ def lables(self) -> list[Label]: @pytest.fixture(scope="function") -def process_github_webhook(mocker): +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" @@ -85,22 +129,62 @@ def process_github_webhook(mocker): {"repository": {"name": Repository().name}}, Headers({"X-GitHub-Event": "test-event"}), logging.getLogger() ) process_github_webhook.pull_request_branch = "main" - process_github_webhook.changed_files = ["OWNERS", "test1/OWNERS", "code/file.py", "README.md"] + 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 approvers_and_reviewers(process_github_webhook): - process_github_webhook.approvers_and_reviewers = { - ".": {"approvers": ["approver1", "approver2"], "reviewers": ["reviewer1", "reviewer2"]}, - "test1": {"approvers": ["approver3", "approver4"], "reviewers": ["reviewer3", "reviewer4"]}, + process_github_webhook.all_approvers_and_reviewers = { + ".": {"approvers": ["root_approver1", "root_approver2"], "reviewers": ["root_reviewer1", "root_reviewer2"]}, + "folder1": { + "approvers": ["folder1_approver1", "folder1_approver2"], + "reviewers": ["folder1_reviewer1", "folder1_reviewer2"], + }, + "folder2": {}, + "folder/folder4": { + "approvers": ["folder4_approver1", "folder4_approver2"], + "reviewers": ["folder4_reviewer1", "folder4_reviewer2"], + }, + "folder5": { + "approvers": ["folder5_approver1", "folder5_approver2"], + "reviewers": ["folder5_reviewer1", "folder5_reviewer2"], + "root-approvers": False, + }, } @pytest.fixture(scope="function") def all_approvers_reviewers(process_github_webhook): - process_github_webhook.all_approvers = ["approver1", "approver2", "approver3", "approver4"] - process_github_webhook.all_reviewers = ["reviewer1", "reviewer2", "reviewer3", "reviewer4"] + process_github_webhook.all_approvers = [ + "folder1_approver1", + "folder1_approver2", + "folder4_approver1", + "folder4_approver2", + "folder5_approver1", + "folder5_approver2", + "root_approver1", + "root_approver2", + ] + + process_github_webhook.all_approvers.sort() + + process_github_webhook.all_reviewers = [ + "folder1_reviewer1", + "folder1_reviewer2", + "folder4_reviewer1", + "folder4_reviewer2", + "folder5_reviewer1", + "folder5_reviewer2", + "root_reviewer1", + "root_reviewer2", + ] + + process_github_webhook.all_reviewers.sort() @pytest.mark.parametrize( @@ -124,26 +208,29 @@ def test_get_size_thresholds(process_github_webhook, additions, deletions, expec def test_get_approvers_and_reviewers(process_github_webhook, approvers_and_reviewers): process_github_webhook.repository = Repository() - read_owners_result = process_github_webhook.get_approvers_and_reviewers() - assert read_owners_result == process_github_webhook.approvers_and_reviewers + read_owners_result = process_github_webhook.get_all_approvers_and_reviewers() + assert read_owners_result == process_github_webhook.all_approvers_and_reviewers def test_owners_data_for_changed_files(process_github_webhook, approvers_and_reviewers): owners_data_chaged_files_result = process_github_webhook.owners_data_for_changed_files() owners_data_chaged_files_expected = { "approvers": [ - ["approver1", "approver2"], - ["approver1", "approver2"], - ["approver3", "approver4"], - ["approver1", "approver2"], + [], + ["folder1_approver1", "folder1_approver2"], + ["folder4_approver1", "folder4_approver2"], + ["folder5_approver1", "folder5_approver2"], + ["root_approver1", "root_approver2"], ], "reviewers": [ - ["reviewer1", "reviewer2"], - ["reviewer1", "reviewer2"], - ["reviewer3", "reviewer4"], - ["reviewer1", "reviewer2"], + [], + ["folder1_reviewer1", "folder1_reviewer2"], + ["folder4_reviewer1", "folder4_reviewer2"], + ["folder5_reviewer1", "folder5_reviewer2"], + ["root_reviewer1", "root_reviewer2"], ], } + owners_data_chaged_files_expected["approvers"].sort() owners_data_chaged_files_expected["reviewers"].sort() assert owners_data_chaged_files_result == owners_data_chaged_files_expected @@ -157,36 +244,165 @@ def test_all_approvers_reviewers(process_github_webhook, approvers_and_reviewers assert all_reviewers == process_github_webhook.all_reviewers -def test_check_if_pr_approved(process_github_webhook, approvers_and_reviewers, all_approvers_reviewers): - pr_approved_all_result = process_github_webhook._check_if_pr_approved( +def test_check_pr_approved(process_github_webhook, approvers_and_reviewers, all_approvers_reviewers): + check_if_pr_approved = process_github_webhook._check_if_pr_approved( labels=[ - f"{APPROVED_BY_LABEL_PREFIX}approver1", - f"{APPROVED_BY_LABEL_PREFIX}approver2", - f"{APPROVED_BY_LABEL_PREFIX}approver3", - f"{APPROVED_BY_LABEL_PREFIX}approver4", + 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 pr_approved_all_result == "" + assert check_if_pr_approved == "" + - pr_approved_minimum_result = process_github_webhook._check_if_pr_approved( +def test_check_pr_minimum_approved(process_github_webhook, approvers_and_reviewers, all_approvers_reviewers): + check_if_pr_approved = process_github_webhook._check_if_pr_approved( labels=[ - f"{APPROVED_BY_LABEL_PREFIX}approver1", - f"{APPROVED_BY_LABEL_PREFIX}approver3", + 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 pr_approved_minimum_result == "" + assert check_if_pr_approved == "" + - pr_not_approved_result = process_github_webhook._check_if_pr_approved( +def test_check_pr_not_approved(process_github_webhook, approvers_and_reviewers, all_approvers_reviewers): + check_if_pr_approved = process_github_webhook._check_if_pr_approved( labels=[ - f"{APPROVED_BY_LABEL_PREFIX}approver1", + f"{APPROVED_BY_LABEL_PREFIX}root_approver1", ] ) - assert pr_not_approved_result == "Missing lgtm/approved from approvers: approver3, approver4\n" + assert not [ + True + for appr in ["folder1_approver1", "folder1_approver2", "folder4_approver1", "folder4_approver2"] + if appr not in check_if_pr_approved + ] - pr_partial_approved_result = process_github_webhook._check_if_pr_approved( + +def test_check_pr_partial_approved(process_github_webhook, approvers_and_reviewers, all_approvers_reviewers): + 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", + ] + ) + assert not [ + True + for appr in ["folder1_approver1", "folder1_approver2", "folder4_approver1", "folder4_approver2"] + if appr not in check_if_pr_approved + ] + + +@pytest.mark.parametrize( + "process_github_webhook", + [ + pytest.param([ + [ + "file_in_root", + "folder2", + "folder/folder4/another_file.txt", + ] + ]) + ], + indirect=True, +) +def test_check_pr_approved_specific_folder(process_github_webhook, approvers_and_reviewers, all_approvers_reviewers): + process_github_webhook.approvers_and_reviewers = process_github_webhook.get_all_approvers_and_reviewers() + 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([ + [ + "file_in_root", + "folder2", + "folder/another_file.txt", + ] + ]) + ], + indirect=True, +) +def test_check_pr_approved_nested_folder_no_owners( + process_github_webhook, approvers_and_reviewers, all_approvers_reviewers +): + process_github_webhook.approvers_and_reviewers = process_github_webhook.get_all_approvers_and_reviewers() + 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([ + [ + "folder1/file", + ] + ]) + ], + indirect=True, +) +def test_check_pr_approved_specific_folder_with_root_approvers( + process_github_webhook, approvers_and_reviewers, all_approvers_reviewers +): + process_github_webhook.all_approvers_and_reviewers = { + ".": {"approvers": ["root_approver1", "root_approver2"], "reviewers": ["root_reviewer1", "root_reviewer2"]}, + "folder1": { + "approvers": ["folder1_approver1", "folder1_approver2"], + "reviewers": ["folder1_reviewer1", "folder1_reviewer2"], + }, + } + check_if_pr_approved = process_github_webhook._check_if_pr_approved( + labels=[ + f"{APPROVED_BY_LABEL_PREFIX}folder1_approver1", + ] + ) + assert not [True for appr in ["root_approver1", "root_approver2"] if appr not in check_if_pr_approved] + + +@pytest.mark.parametrize( + "process_github_webhook", + [ + pytest.param([ + [ + "folder5/file", + ] + ]) + ], + indirect=True, +) +@pytest.mark.xfail +def test_check_pr_approved_specific_folder_no_root_approvers( + process_github_webhook, approvers_and_reviewers, all_approvers_reviewers +): + process_github_webhook.all_approvers_and_reviewers = { + ".": {"approvers": ["root_approver1", "root_approver2"], "reviewers": ["root_reviewer1", "root_reviewer2"]}, + "folder5": { + "approvers": ["folder5_approver1", "folder5_approver2"], + "reviewers": ["folder5_reviewer1", "folder5_reviewer2"], + "root-approvers": False, + }, + } + check_if_pr_approved = process_github_webhook._check_if_pr_approved( labels=[ - f"{APPROVED_BY_LABEL_PREFIX}approver1", - f"{APPROVED_BY_LABEL_PREFIX}approver2", + f"{APPROVED_BY_LABEL_PREFIX}folder5_approver1", ] ) - assert pr_partial_approved_result == "Missing lgtm/approved from approvers: approver3, approver4\n" + assert check_if_pr_approved == "" From 8d3c61f4ab49c6c0a0bf2556bd5ecf55ca8dca2b Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 26 Nov 2024 03:04:00 +0200 Subject: [PATCH 2/5] Refactor all owners code --- webhook_server_container/libs/github_api.py | 88 ++++----- .../tests/test_github_api.py | 179 ++++++++++++------ 2 files changed, 167 insertions(+), 100 deletions(-) diff --git a/webhook_server_container/libs/github_api.py b/webhook_server_container/libs/github_api.py index 2f0ba7c6..d00a9473 100644 --- a/webhook_server_container/libs/github_api.py +++ b/webhook_server_container/libs/github_api.py @@ -1998,9 +1998,9 @@ def run_podman_command(self, command: str, pipe: bool = False) -> Tuple[bool, st return rc, out, err - def get_all_approvers_and_reviewers(self) -> dict[str, dict[str, list[str]]]: + def get_all_approvers_and_reviewers(self) -> dict[str, dict[str, Any]]: # Dictionary mapping OWNERS file paths to their approvers and reviewers - _owners: dict[str, dict[str, list[str]]] = {} + _owners: dict[str, dict[str, Any]] = {} max_owners_files = 1000 # Configurable limit owners_count = 0 @@ -2035,44 +2035,47 @@ def get_all_approvers_and_reviewers(self) -> dict[str, dict[str, list[str]]]: def get_all_approvers(self) -> list[str]: _approvers: list[str] = [] - for list_of_approvers in self.owners_data_for_changed_files()["approvers"]: - for _approver in list_of_approvers: + for list_of_approvers in self.owners_data_for_changed_files().values(): + for _approver in list_of_approvers.get("approvers", []): _approvers.append(_approver) - approvers = list(set(self.root_approvers + _approvers)) - approvers.sort() - return approvers + _approvers.sort() + return _approvers def get_all_reviewers(self) -> list[str]: _reviewers: list[str] = [] - for list_of_reviewers in self.owners_data_for_changed_files()["reviewers"]: - for _approver in list_of_reviewers: + for list_of_reviewers in self.owners_data_for_changed_files().values(): + for _approver in list_of_reviewers.get("reviewers", []): _reviewers.append(_approver) - reviewers = list(set(self.root_reviewers + _reviewers)) - reviewers.sort() - return reviewers + _reviewers.sort() + return _reviewers - def owners_data_for_changed_files(self) -> dict[str, list[list[str]]]: - data: dict[str, list[list[str]]] = {"approvers": [], "reviewers": []} + 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} - for changed_folder_path in changed_folders: - for owners_dir, owners_data in self.all_approvers_and_reviewers.items(): - _owners_dir = Path(owners_dir) + changed_folder_match: list[str] = [] + + require_root_approvers: bool = False - if _owners_dir == changed_folder_path or _owners_dir == changed_folder_path.parents: - _reviewers = owners_data.get("reviewers", []) - self.logger.debug(f"{self.log_prefix} Found reviewers for {owners_dir}: {_reviewers}") - data["reviewers"].append(_reviewers) + for owners_dir, owners_data in self.all_approvers_and_reviewers.items(): + _owners_dir = Path(owners_dir) - _approvers = owners_data.get("approvers", []) - self.logger.debug(f"{self.log_prefix} Found approvers for {owners_dir}: {_approvers}") - data["approvers"].append(_approvers) + 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 require_root_approvers: + data["."] = self.all_approvers_and_reviewers.get(".", {}) - data["reviewers"].sort() - data["approvers"].sort() return data def _validate_owners_content(self, content: Any, path: str) -> bool: @@ -2171,16 +2174,6 @@ def _check_lables_for_can_be_merged(self, labels: list[str]) -> str: return failure_output def _check_if_pr_approved(self, labels: list[str]) -> str: - _pr_approvers: list[str] = [] - # all_needed_approvers = [] - required_pr_approvers = self.owners_data_for_changed_files()["approvers"] - - root_approvers = self.all_approvers_and_reviewers.get(".", {}).get("approvers") - if root_approvers: - required_pr_approvers.append(root_approvers) - - # required_pr_approvers is [['approver1', 'approver2'], ['approver3', 'approver4']] - # To mark PR as approved we need at least one lgtm/approved from each nested list inside all_needed_approvers approved_by = [] for _label in labels: if APPROVED_BY_LABEL_PREFIX.lower() in _label.lower(): @@ -2190,21 +2183,22 @@ def _check_if_pr_approved(self, labels: list[str]) -> str: approved_by.append(approved_user) - missing_approvers: list[str] = [] - for list_of_reviewers in required_pr_approvers: - for _approver in list_of_reviewers: - missing_approvers.append(_approver) - - missing_approvers = list(set(missing_approvers)) + missing_approvers = self.all_approvers.copy() - for approvers in required_pr_approvers: - for approver in approvers: - if approver in approved_by: - _pr_approvers.append(approver) + for data in self.owners_data_for_changed_files().values(): + required_pr_approvers = data.get("approvers", []) + for required_pr_approver in required_pr_approvers: + if required_pr_approver in approved_by: # Once we found approver in approved_by list, we remove all approvers from missing_approvers list for this owners file - {missing_approvers.remove(_approver) for _approver in approvers if _approver in missing_approvers} # type: ignore + { + missing_approvers.remove(_approver) # type: ignore + for _approver in required_pr_approvers + if _approver in missing_approvers + } break + missing_approvers = list(set(missing_approvers)) + if missing_approvers: return f"Missing lgtm/approved from approvers: {', '.join(missing_approvers)}\n" diff --git a/webhook_server_container/tests/test_github_api.py b/webhook_server_container/tests/test_github_api.py index 4165d0c2..7fb32cef 100644 --- a/webhook_server_container/tests/test_github_api.py +++ b/webhook_server_container/tests/test_github_api.py @@ -34,8 +34,6 @@ def tree(self): for _path in [ "OWNERS", "folder1/OWNERS", - "code/file.py", - "README.md", "folder2/OWNERS", "folder/folder4/OWNERS", "folder5/OWNERS", @@ -138,7 +136,7 @@ def process_github_webhook(mocker, request): @pytest.fixture(scope="function") -def approvers_and_reviewers(process_github_webhook): +def all_approvers_and_reviewers(process_github_webhook): process_github_webhook.all_approvers_and_reviewers = { ".": {"approvers": ["root_approver1", "root_approver2"], "reviewers": ["root_reviewer1", "root_reviewer2"]}, "folder1": { @@ -206,37 +204,36 @@ def test_get_size_thresholds(process_github_webhook, additions, deletions, expec assert result == f"{SIZE_LABEL_PREFIX}{expected_label}" -def test_get_approvers_and_reviewers(process_github_webhook, approvers_and_reviewers): +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() assert read_owners_result == process_github_webhook.all_approvers_and_reviewers -def test_owners_data_for_changed_files(process_github_webhook, approvers_and_reviewers): +def test_owners_data_for_changed_files(process_github_webhook, all_approvers_and_reviewers): owners_data_chaged_files_result = process_github_webhook.owners_data_for_changed_files() owners_data_chaged_files_expected = { - "approvers": [ - [], - ["folder1_approver1", "folder1_approver2"], - ["folder4_approver1", "folder4_approver2"], - ["folder5_approver1", "folder5_approver2"], - ["root_approver1", "root_approver2"], - ], - "reviewers": [ - [], - ["folder1_reviewer1", "folder1_reviewer2"], - ["folder4_reviewer1", "folder4_reviewer2"], - ["folder5_reviewer1", "folder5_reviewer2"], - ["root_reviewer1", "root_reviewer2"], - ], + "folder5": { + "approvers": ["folder5_approver1", "folder5_approver2"], + "reviewers": ["folder5_reviewer1", "folder5_reviewer2"], + "root-approvers": False, + }, + "folder1": { + "approvers": ["folder1_approver1", "folder1_approver2"], + "reviewers": ["folder1_reviewer1", "folder1_reviewer2"], + }, + ".": {"approvers": ["root_approver1", "root_approver2"], "reviewers": ["root_reviewer1", "root_reviewer2"]}, + "folder2": {}, + "folder/folder4": { + "approvers": ["folder4_approver1", "folder4_approver2"], + "reviewers": ["folder4_reviewer1", "folder4_reviewer2"], + }, } - owners_data_chaged_files_expected["approvers"].sort() - owners_data_chaged_files_expected["reviewers"].sort() assert owners_data_chaged_files_result == owners_data_chaged_files_expected -def test_all_approvers_reviewers(process_github_webhook, approvers_and_reviewers, all_approvers_reviewers): +def test_all_approvers_reviewers(process_github_webhook, all_approvers_and_reviewers, all_approvers_reviewers): all_approvers = process_github_webhook.get_all_approvers() assert all_approvers == process_github_webhook.all_approvers @@ -244,7 +241,9 @@ def test_all_approvers_reviewers(process_github_webhook, approvers_and_reviewers assert all_reviewers == process_github_webhook.all_reviewers -def test_check_pr_approved(process_github_webhook, approvers_and_reviewers, all_approvers_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", @@ -260,7 +259,8 @@ def test_check_pr_approved(process_github_webhook, approvers_and_reviewers, all_ assert check_if_pr_approved == "" -def test_check_pr_minimum_approved(process_github_webhook, approvers_and_reviewers, all_approvers_reviewers): +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", @@ -272,7 +272,8 @@ def test_check_pr_minimum_approved(process_github_webhook, approvers_and_reviewe assert check_if_pr_approved == "" -def test_check_pr_not_approved(process_github_webhook, approvers_and_reviewers, all_approvers_reviewers): +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", @@ -285,7 +286,8 @@ def test_check_pr_not_approved(process_github_webhook, approvers_and_reviewers, ] -def test_check_pr_partial_approved(process_github_webhook, approvers_and_reviewers, all_approvers_reviewers): +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", @@ -312,8 +314,8 @@ def test_check_pr_partial_approved(process_github_webhook, approvers_and_reviewe ], indirect=True, ) -def test_check_pr_approved_specific_folder(process_github_webhook, approvers_and_reviewers, all_approvers_reviewers): - process_github_webhook.approvers_and_reviewers = process_github_webhook.get_all_approvers_and_reviewers() +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", @@ -336,10 +338,8 @@ def test_check_pr_approved_specific_folder(process_github_webhook, approvers_and ], indirect=True, ) -def test_check_pr_approved_nested_folder_no_owners( - process_github_webhook, approvers_and_reviewers, all_approvers_reviewers -): - process_github_webhook.approvers_and_reviewers = process_github_webhook.get_all_approvers_and_reviewers() +def test_check_pr_approved_nested_folder_no_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", @@ -359,16 +359,8 @@ def test_check_pr_approved_nested_folder_no_owners( ], indirect=True, ) -def test_check_pr_approved_specific_folder_with_root_approvers( - process_github_webhook, approvers_and_reviewers, all_approvers_reviewers -): - process_github_webhook.all_approvers_and_reviewers = { - ".": {"approvers": ["root_approver1", "root_approver2"], "reviewers": ["root_reviewer1", "root_reviewer2"]}, - "folder1": { - "approvers": ["folder1_approver1", "folder1_approver2"], - "reviewers": ["folder1_reviewer1", "folder1_reviewer2"], - }, - } +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", @@ -388,21 +380,102 @@ def test_check_pr_approved_specific_folder_with_root_approvers( ], indirect=True, ) -@pytest.mark.xfail -def test_check_pr_approved_specific_folder_no_root_approvers( - process_github_webhook, approvers_and_reviewers, all_approvers_reviewers +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([ + [ + "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", + ] + ) + assert not [True for appr in ["root_approver1", "root_approver2"] if appr not in check_if_pr_approved] + + +@pytest.mark.parametrize( + "process_github_webhook", + [ + 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([ + [ + "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_and_reviewers = { - ".": {"approvers": ["root_approver1", "root_approver2"], "reviewers": ["root_reviewer1", "root_reviewer2"]}, - "folder5": { - "approvers": ["folder5_approver1", "folder5_approver2"], - "reviewers": ["folder5_reviewer1", "folder5_reviewer2"], - "root-approvers": False, - }, - } + 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([ + [ + "folder_with_no_owners/file", + "folder5/file", + ] + ]) + ], + indirect=True, +) +def test_check_pr_not_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}folder5_approver1", + ] + ) + assert not [True for appr in ["root_approver1", "root_approver2"] if appr not in check_if_pr_approved] From e17e9961442c531e17a61cb35a0ae7bc4bf40697 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 26 Nov 2024 15:17:14 +0200 Subject: [PATCH 3/5] Fix is not mergeable fstring --- webhook_server_container/libs/github_api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/webhook_server_container/libs/github_api.py b/webhook_server_container/libs/github_api.py index d00a9473..8b87dfc8 100644 --- a/webhook_server_container/libs/github_api.py +++ b/webhook_server_container/libs/github_api.py @@ -1299,7 +1299,7 @@ def check_if_can_be_merged(self) -> None: self.logger.debug(f"{self.log_prefix} check if can be merged. PR labels are: {_labels}") if not self.pull_request.mergeable: - failure_output += "PR is not mergeable: {self.pull_request.mergeable_state}\n" + failure_output += f"PR is not mergeable: {self.pull_request.mergeable_state}\n" required_check_in_progress_failure_output, check_runs_in_progress = self._required_check_in_progress( last_commit_check_runs=last_commit_check_runs From 015b55b63a764c82a5a641179656910c76ce6124 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 26 Nov 2024 15:18:26 +0200 Subject: [PATCH 4/5] Fix is not mergeable fstring --- webhook_server_container/libs/github_api.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/webhook_server_container/libs/github_api.py b/webhook_server_container/libs/github_api.py index 8b87dfc8..f22a2150 100644 --- a/webhook_server_container/libs/github_api.py +++ b/webhook_server_container/libs/github_api.py @@ -1298,8 +1298,9 @@ def check_if_can_be_merged(self) -> None: _labels = self.pull_request_labels_names() self.logger.debug(f"{self.log_prefix} check if can be merged. PR labels are: {_labels}") - if not self.pull_request.mergeable: - failure_output += f"PR is not mergeable: {self.pull_request.mergeable_state}\n" + is_pr_mergable = self.pull_request.mergeable + if not is_pr_mergable: + failure_output += f"PR is not mergeable: {is_pr_mergable}\n" required_check_in_progress_failure_output, check_runs_in_progress = self._required_check_in_progress( last_commit_check_runs=last_commit_check_runs From dbb8def5d6b44f906faeb083a552cc4069d640dd Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 26 Nov 2024 15:39:04 +0200 Subject: [PATCH 5/5] Fix tests --- .../tests/test_github_api.py | 57 +++++++++++++++---- 1 file changed, 46 insertions(+), 11 deletions(-) diff --git a/webhook_server_container/tests/test_github_api.py b/webhook_server_container/tests/test_github_api.py index 7fb32cef..c8fd2234 100644 --- a/webhook_server_container/tests/test_github_api.py +++ b/webhook_server_container/tests/test_github_api.py @@ -279,11 +279,18 @@ def test_check_pr_not_approved(process_github_webhook, all_approvers_and_reviewe f"{APPROVED_BY_LABEL_PREFIX}root_approver1", ] ) - assert not [ - True - for appr in ["folder1_approver1", "folder1_approver2", "folder4_approver1", "folder4_approver2"] - if appr not in check_if_pr_approved + 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): @@ -294,11 +301,18 @@ def test_check_pr_partial_approved(process_github_webhook, all_approvers_and_rev f"{APPROVED_BY_LABEL_PREFIX}root_approver2", ] ) - assert not [ - True - for appr in ["folder1_approver1", "folder1_approver2", "folder4_approver1", "folder4_approver2"] - if appr not in check_if_pr_approved + 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( @@ -366,7 +380,14 @@ def test_check_pr_approved_specific_folder_with_root_approvers(process_github_we f"{APPROVED_BY_LABEL_PREFIX}folder1_approver1", ] ) - assert not [True for appr in ["root_approver1", "root_approver2"] if appr not in 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 @pytest.mark.parametrize( @@ -408,7 +429,14 @@ def test_check_pr_not_approved_specific_folder_without_owners(process_github_web f"{APPROVED_BY_LABEL_PREFIX}folder5_approver1", ] ) - assert not [True for appr in ["root_approver1", "root_approver2"] if appr not in 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 @pytest.mark.parametrize( @@ -478,4 +506,11 @@ def test_check_pr_not_approved_folder_with_no_owners_and_folder_without_root_app f"{APPROVED_BY_LABEL_PREFIX}folder5_approver1", ] ) - assert not [True for appr in ["root_approver1", "root_approver2"] if appr not in 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