Use repo api token for webhooks and set repositories when available#686
Use repo api token for webhooks and set repositories when available#686
Conversation
|
Report bugs in Issues The following are automatically added:
Available user actions:
Supported /retest check runs
Supported labels
|
|
Caution Review failedThe pull request is closed. WalkthroughThe changes update the GitHub webhook handling components. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✨ Finishing Touches
🪧 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: 2
🧹 Nitpick comments (5)
webhook_server_container/utils/helpers.py (2)
30-37: Slightly more idiomatic existence checks.
While your solution is correct, you can consider usingif key in primary_dict:instead ofif primary_dict.get(key):to more explicitly distinguish between "value is None" vs. "key doesn't exist."- if primary_dict.get(key): - return primary_dict[key] - elif secondary_dict.get(key): - return secondary_dict[key] - else: - return return_on_none + if key in primary_dict: + return primary_dict[key] + elif key in secondary_dict: + return secondary_dict[key] + return return_on_none
150-151: Typographical fix suggestion.
The function name has a minor misspelling: “tokes” instead of “tokens.”-def get_apis_and_tokes_from_config(config: Config, repository_name: str = "") -> list[tuple[github.Github, str]]: +def get_apis_and_tokens_from_config(config: Config, repository_name: str = "") -> list[tuple[github.Github, str]]:webhook_server_container/utils/github_repository_settings.py (3)
414-428: Add error handling for API retrievalThe code retrieves APIs in parallel but doesn't handle potential exceptions that might occur during retrieval. This could lead to missing keys in the
apis_dict.apis: list = [] with ThreadPoolExecutor() as executor: for repo, data in config.data["repositories"].items(): apis.append( executor.submit( get_repository_api, - **{"repository": repo}, + **{"repository": repo, "config": config}, ) ) for result in as_completed(apis): - repository, github_api, api_user = result.result() - apis_dict[repository] = {"api": github_api, "user": api_user} + try: + repository, github_api, api_user = result.result() + if github_api: + apis_dict[repository] = {"api": github_api, "user": api_user} + else: + logger.error(f"Failed to get API for repository {repository}") + except Exception as e: + logger.error(f"Error retrieving API for repository: {e}")
235-254: Improve clarity between repository_name and full_repository_nameThe function uses two different variables for repository names, which could lead to confusion:
repository_name- passed as a parameter and used to look up in theapis_dictfull_repository_name- retrieved fromdata["name"]and used in log messagesConsider clarifying the naming or adding comments to explain the difference between these variables.
def set_repository( repository_name: str, data: Dict[str, Any], default_status_checks: List[str], apis_dict: dict[str, dict[str, Any]] ) -> Tuple[bool, str, Callable]: logger = get_logger_with_params(name="github-repository-settings") + # repository_name is the key in the config, full_repository_name includes the org/repo format full_repository_name: str = data["name"] logger.info(f"Processing repository {full_repository_name}") protected_branches: Dict[str, Any] = data.get("protected-branches", {}) repo_branch_protection_rules: Dict[str, Any] = data["branch_protection"] github_api = apis_dict[repository_name].get("api") api_user = apis_dict[repository_name].get("user", "")
432-433: Consider handling failures in the main execution flowsThe main execution flows
set_repositories_settingsandset_all_in_progress_check_runs_to_queueddon't have any error handling or reporting. Consider adding try/except blocks and returning appropriate exit codes if critical operations fail.-set_repositories_settings(config_=config, apis_dict=apis_dict) -set_all_in_progress_check_runs_to_queued(config_=config, apis_dict=apis_dict) +try: + set_repositories_settings(config_=config, apis_dict=apis_dict) + set_all_in_progress_check_runs_to_queued(config_=config, apis_dict=apis_dict) + logger.info("Repository settings update completed successfully") +except Exception as e: + logger.error(f"Failed to update repository settings: {e}") + sys.exit(1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
webhook_server_container/libs/github_api.py(2 hunks)webhook_server_container/utils/github_repository_settings.py(11 hunks)webhook_server_container/utils/helpers.py(8 hunks)webhook_server_container/utils/webhook.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: pre-commit
- GitHub Check: tox
- GitHub Check: build-container
- GitHub Check: python-module-install
🔇 Additional comments (37)
webhook_server_container/utils/helpers.py (14)
8-8: Use of built-in type hints is good.
Switching toAnyfrom thetypingmodule in a Python 3.9+ context is standard.
20-21: Consistent type annotations.
Usingdict[Any, Any]and anAnyreturn value is clear and matches your usage.Also applies to: 23-23
40-40: New function signature with optional parameter.
The addition ofrepository_nameis well-defined and properly typed.
43-43: Initialization ofrepo_data.
Defining a defaultdict[str, Any]keeps the code robust.
55-55: Refined type annotation for function arguments.
The shift fromDict[..., ...]to the built-indict[...]is a fine stylistic choice.
78-78: Modern union operator for type hints.
int | Noneis clean and readable.
83-83: Explicit tuple return type.
This clarifies the function’s possible outputs.
167-168: Augmented return type for additional context.
Including the_api_userin the returned tuple enhances traceability.
181-182: Initialization of variables.
apiandtokenset toNoneby default is fine.
184-184: Graceful handling ofrate_limit.
Setting it toNoneavoids potential uninitialized references.
188-189: Improved debug clarity.
A general message for retrieving API and token is helpful.
190-194: Conditional logging.
Appending the repository name in the debug message can prevent confusion.
207-207: Return structure expansion.
Returning(api, token, _api_user)is consistent with the earlier changes.
236-239: Extended docstring note.
The updated docstring clarifies the expected tuple shape for future results.webhook_server_container/libs/github_api.py (4)
116-118: Introduced retrieval ofapi_user.
Aligns with the refactor to include API user context in logs and operations.
120-122: Defensive check onself.github_apiandself.token.
Ensures referencing valid objects before proceeding.
123-126: Early return if GitHub API/token retrieval fails.
This prevents downstream null references.
207-207: Potential double re-initialization ofself.log_prefix.
Review if reassigningself.log_prefixhere and at line 127 is intentional. Re-setting the same variable again might cause confusion unless needed.Do you want me to generate a quick script to search for multiple
self.log_prefixassignments or do you have it covered?webhook_server_container/utils/webhook.py (18)
1-1: Use of ThreadPoolExecutor.
Import statements look fine; concurrency can improve performance when creating hooks.
4-4: New direct import ofgithub.
No issues, improves readability for GitHub-specific calls.
18-20: Function signature update addingapis_dict.
Refactoring to centralize API user retrieval is logical.
21-23: Dynamic retrieval of API and user.
Provides more flexibility for multi-repository scenarios.
25-26: Guard clause for missing API.
Immediate return helps avoid deeper failures.
27-27: Return message clarifies cause.
Returning the repository name aids debugging.
28-30: Additional user context in error messages.
Improves traceability when repositories are missing.
38-42: More precise exception message.
Including theapi_usercan pinpoint token permission issues quickly.
46-50: Immediate return on existing webhook.
Properly short-circuits redundant setup.
52-54: Logging new webhook creation.
Including events and user info is helpful.
56-57: Clear success message.
Including[API user ...]fosters consistent logging.
59-61: New helper function to get repository & user.
Automatically callsget_api_with_highest_rate_limitfor concurrency.
67-68: Dictionary to store APIs per repository.
apis_dictstructure is readable.
69-78: Concurrent retrieval of repository APIs.
Fetching them in parallel is efficient for large sets of repos.
79-82: Parsing future results intoapis_dict.
Threaded logic looks sound.
83-83: Detailed debug logging.
Helps confirm that all repositories have assigned API credentials.
87-87: Begin loop for processing each repo.
Parallel approach for hooking each repository is consistent with the design.
91-91: Reuse ofprocess_github_webhook.
Coherent approach to pass arguments via**.webhook_server_container/utils/github_repository_settings.py (1)
57-86:Details
✅ Verification successful
Verify the parameter usage in branch protection
The
api_userparameter is being used as individual bypass allowances for users, teams, and apps. However, these parameters expect lists. Make sureapi_useris correctly interpreted here.
🏁 Script executed:
#!/bin/bash # Check if api_user is used as a string or list in other places in the codebase rg -A 3 -B 3 "bypass_pull_request_allowances" --glob "*.py" # Look for documentation or usage examples of edit_protection method rg "edit_protection\(" --glob "*.py" -A 5 -B 5Length of output: 2209
Verification of Branch Protection Parameter Usage
After running the search commands, we confirmed that in our code the
api_userparameter is consistently used as a string which is then wrapped in a list (i.e.,[api_user]) for the bypass allowances. This aligns with the API’s expectation for these parameters to be lists. No changes are needed.
| def get_repository_api(repository: str) -> tuple[str, github.Github | None, str]: | ||
| github_api, _, api_user = get_api_with_highest_rate_limit(config=config, repository_name=repository) | ||
| return repository, github_api, api_user | ||
|
|
There was a problem hiding this comment.
Missing parameter in get_repository_api function
The function references a global config variable that's not passed as a parameter. This could lead to runtime errors.
-def get_repository_api(repository: str) -> tuple[str, github.Github | None, str]:
- github_api, _, api_user = get_api_with_highest_rate_limit(config=config, repository_name=repository)
+def get_repository_api(repository: str, config: Config) -> tuple[str, github.Github | None, str]:
+ github_api, _, api_user = get_api_with_highest_rate_limit(config=config, repository_name=repository)
return repository, github_api, api_user📝 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.
| def get_repository_api(repository: str) -> tuple[str, github.Github | None, str]: | |
| github_api, _, api_user = get_api_with_highest_rate_limit(config=config, repository_name=repository) | |
| return repository, github_api, api_user | |
| def get_repository_api(repository: str, config: Config) -> tuple[str, github.Github | None, str]: | |
| github_api, _, api_user = get_api_with_highest_rate_limit(config=config, repository_name=repository) | |
| return repository, github_api, api_user |
| for repo, data in config_.data["repositories"].items(): | ||
| futures.append( | ||
| executor.submit( | ||
| set_repository_check_runs_to_queued, | ||
| **{ | ||
| "config_": config_, | ||
| "data": data, | ||
| "github_api": github_api, | ||
| "github_api": apis_dict[repo]["api"], | ||
| "check_runs": check_runs, | ||
| "api_user": apis_dict[repo]["user"], | ||
| }, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate repository exists in apis_dict before accessing it
The code accesses the repository key directly in apis_dict without checking if it exists. This could lead to KeyError exceptions.
with ThreadPoolExecutor() as executor:
for repo, data in config_.data["repositories"].items():
+ if repo not in apis_dict:
+ logger.error(f"Repository {repo} not found in APIs dictionary. Skipping.")
+ continue
+ if "api" not in apis_dict[repo] or "user" not in apis_dict[repo]:
+ logger.error(f"Missing API or user for repository {repo}. Skipping.")
+ continue
futures.append(
executor.submit(
set_repository_check_runs_to_queued,
**{
"config_": config_,
"data": data,
"github_api": apis_dict[repo]["api"],
"check_runs": check_runs,
"api_user": apis_dict[repo]["user"],
},
)
)📝 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 repo, data in config_.data["repositories"].items(): | |
| futures.append( | |
| executor.submit( | |
| set_repository_check_runs_to_queued, | |
| **{ | |
| "config_": config_, | |
| "data": data, | |
| "github_api": github_api, | |
| "github_api": apis_dict[repo]["api"], | |
| "check_runs": check_runs, | |
| "api_user": apis_dict[repo]["user"], | |
| }, | |
| ) | |
| ) | |
| with ThreadPoolExecutor() as executor: | |
| for repo, data in config_.data["repositories"].items(): | |
| if repo not in apis_dict: | |
| logger.error(f"Repository {repo} not found in APIs dictionary. Skipping.") | |
| continue | |
| if "api" not in apis_dict[repo] or "user" not in apis_dict[repo]: | |
| logger.error(f"Missing API or user for repository {repo}. Skipping.") | |
| continue | |
| futures.append( | |
| executor.submit( | |
| set_repository_check_runs_to_queued, | |
| **{ | |
| "config_": config_, | |
| "data": data, | |
| "github_api": apis_dict[repo]["api"], | |
| "check_runs": check_runs, | |
| "api_user": apis_dict[repo]["user"], | |
| }, | |
| ) | |
| ) |
|
/verified |
|
New container for ghcr.io/myk-org/github-webhook-server:latest published |
2 participants
Summary by CodeRabbit