check only parent path for owners file, make sure root owners are alw…#631
check only parent path for owners file, make sure root owners are alw…#631
Conversation
|
Report bugs in Issues The following are automatically added:
Available user actions:
Supported /retest check runs
Supported labels
|
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (6)
webhook_server_container/tests/test_github_api.py (4)
391-392: Test marked as xfail without a reasonThe test
test_check_pr_approved_specific_folder_no_root_approversis marked with@pytest.mark.xfailbut lacks a reason. Providing a reason for expected failures improves test documentation and clarity.Add a reason to the
xfailmarker:@pytest.mark.xfail +@pytest.mark.xfail(reason="Feature not yet implemented") def test_check_pr_approved_specific_folder_no_root_approvers(
96-98: Unnecessary handling of path"folder"inget_contentsIn the
get_contentsmethod, theelif path == "folder":block returns an empty content file. If this case is not required for the tests or does not reflect a realistic scenario, it might be unnecessary.Consider removing this block if it's not needed:
-elif path == "folder": - return ContentFile(yaml.dump({}))
248-260: Hardcoded labels may not cover all test scenariosIn
test_check_pr_approved, the labels are hardcoded, which may limit the test's flexibility. Consider using the data fromprocess_github_webhook.all_approversto generate the labels, ensuring the test stays up-to-date with any changes to approvers.Refactor the labels generation:
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", + *[ + f"{APPROVED_BY_LABEL_PREFIX}{approver}" + for approver in process_github_webhook.all_approvers + ], ]
163-187: Duplicate sorting of approvers and reviewersThe sorting of
process_github_webhook.all_approversandprocess_github_webhook.all_reviewersis performed separately after defining the lists. To enhance maintainability, consider sorting the lists at the time of assignment.Inline the sorting during assignment:
process_github_webhook.all_approvers = sorted([ "folder1_approver1", "folder1_approver2", "folder4_approver1", "folder4_approver2", "folder5_approver1", "folder5_approver2", "root_approver1", "root_approver2", ]) # Remove the explicit sort call -process_github_webhook.all_approvers.sort() # Similarly for all_reviewers process_github_webhook.all_reviewers = sorted([ "folder1_reviewer1", "folder1_reviewer2", "folder4_reviewer1", "folder4_reviewer2", "folder5_reviewer1", "folder5_reviewer2", "root_reviewer1", "root_reviewer2", ])webhook_server_container/libs/github_api.py (2)
2178-2180: Ensure root approvers are included consistentlyThe code appends
root_approverstorequired_pr_approverswithout checking if they are already included. Consider combining and de-duplicating approvers to avoid redundancy.Apply this diff to improve the code:
- root_approvers = self.all_approvers_and_reviewers.get(".", {}).get("approvers") - if root_approvers: - required_pr_approvers.append(root_approvers) + root_approvers = self.all_approvers_and_reviewers.get(".", {}).get("approvers", []) + for approver in root_approvers: + if approver not in required_pr_approvers: + required_pr_approvers.append([approver])
2198-2198: Consider whether converting to a set is necessaryThe line
missing_approvers = list(set(missing_approvers))removes duplicates but alters the original order. If order matters, consider usingdict.fromkeysor another method to preserve order while removing duplicates.Optional improvement:
- missing_approvers = list(set(missing_approvers)) + from collections import OrderedDict + missing_approvers = list(OrderedDict.fromkeys(missing_approvers))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
webhook_server_container/libs/github_api.py(6 hunks)webhook_server_container/tests/test_github_api.py(7 hunks)
🔇 Additional comments (7)
webhook_server_container/tests/test_github_api.py (1)
148-148: Empty dictionary for folder2 may cause KeyError
In the approvers_and_reviewers fixture, folder2 is assigned an empty dictionary. If the code expects keys like "approvers" and "reviewers", this may lead to a KeyError. Ensure that all entries have the necessary keys, even if the lists are empty.
Please confirm if the empty dictionary is intentional. If not, consider initializing with empty lists:
"folder2": {
+ "approvers": [],
+ "reviewers": [],
},webhook_server_container/libs/github_api.py (6)
205-207: Initialization of approvers and reviewers looks good
The assignment of self.all_approvers_and_reviewers, self.all_approvers, and self.all_reviewers is clear and correctly utilizes the new methods.
585-587: Root reviewers retrieval is correctly updated
The property root_reviewers now appropriately accesses the root reviewers from all_approvers_and_reviewers. Logging is also implemented for debugging purposes.
591-593: Root approvers retrieval is correctly updated
The property root_approvers now correctly fetches the root approvers from all_approvers_and_reviewers. The logging statement aids in tracing.
Line range hint 2001-2023: Efficient traversal and validation of OWNERS files
The new method get_all_approvers_and_reviewers efficiently traverses the repository to collect OWNERS files, while limiting the maximum number to prevent excessive processing. The use of yaml.safe_load and validation through _validate_owners_content ensures that the OWNERS files are correctly parsed and validated.
2042-2044: Ensure consistent sorting of approvers list
While sorting approvers ensures consistency, consider if the order of approvers is significant elsewhere in the code. If not, this is acceptable.
2052-2054: Ensure consistent sorting of reviewers list
Similarly, sorting the reviewers list helps maintain consistency. Confirm that this does not affect any logic that relies on the order of reviewers.
| @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): |
There was a problem hiding this comment.
Potential misuse of pytest.param in @pytest.mark.parametrize
When passing parameters to the process_github_webhook fixture, wrapping the list of files inside another list may cause unintended behavior. The fixture expects request.param to be the list of changed files.
Adjust the parameterization to pass the list correctly:
@pytest.mark.parametrize(
"process_github_webhook",
[
- pytest.param([
- [
+ pytest.param(
+ [
"file_in_root",
"folder2",
"folder/folder4/another_file.txt",
- ]
])
],
indirect=True,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @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): | |
| @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): |
| assert not [ | ||
| True | ||
| for appr in ["folder1_approver1", "folder1_approver2", "folder4_approver1", "folder4_approver2"] | ||
| if appr not in check_if_pr_approved | ||
| ] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve assertion for better readability and accuracy
The assertion using assert not [True for appr in ... if appr not in check_if_pr_approved] can be unclear and may not function as intended if check_if_pr_approved is a string. It's better to use a more explicit assertion to check the absence of expected approvers.
Refactor the assertion for clarity:
-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
+ for appr in ["folder1_approver1", "folder1_approver2", "folder4_approver1", "folder4_approver2"]
+ if appr not in check_if_pr_approved
+]
+assert not missing_approvers, f"Missing approvers: {missing_approvers}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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 | |
| for appr in ["folder1_approver1", "folder1_approver2", "folder4_approver1", "folder4_approver2"] | |
| if appr not in check_if_pr_approved | |
| ] | |
| assert not missing_approvers, f"Missing approvers: {missing_approvers}" |
| assert not [ | ||
| True | ||
| for appr in ["folder1_approver1", "folder1_approver2", "folder4_approver1", "folder4_approver2"] | ||
| if appr not in check_if_pr_approved | ||
| ] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify assertion to check for missing approvers
Similar to the previous comment, the assertion in test_check_pr_partial_approved can be refactored for better understanding and correct functionality.
Refactor the assertion:
-missing_approvers = [
- True
- for appr in ["folder1_approver1", "folder1_approver2", "folder4_approver1", "folder4_approver2"]
- if appr not in check_if_pr_approved
-]
-assert not missing_approvers
+missing_approvers = [
+ appr
+ for appr in ["folder1_approver1", "folder1_approver2", "folder4_approver1", "folder4_approver2"]
+ if appr not in check_if_pr_approved
+]
+assert missing_approvers, f"Approvals missing from: {missing_approvers}"This ensures that the test accurately reflects the partial approval state.
Committable suggestion skipped: line range outside the PR's diff.
| if hasattr(request, "param") and request.param: | ||
| process_github_webhook.changed_files = request.param[0] | ||
| else: | ||
| process_github_webhook.changed_files = ALL_CHANGED_FILES |
There was a problem hiding this comment.
Potential IndexError when accessing request.param[0]
In the process_github_webhook fixture, accessing request.param[0] may raise an IndexError if request.param is an empty list. It's safer to directly assign request.param to process_github_webhook.changed_files without indexing, or check if request.param is a non-empty list.
Apply this diff to fix the issue:
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_FILESCommittable suggestion skipped: line range outside the PR's diff.
| _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: |
There was a problem hiding this comment.
Fix comparison logic when checking for matching directories
The comparison if _owners_dir == changed_folder_path.parents is incorrect because changed_folder_path.parents is a sequence of parent directories, not a single Path object. Instead, you should check if _owners_dir is in changed_folder_path.parents.
Apply this diff to fix the issue:
- if _owners_dir == changed_folder_path or _owners_dir == changed_folder_path.parents:
+ if _owners_dir == changed_folder_path or _owners_dir in changed_folder_path.parents:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if _owners_dir == changed_folder_path or _owners_dir == changed_folder_path.parents: | |
| if _owners_dir == changed_folder_path or _owners_dir in changed_folder_path.parents: |
| _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 |
There was a problem hiding this comment.
Incorrect use of set comprehension for side-effects
Using a set comprehension {missing_approvers.remove(_approver) for _approver in approvers if _approver in missing_approvers} to modify missing_approvers relies on side-effects, which is not appropriate. Comprehensions should be used for generating new collections, not for side-effects.
Apply this diff to fix the issue:
- {missing_approvers.remove(_approver) for _approver in approvers if _approver in missing_approvers} # type: ignore
+ for _approver in approvers:
+ if _approver in missing_approvers:
+ missing_approvers.remove(_approver)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {missing_approvers.remove(_approver) for _approver in approvers if _approver in missing_approvers} # type: ignore | |
| for _approver in approvers: | |
| if _approver in missing_approvers: | |
| missing_approvers.remove(_approver) |
| assert not [ | ||
| True | ||
| for appr in ["folder1_approver1", "folder1_approver2", "folder4_approver1", "folder4_approver2"] | ||
| if appr not in check_if_pr_approved | ||
| ] |
There was a problem hiding this comment.
should not this include folder5_approver1 and folder5_approver2? ALL_CHANGED_FILES includes "folder5/file" and it has it's own owners file
| ) | ||
| assert not [ | ||
| True | ||
| for appr in ["folder1_approver1", "folder1_approver2", "folder4_approver1", "folder4_approver2"] |
There was a problem hiding this comment.
Same question, about folder5_approver1 and approver2.
| for _path in ["OWNERS", "test1/OWNERS", "code/file.py", "README.md"]: | ||
| for _path in [ | ||
| "OWNERS", | ||
| "folder1/OWNERS", |
There was a problem hiding this comment.
Do you need to update it? Between the this, tests and get_contents() I was a bit confused.
myakove-bot
added
size/XXL
and removed
commented-coderabbitai[bot]
size/XL
commented-dbasunag
labels
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
webhook_server_container/tests/test_github_api.py (4)
10-18: Add documentation to clarify the relationship between test data structuresThe relationship between
ALL_CHANGED_FILES,Tree.treepaths, andRepository.get_contents()responses could be clearer. Consider adding docstrings to explain how these components work together in the test setup.+# List of files that simulate changes in a pull request ALL_CHANGED_FILES = [ "OWNERS", "folder1/OWNERS", # ... ] class Tree: + """Simulates GitHub's git tree API response. + The tree property returns paths that should match the Repository.get_contents() mock data. + """Also applies to: 34-40
62-98: Consider refactoring mock data for better maintainabilityThe mock YAML data could be extracted into class constants or a separate test data file to improve maintainability and reusability.
class Repository: + # Mock YAML data for different owner files + MOCK_DATA = { + "OWNERS": { + "approvers": ["root_approver1", "root_approver2"], + "reviewers": ["root_reviewer1", "root_reviewer2"], + }, + "folder1/OWNERS": { + "approvers": ["folder1_approver1", "folder1_approver2"], + "reviewers": ["folder1_reviewer1", "folder1_reviewer2"], + }, + # ... other mock data + } + def get_contents(self, path: str): - owners_data = yaml.dump({ - "approvers": ["root_approver1", "root_approver2"], - "reviewers": ["root_reviewer1", "root_reviewer2"], - }) - # ... other mock data + if path in self.MOCK_DATA: + return ContentFile(yaml.dump(self.MOCK_DATA[path])) + return ContentFile(yaml.dump({}))
207-457: Add tests for error scenariosThe test suite thoroughly covers happy paths but could benefit from additional test cases for error scenarios:
- Invalid YAML in owner files
- Missing required fields in owner files
- Permission denied errors when accessing files
Would you like me to help generate these additional test cases?
244-481: Consider grouping related tests using test classesThe test methods could be organized into test classes for better structure and readability:
TestPRApprovalfor basic approval testsTestSpecificFolderApprovalfor folder-specific testsTestRootApproverBehaviorfor root approver testsWould you like me to help restructure the tests into classes?
webhook_server_container/libs/github_api.py (1)
Line range hint
2012-2013: Consider using Path.resolve() for parent path handlingWhen determining the parent path, consider using
Path(content_path).resolve()to handle edge cases with symlinks and normalize the path.- parent_path = str(Path(content_path).parent) + parent_path = str(Path(content_path).resolve().parent)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
webhook_server_container/libs/github_api.py(5 hunks)webhook_server_container/tests/test_github_api.py(6 hunks)
🔇 Additional comments (5)
webhook_server_container/tests/test_github_api.py (3)
130-133:
Fix potential IndexError in request.param handling
This code segment still has the potential IndexError issue mentioned in a previous review.
Apply the previously suggested fix:
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_FILES282-286: 🛠️ Refactor suggestion
Improve assertion readability
The assertion using list comprehension with not [True for ...] is unclear and was previously flagged for improvement.
Apply the previously suggested fix to both test methods:
- 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
+ for appr in ["folder1_approver1", "folder1_approver2", "folder4_approver1", "folder4_approver2"]
+ if appr not in check_if_pr_approved
+ ]
+ assert not missing_approvers, f"Missing approvers: {missing_approvers}"Also applies to: 297-301
304-316:
Fix pytest.param usage
The parameterization still has the nested list issue mentioned in a previous review.
Apply the previously suggested fix:
@pytest.mark.parametrize(
"process_github_webhook",
[
- pytest.param([
- [
+ pytest.param(
+ [
"file_in_root",
"folder2",
"folder/folder4/another_file.txt",
- ]
])
],
indirect=True,
)webhook_server_container/libs/github_api.py (2)
205-205: LGTM: Property updates are consistent with new data structure
The changes to the properties and their usage are consistent with the new dictionary structure for approvers and reviewers data.
Also applies to: 585-593
Line range hint 2001-2043: LGTM: Robust OWNERS file handling implementation
The implementation includes:
- Safety limit for maximum OWNERS files
- Proper YAML validation and error handling
- Structured content validation
| for changed_folder in changed_folders: | ||
| if _owners_dir == changed_folder or _owners_dir.parents == changed_folders: | ||
| data[owners_dir] = owners_data |
There was a problem hiding this comment.
Fix path comparison logic in owners_data_for_changed_files
There's a critical issue in the path comparison logic. The current implementation incorrectly compares _owners_dir.parents (which is a sequence of paths) with changed_folders (which is a set).
Apply this fix:
- if _owners_dir == changed_folder or _owners_dir.parents == changed_folders:
+ if _owners_dir == changed_folder or changed_folder in _owners_dir.parents:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for changed_folder in changed_folders: | |
| if _owners_dir == changed_folder or _owners_dir.parents == changed_folders: | |
| data[owners_dir] = owners_data | |
| for changed_folder in changed_folders: | |
| if _owners_dir == changed_folder or changed_folder in _owners_dir.parents: | |
| data[owners_dir] = owners_data |
| 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 | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor approval validation to avoid side effects in comprehension
The current implementation uses a set comprehension for its side effects, which is an anti-pattern. Comprehensions should be used for creating new collections, not for side effects.
Apply this refactor:
- {
- missing_approvers.remove(_approver) # type: ignore
- for _approver in required_pr_approvers
- if _approver in missing_approvers
- }
+ for _approver in required_pr_approvers:
+ if _approver in missing_approvers:
+ missing_approvers.remove(_approver)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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 | |
| } | |
| 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 | |
| for _approver in required_pr_approvers: | |
| if _approver in missing_approvers: | |
| missing_approvers.remove(_approver) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
webhook_server_container/libs/github_api.py (2)
Line range hint
2002-2044: Enhance error handling for YAML parsingWhile the implementation is solid, consider improving the error handling for invalid YAML files by:
- Adding more specific error details in the log
- Potentially notifying repository owners about invalid OWNERS files
try: content = yaml.safe_load(_path.decoded_content) if self._validate_owners_content(content, content_path): parent_path = str(Path(content_path).parent) if not parent_path: parent_path = "." _owners[parent_path] = content except yaml.YAMLError as exp: - self.logger.error(f"{self.log_prefix} Invalid OWNERS file {content_path}: {exp}") + error_msg = f"Invalid YAML in OWNERS file {content_path}: {exp}" + self.logger.error(f"{self.log_prefix} {error_msg}") + if self.pull_request: + self.pull_request.create_issue_comment(f"⚠️ {error_msg}") continue
2201-2202: Optimize the deduplication logicThe conversion between list and set could be more efficient by using a set earlier in the process.
- missing_approvers = self.all_approvers.copy() + missing_approvers = set(self.all_approvers) # ... rest of the code ... - missing_approvers = list(set(missing_approvers)) + missing_approvers = list(missing_approvers)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
webhook_server_container/libs/github_api.py(6 hunks)
🔇 Additional comments (5)
webhook_server_container/libs/github_api.py (5)
205-205: LGTM: Property initialization is correct
The initialization of all_approvers_and_reviewers is properly placed in the constructor sequence.
585-593: LGTM: Root level access implementation is correct
The properties correctly access root level ("." key) approvers and reviewers from the all_approvers_and_reviewers dictionary with appropriate debug logging.
2048-2053: LGTM: Reviewer collection implementation is correct
The method properly collects and sorts reviewers from all relevant OWNERS files, maintaining consistency in the output order.
2067-2069:
Fix path comparison logic
The path comparison logic is incorrect. The expression _owners_dir.parents == changed_folders compares a sequence of paths with a set, which won't work as intended.
Apply this fix as suggested in the previous review:
- if _owners_dir == changed_folder or _owners_dir.parents == changed_folders:
+ if _owners_dir == changed_folder or changed_folder in _owners_dir.parents:2189-2198:
Refactor the approval validation logic
The code uses set comprehension for side effects, which is an anti-pattern. This makes the code harder to understand and maintain.
Apply this fix as suggested in the previous review:
- {
- missing_approvers.remove(_approver) # type: ignore
- for _approver in required_pr_approvers
- if _approver in missing_approvers
- }
+ for _approver in required_pr_approvers:
+ if _approver in missing_approvers:
+ missing_approvers.remove(_approver)
dbasunag
left a comment
dbasunag
left a comment
There was a problem hiding this comment.
Other than comments on test_check_pr_partial_approved() and test_check_pr_not_approved(), looks good! Let me know once merged etc. I will keep an eye.
|
/verified |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
webhook_server_container/tests/test_github_api.py (1)
282-293: Refactor repeated assertion patternMultiple test cases use the same pattern for asserting missing approvers. Consider extracting this into a helper function.
Add this helper function:
def assert_missing_approvers(actual_message, expected_approvers): missing_approvers = [appr.strip() for appr in actual_message.split(":")[-1].strip().split(",")] missing_approvers.sort() expected_approvers.sort() assert missing_approvers == expected_approversThen use it in the test cases:
assert_missing_approvers(check_if_pr_approved, expected_approvers)Also applies to: 304-315, 383-390, 432-439, 509-516
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
webhook_server_container/tests/test_github_api.py(6 hunks)
🔇 Additional comments (4)
webhook_server_container/tests/test_github_api.py (4)
77-81: LGTM! Good test coverage for root-approvers feature
The YAML data for folder5 correctly tests the root-approvers: false case, which is essential for verifying that folders can opt out of requiring root approver approval.
376-390: LGTM! Good test coverage for root approver requirement
This test case correctly verifies that changes in folders with OWNERS files still require root approver approval, which aligns with the PR objective of ensuring root owners are always approvers.
404-411: LGTM! Good test coverage for root-approvers override
This test case correctly verifies that folders can opt out of requiring root approver approval using the root-approvers: false setting.
130-133:
Fix potential IndexError in process_github_webhook fixture
The current implementation assumes request.param[0] exists, which could raise an IndexError.
Apply this fix:
if hasattr(request, "param") and request.param:
- process_github_webhook.changed_files = request.param[0]
+ process_github_webhook.changed_files = request.param if isinstance(request.param, list) else [request.param]
else:
process_github_webhook.changed_files = ALL_CHANGED_FILESLikely invalid or redundant comment.
|
New container for quay.io/myakove/github-webhook-server:latest published |
3 participants
check only parent path for owners file, make sure root owners are always approvers
Summary by CodeRabbit
New Features
Bug Fixes
Tests