feat: read repo config from repository config file#668
Conversation
|
Report bugs in Issues The following are automatically added:
Available user actions:
Supported /retest check runs
Supported labels
|
WalkthroughThe pull request updates type annotations in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Pro Plan! Simply use the command ✨ Finishing Touches
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: 1
🧹 Nitpick comments (1)
webhook_server_container/libs/github_api.py (1)
372-372: Fix typo in configuration filename.There's a typo in the configuration filename:
.gethub-webhok_server.configshould probably be.github-webhook_server.config.- content = repository.get_contents(".gethub-webhok_server.config", ref=self.pull_request_branch) + content = repository.get_contents(".github-webhook_server.config", ref=self.pull_request_branch)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
webhook_server_container/libs/github_api.py(27 hunks)webhook_server_container/utils/helpers.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: tox
- GitHub Check: python-module-install
- GitHub Check: build-container
- GitHub Check: pre-commit
🔇 Additional comments (5)
webhook_server_container/utils/helpers.py (1)
19-32: Implementation ofthird_dictparameter looks good!The function has been updated to accept an optional third dictionary for lookups. The implementation correctly handles the case when
third_dictis None and the docstring has been updated to reflect the new behavior.webhook_server_container/libs/github_api.py (4)
387-391: Good use of dictionary unpacking for cleaner code.The use of dictionary unpacking to pass parameters to
get_value_from_dictsmakes the code cleaner and more maintainable.
131-136: Improved error handling with try-except block.Adding a try-except block for
NoPullRequestErrorallows the code to gracefully handle cases where a pull request cannot be found.
208-210: Efficient resource usage.Checking if
self.pull_requestis None before calling_get_pull_request()avoids unnecessary API calls, which is a good optimization.
13-13: Modernized type annotations.Replacing
Dictwithdictaligns with modern Python type annotation practices. In Python 3.9+, the built-in types can be used directly for annotations.
| if not repo_data or local_repo_dict: | ||
| raise RepositoryNotFoundError(f"Repository {self.repository_name} not found in config file") |
There was a problem hiding this comment.
Fix incorrect conditional logic.
The current condition will raise an error if repo_data is empty BUT local_repo_dict has data, which doesn't seem to be the intended behavior. The logic should check if both are empty before raising an error.
- if not repo_data or local_repo_dict:
+ if not repo_data and not local_repo_dict:📝 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 not repo_data or local_repo_dict: | |
| raise RepositoryNotFoundError(f"Repository {self.repository_name} not found in config file") | |
| if not repo_data and not local_repo_dict: | |
| raise RepositoryNotFoundError(f"Repository {self.repository_name} not found in config file") |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (4)
webhook_server_container/app.py (4)
18-18: 🛠️ Refactor suggestionUpdate return type annotation to use lowercase types.
For consistency with the modernization effort, the return type should also use lowercase
dictinstead ofDict.-def healthcheck() -> Dict[str, Any]: +def healthcheck() -> dict[str, Any]:
23-23: 🛠️ Refactor suggestionUpdate return type annotation to use lowercase types.
For consistency with the modernization effort, the return type should also use lowercase
dictinstead ofDict.-async def process_webhook(request: Request) -> Dict[str, Any]: +async def process_webhook(request: Request) -> dict[str, Any]:
27-27: 🛠️ Refactor suggestionUpdate type annotation to use lowercase types.
For consistency with the modernization effort, the type annotation should use lowercase
dictinstead ofDict.- process_failed_msg: Dict[str, Any] = { + process_failed_msg: dict[str, Any] = {
33-33: 🛠️ Refactor suggestionUpdate type annotation to use lowercase types.
For consistency with the modernization effort, the type annotation should use lowercase
dictinstead ofDict.- hook_data: Dict[Any, Any] = await request.json() + hook_data: dict[Any, Any] = await request.json()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
webhook_server_container/app.py(2 hunks)webhook_server_container/libs/github_api.py(56 hunks)webhook_server_container/utils/github_repository_settings.py(11 hunks)webhook_server_container/utils/helpers.py(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- webhook_server_container/utils/helpers.py
- webhook_server_container/libs/github_api.py
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: tox
- GitHub Check: python-module-install
- GitHub Check: build-container
- GitHub Check: pre-commit
🔇 Additional comments (16)
webhook_server_container/app.py (3)
7-7: LGTM! Good addition of explicit Request import.Explicitly importing
Requestfrom FastAPI is a good practice since it's being used in the function signature at line 23.
47-47: Good simplification of exception handling.Nice cleanup by removing the unused
exc_objvariable and replacing it with the underscore convention for unused variables.
53-53: Good addition of error logging.Adding this logging statement enhances error visibility and debugging capabilities.
webhook_server_container/utils/github_repository_settings.py (13)
5-5: LGTM! Good cleanup of imports.The import statement has been updated to only include the types that are actually used in the file, removing unnecessary imports.
50-50: LGTM! Modernized type annotation.The type annotation has been updated to use lowercase
listinstead ofListin accordance with modern Python typing practices.
104-107: LGTM! Consistent type annotation updates.All type annotations have been properly updated to use lowercase types (
dictandlist) instead of their capitalized equivalents.
134-136: LGTM! Modernized type annotations.The type annotations have been updated to use lowercase types (
tuple,list, anddict) instead of their capitalized equivalents, making the code more aligned with modern Python typing practices.
148-148: LGTM! Modernized type annotation.The type annotation has been updated to use lowercase
dictinstead ofDictin accordance with modern Python typing practices.
176-178: LGTM! Modernized type annotation.The type annotation has been updated to use lowercase
listinstead ofListin accordance with modern Python typing practices.
179-179: LGTM! Modernized type annotation.The type annotation has been updated to use lowercase
dictinstead ofDictin accordance with modern Python typing practices.
204-205: LGTM! Modernized type annotations.All type annotations in the function signature have been properly updated to use lowercase types (
dict,list, andtuple) instead of their capitalized equivalents.
210-210: LGTM! Modernized type annotation.The type annotation has been updated to use lowercase
dictinstead ofDictin accordance with modern Python typing practices.
222-222: LGTM! Modernized type annotation.The type annotation has been updated to use lowercase
listinstead ofListin accordance with modern Python typing practices.
275-275: LGTM! Modernized type annotation.The type annotation has been updated to use lowercase
listinstead ofListin accordance with modern Python typing practices.
295-296: LGTM! Modernized type annotations.All type annotations in the function signature have been properly updated to use lowercase types (
dictandtuple) instead of their capitalized equivalents.
328-328: LGTM! Modernized return type using pipe syntax.The return type has been updated from
Optional[Github]toGithub | None, which is the newer Python 3.10+ syntax for union types.
| from typing import Any, Dict | ||
| import os | ||
| import sys | ||
| from typing import Any, Dict |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Type annotation should use lowercase types for consistency.
The type annotation still uses Dict instead of the lowercase dict. According to the PR summary, one of the goals is to modernize type annotations by replacing capitalized types with their lowercase equivalents.
-from typing import Any, Dict
+from typing import Any📝 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.
| from typing import Any, Dict | |
| from typing import Any |
| # self.github_app_id: str = get_value_from_dicts( | ||
| # primary_dict=repo_data, secondary_dict=config_data, key="github-app-id" | ||
| # ) |
|
|
||
| if not value: | ||
| value = secondary_dict.get(key) | ||
| if not value: |
There was a problem hiding this comment.
| if not value: | |
| if third_dict and not value: |
3 participants
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores