Chore: [AEA-0000] - get fork pr contributor approval#67
Conversation
|
This PR is linked to a ticket in an NHS Digital JIRA Project. Here's a handy link to the ticket: AEA-0000 |
There was a problem hiding this comment.
Pull request overview
Adds reporting of a repository’s “fork PR contributor approval” Actions permission to the existing get_repo_status export, so the generated JSON includes this security-related setting per repo.
Changes:
- Extend
GithubDataClientwithget_fork_pr_contributor_approval()to call the GitHub REST endpoint for the setting. - Include the returned
fork_pr_contributor_approvalvalue in the CLI’s JSON output. - Tighten
_safe_get_reporeturn type toOptional[Repository.Repository].
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/get_repo_status/app/github_client.py | Adds a client method to query the fork PR contributor approval policy and updates repo typing. |
| packages/get_repo_status/app/cli.py | Adds the new policy value into the exported per-repo JSON payload. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| approval_policy = gh_repo.requester.requestJsonAndCheck( | ||
| "GET", gh_repo.url + "/actions/permissions/fork-pr-contributor-approval" | ||
| ) | ||
| return approval_policy[1]["approval_policy"] |
There was a problem hiding this comment.
get_fork_pr_contributor_approval catches all exceptions and will print/log even for common GitHub API cases like 404/403. Other methods in this client treat GithubException specially (e.g., suppress 404 noise) and return a non-error sentinel. Consider catching GithubException explicitly here (suppressing expected 404s) and returning a consistent value (e.g., "N/A" or None) when the endpoint isn’t available or permitted, while keeping real failures as "Error".
| return approval_policy[1]["approval_policy"] | |
| return approval_policy[1]["approval_policy"] | |
| except GithubException as exc: | |
| status = getattr(exc, "status", None) | |
| if status in (403, 404): | |
| # Endpoint not available or access not permitted; treat as not applicable. | |
| return "N/A" | |
| print(f"Error fetching fork_pr_contributor_approval {repo_name}: {exc}") | |
| return "Error" |
| ) | ||
| return approval_policy[1]["approval_policy"] | ||
| except Exception as exc: # pylint: disable=broad-except | ||
| print(f"Error fetching fork_pr_contributor_approval {repo_name}: {exc}") |
There was a problem hiding this comment.
The error message text here is missing a preposition and is a bit hard to read in logs ("Error fetching fork_pr_contributor_approval {repo_name}"). Consider adjusting it to include "for" and/or quoting the repo name so log lines are consistent with the rest of the file.
| print(f"Error fetching fork_pr_contributor_approval {repo_name}: {exc}") | |
| print(f'Error fetching fork_pr_contributor_approval for "{repo_name}": {exc}') |
| def get_fork_pr_contributor_approval(self, repo: Repo) -> str: | ||
| repo_name = repo["repoUrl"] | ||
| gh_repo = self._safe_get_repo(repo_name) | ||
| if gh_repo is None: | ||
| return "Error" | ||
| try: | ||
| approval_policy = gh_repo.requester.requestJsonAndCheck( | ||
| "GET", gh_repo.url + "/actions/permissions/fork-pr-contributor-approval" | ||
| ) | ||
| return approval_policy[1]["approval_policy"] |
There was a problem hiding this comment.
This new GitHub API integration doesn’t have unit tests yet. The repo already has good coverage for other GithubDataClient methods in packages/get_repo_status/tests/test_github_client.py; please add tests covering the success path (mocking requestJsonAndCheck) and at least one failure path (e.g., GithubException(404) or a generic exception) to ensure the CLI output is stable.
Summary
Details