Fix 403 error on private repos (attempt)#4831
Conversation
Signed-off-by: Lukasz Gryglicki <lgryglicki@cncf.io> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
lukaszgryglicki
requested review from
ahmedomosanya,
jarias-lfx and
mlehotskylf
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughGetPRHeadSHA (Go) now falls back to listing PR commits with paging when PullRequests.Get fails to derive the head SHA; Python update_pull_request similarly guards head SHA retrieval and falls back to the latest PR commit. Minor log-format and shell usage-example edits included. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant GetPRHeadSHA
participant PullRequests
participant ListCommits
Caller->>GetPRHeadSHA: Request PR head SHA
GetPRHeadSHA->>PullRequests: PullRequests.Get(PR)
alt PullRequests.Get succeeds
PullRequests-->>GetPRHeadSHA: PR (head.sha)
GetPRHeadSHA-->>Caller: return head.sha
else PullRequests.Get fails
PullRequests--xGetPRHeadSHA: error
Note right of GetPRHeadSHA #f9f7d9: log warning, fallback to REST commits
GetPRHeadSHA->>ListCommits: ListCommits(PR, PerPage=1)
ListCommits-->>GetPRHeadSHA: commits (page 1) + pagination info
alt multiple pages
GetPRHeadSHA->>ListCommits: ListCommits(lastPage)
ListCommits-->>GetPRHeadSHA: commits(last page)
end
alt commit with SHA found
GetPRHeadSHA-->>Caller: return commit.sha
else no commits or missing SHA
GetPRHeadSHA-->>Caller: return error
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull Request Overview
Adds additional logging and fallback logic to retrieve the latest commit SHA for pull requests, aiming to mitigate 403 errors when accessing private repository data. Also adds a usage example comment to an AWS log search script.
- Adds a debug log and try/except around commit retrieval in update_pull_request with a fallback to pull_request.get_commits()
- Adds fallback logic in Go to derive PR head SHA via ListCommits when PullRequests.Get fails
- Adds an extra usage example comment in a utility script
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| utils/search_aws_log_group.sh | Adds an additional example search pattern comment (documentation only). |
| cla-backend/cla/models/github_models.py | Introduces error handling and fallback commit retrieval logic for PR updates. |
| cla-backend-go/github/github_repository.go | Adds fallback method to obtain PR head SHA with enhanced logging. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![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 (2)
utils/search_aws_log_group.sh (1)
4-4: Example LGTM; mirror quoting in DEBUG echo for clarity.The added example is useful. Consider aligning the DEBUG echo with the actual invocation so operators see exactly what runs.
Apply this diff to Line 90:
- echo "aws --region \"${REGION}\" --profile \"lfproduct-${STAGE}\" logs filter-log-events --log-group-name \"/aws/lambda/${log_group}\" --start-time \"${DTFROM}\" --end-time \"${DTTO}\" --filter-pattern \"${search}\"" + echo "aws --region \"${REGION}\" --profile \"lfproduct-${STAGE}\" logs filter-log-events --log-group-name \"/aws/lambda/${log_group}\" --start-time \"${DTFROM}\" --end-time \"${DTTO}\" --filter-pattern \"\\\"${search}\\\"\""cla-backend-go/github/github_repository.go (1)
1526-1550: Good fallback for PR head SHA; tighten edge handling.Works as intended. Two small hardening tweaks:
- Guard against a nil commit element.
- Always fetch the last page if available (>=1) to consistently return the true head when PerPage=1.
- opts := &github.ListOptions{PerPage: 1} - commits, resp, comErr := gh.PullRequests.ListCommits(ctx, owner, repo, prNumber, opts) + opts := &github.ListOptions{PerPage: 1} + commits, resp, comErr := gh.PullRequests.ListCommits(ctx, owner, repo, prNumber, opts) if comErr != nil { log.WithFields(f).WithError(comErr).Warnf("problem listing commits for repo: %s/%s pull request: %d", owner, repo, prNumber) return "", comErr } - if resp != nil && resp.LastPage > 1 { - opts.Page = resp.LastPage - commits, _, comErr = gh.PullRequests.ListCommits(ctx, owner, repo, prNumber, opts) + if resp != nil && resp.LastPage >= 1 { + // With PerPage=1, the last page contains the head commit. + opts.Page = resp.LastPage + commits, _, comErr = gh.PullRequests.ListCommits(ctx, owner, repo, prNumber, opts) if comErr != nil { log.WithFields(f).WithError(comErr).Warnf("problem listing commits for repo: %s/%s pull request: %d (last page)", owner, repo, prNumber) return "", comErr } } - if len(commits) == 0 || commits[0].SHA == nil { + if len(commits) == 0 || commits[0] == nil || commits[0].SHA == nil { return "", fmt.Errorf("missing head SHA for %s/%s PR #%d (via ListCommits)", owner, repo, prNumber) } return *commits[0].SHA, nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
cla-backend-go/github/github_repository.go(1 hunks)cla-backend/cla/models/github_models.py(2 hunks)utils/search_aws_log_group.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test-lint
- GitHub Check: cypress-functional
🔇 Additional comments (1)
cla-backend/cla/models/github_models.py (1)
2825-2825: Minor log formatting change LGTM.
Signed-off-by: Lukasz Gryglicki <lgryglicki@cncf.io> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cla-backend/cla/models/github_models.py (1)
2645-2656: Critical:.reversed[0]iterator indexing will fail—past review guidance not implemented.Past reviewers identified that
pull_request.get_commits().reversedreturns an iterator in PyGithub 1.55, which cannot be indexed with[0]. Line 2652 still uses this pattern, so the fallback will always raiseTypeErrorand hit the exception handler on Line 2654, settinglast_commit_sha = Noneand causing an early return on Line 2658.This defeats the purpose of the fallback—status updates will be skipped when they should succeed using the latest commit from the PR.
Apply the fix suggested in past reviews:
except (GithubException, AttributeError, TypeError) as exc: cla.log.error(f"{fn} - PR {pull_request.number}: exception getting head.sha: {exc}") try: - commit_obj = pull_request.get_commits().reversed[0] - last_commit_sha = commit_obj.sha + commits = pull_request.get_commits().reversed + commit_obj = next(iter(commits), None) + if commit_obj is None: + cla.log.error(f"{fn} - PR {pull_request.number}: no commits found in PR; cannot create statuses") + last_commit_sha = None + else: + last_commit_sha = commit_obj.sha except Exception as exc2: cla.log.error(f"{fn} - PR {pull_request.number}: exception getting last commit from PR commits: {exc2}") last_commit_sha = NoneThis properly consumes the iterator and handles empty commit lists.
Note: The static analysis warning about catching blind
Exceptionon Line 2654 is acceptable here since we're in a fallback path, logging the error, and safely degrading.Based on past reviews
🧹 Nitpick comments (1)
cla-backend-go/github/github_repository.go (1)
1526-1550: Fallback logic looks good; verify edge case handling.The fallback implementation correctly addresses 403 errors on private repos by using
ListCommitswhenPullRequests.Getfails. The pagination handling to reach the last page is sound.However, there's a subtle edge case on Line 1547: after checking
len(commits) == 0, you also checkcommits[0].SHA == nilin the same condition. Ifcommitsis empty, accessingcommits[0]would panic. While the short-circuit evaluation of||prevents this in Go (left side is true, right side won't evaluate), consider separating these checks for clarity:- if len(commits) == 0 || commits[0].SHA == nil { + if len(commits) == 0 { + return "", fmt.Errorf("no commits found for %s/%s PR #%d (via ListCommits)", owner, repo, prNumber) + } + if commits[0].SHA == nil { return "", fmt.Errorf("missing head SHA for %s/%s PR #%d (via ListCommits)", owner, repo, prNumber) }Also noting: The past review comment about error logging duplication has been properly addressed on Line 1532. ✓
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
cla-backend-go/github/github_repository.go(1 hunks)cla-backend/cla/models/github_models.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.14.0)
cla-backend/cla/models/github_models.py
2654-2654: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test-lint
- GitHub Check: cypress-functional
🔇 Additional comments (1)
cla-backend/cla/models/github_models.py (1)
2828-2828: LGTM: Log formatting fix.Correctly removed the extraneous space before the colon for cleaner log output.
Signed-off-by: Lukasz Gryglicki <lgryglicki@cncf.io> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
cla-backend/cla/models/github_models.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.14.0)
cla-backend/cla/models/github_models.py
2656-2656: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test-lint
- GitHub Check: cypress-functional
🔇 Additional comments (2)
cla-backend/cla/models/github_models.py (2)
2644-2644: LGTM!The debug log is helpful for tracing PR update flow and notification modes.
2830-2830: LGTM!Good catch on the formatting - removes the extra space for consistent log output.
Signed-off-by: Lukasz Gryglicki lgryglicki@cncf.io
Assisted by OpenAI
Assisted by GitHub Copilot