Conversation
WalkthroughThe pull request introduces significant changes to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Report bugs in Issues The following are automatically added:
Available user actions:
Supported /retest check runs
Supported labels
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
webhook_server_container/libs/github_api.py (1)
2082-2089: Consider adding safeguards against infinite recursion.While the recursive approach is clean, it could benefit from additional error handling to prevent infinite recursion in case of deeply nested directories with no OWNERS file.
def _get_owner_dir_name_for_changed_folder(self, changed_folder: str) -> str: + max_depth = 100 # Configurable maximum recursion depth + def _recursive_find(folder: str, depth: int = 0) -> str: + if depth > max_depth: + self.logger.warning(f"{self.log_prefix} Max recursion depth reached for {folder}") + return "." + if folder in self.all_approvers_and_reviewers.keys(): + return folder + parent = str(Path(folder).parent) + if parent == folder: # We've reached the root + return "." + return _recursive_find(parent, depth + 1) + + return _recursive_find(changed_folder)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
webhook_server_container/libs/github_api.py(3 hunks)webhook_server_container/tests/test_github_api.py(1 hunks)
🧰 Additional context used
📓 Learnings (1)
webhook_server_container/libs/github_api.py (1)
Learnt from: myakove
PR: myk-org/github-webhook-server#624
File: webhook_server_container/libs/github_api.py:596-604
Timestamp: 2024-11-21T13:34:45.218Z
Learning: In the codebase, aggregation of reviewers and approvers from all OWNERS files is handled within the `assign_reviewers` method.
🔇 Additional comments (3)
webhook_server_container/tests/test_github_api.py (1)
225-225: LGTM! Test coverage added for the new path.
The test case properly validates that the "code" path inherits approvers and reviewers from the root OWNERS file.
webhook_server_container/libs/github_api.py (2)
2041-2042: LGTM! Duplicate prevention added.
Good addition of duplicate checks before appending approvers and reviewers. This prevents potential issues with duplicate entries in the lists.
Also applies to: 2050-2052
2061-2072: LGTM! Well-structured refactoring.
The refactoring improves code clarity by:
- Using the new helper method for owner directory lookup
- Properly handling the root-approvers flag
- Using clear variable names
|
|
||
| changed_folders = {Path(cf).parent for cf in self.changed_files} | ||
|
|
||
| all_owner_data = self.all_approvers_and_reviewers |
There was a problem hiding this comment.
why is this assignment needed?
There was a problem hiding this comment.
I need to change these based on what you suggested. This PR is not ready for review. I will add a wip.
|
|
||
| return data | ||
|
|
||
| def _get_owner_dir_name_for_changed_folder(self, changed_folder: str) -> str: |
There was a problem hiding this comment.
please get here Path, not str and return Path as well
There was a problem hiding this comment.
I have changed it totally. Now I am returning the combined approvers, reviewers dict. Do you still want me to pass the Path to the function? Let me know I will update accordingly.
| @@ -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} | |||
There was a problem hiding this comment.
can you please add docstring that shows examples?
There was a problem hiding this comment.
Still have not added docstring. Will do it next.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
webhook_server_container/libs/github_api.py (5)
2041-2042: Optimize duplicate checking by using a set for_approversCurrently, the code checks for duplicates manually when appending to
_approvers. Using asetcan improve efficiency and readability.Apply this diff to refactor the code:
def get_all_approvers(self) -> list[str]: - _approvers: list[str] = [] + _approvers: set[str] = set() for list_of_approvers in self.owners_data_for_changed_files().values(): for _approver in list_of_approvers.get("approvers", []): - if _approver not in _approvers: - _approvers.append(_approver) + _approvers.add(_approver) - _approvers.sort() - return _approvers + return sorted(_approvers)
2050-2052: Optimize duplicate checking by using a set for_reviewersSimilar to
_approvers, using asetfor_reviewersavoids manual duplication checks and enhances performance.Apply this diff to refactor the code:
def get_all_reviewers(self) -> list[str]: - _reviewers: list[str] = [] + _reviewers: set[str] = set() for list_of_reviewers in self.owners_data_for_changed_files().values(): for _reviewer in list_of_reviewers.get("reviewers", []): - if _reviewer not in _reviewers: - _reviewers.append(_reviewer) + _reviewers.add(_reviewer) - _reviewers.sort() - return _reviewers + return sorted(_reviewers)
2062-2068: Clarify the control flow and handle missing OWNERS files gracefullyEnsure that all changed folders are processed correctly, even if some do not have associated OWNERS files. Also, consider adding comments to improve readability.
No code changes are necessary, but adding comments can help future maintainers understand the logic.
2074-2105: Avoid leading underscores in parameter namesLeading underscores in parameter names can be confusing, as they typically indicate private variables. Renaming
_changed_foldertochanged_folderimproves code clarity and adheres to Python conventions.Apply this diff to rename the parameter and update its usage:
def _get_owner_data_for_changed_folder(self, changed_folder: str) -> dict[str, Any]: # If we find an 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. # "root-approvers" would be copied from the owner file found below root. # Example: if a file is changed under test_folder0/test_folder1/test_folder2/test_folder3/, # reviewers and approvers from test_folder3, test_folder2, test_folder1, test_folder0, # and root would all be combined to get approvers and reviewers for that file. _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] + if owners_data.get(changed_folder): + return owners_data[changed_folder] else: # Find all OWNERS files up to the root and combine them while changed_folder != ".": - changed_folder = str(Path(_changed_folder).parent) or "." - if owners_data.get(_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", []) + 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", []) + 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
2083-2105: Improve efficiency by using sets for_aggregated_ownersUsing sets for
approversandreviewersimproves performance and simplifies duplicate handling.Apply this diff to implement sets:
-_aggregated_owners: dict[str, Any] = {"approvers": [], "reviewers": []} +_aggregated_owners: dict[str, Any] = {"approvers": set(), "reviewers": set()} owners_data = self.all_approvers_and_reviewers if owners_data.get(changed_folder): return owners_data[changed_folder] else: # Find all OWNERS files up to the root and combine them while changed_folder != ".": changed_folder = str(Path(changed_folder).parent) or "." if owners_data.get(changed_folder): - _aggregated_owners["approvers"].extend([ + _aggregated_owners["approvers"].update([ _approver for _approver in owners_data[changed_folder].get("approvers", []) - if _approver not in _aggregated_owners["approvers"] ]) - _aggregated_owners["reviewers"].extend([ + _aggregated_owners["reviewers"].update([ _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 + return { + "approvers": list(_aggregated_owners["approvers"]), + "reviewers": list(_aggregated_owners["reviewers"]), + "root-approvers": _aggregated_owners.get("root-approvers", True), + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
webhook_server_container/libs/github_api.py(2 hunks)webhook_server_container/tests/test_github_api.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- webhook_server_container/tests/test_github_api.py
🔇 Additional comments (1)
webhook_server_container/libs/github_api.py (1)
2090-2104: Ensure termination of the while loop to prevent infinite loops
The condition changed_folder != "." may not be sufficient if Path(changed_folder).parent returns the same directory. Consider adding a base case or additional checks to ensure the loop will terminate.
Run the following script to check for potential infinite loops:
3 participants
…
Summary by CodeRabbit
New Features
Bug Fixes
Tests