perf(github): cache getPullRequest result in Provider#2621
perf(github): cache getPullRequest result in Provider#2621theakshaypant merged 1 commit intotektoncd:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the GitHub provider's ACL check to use cached pull request data, reducing API calls. The checkPullRequestForSameURL function was updated to compare branch and URL information directly from the event object. Feedback includes suggestions to rename variables and update function comments to accurately reflect that the implementation now checks repository URLs instead of specifically clone URLs.
|
Linked the results for duplicate API calls in the PR description. The reference list for duplications can be found in the parent of the linked issue. |
|
|
||
| // getPullRequest get a pull request details. | ||
| // getPullRequest get a pull request details, caching the result for the lifetime of the event. | ||
| func (v *Provider) getPullRequest(ctx context.Context, runevent *info.Event) (*info.Event, error) { |
There was a problem hiding this comment.
i see there is no unit test for getPullRequest so can you please write tests for it covering this scenario as well?
Cache the GitHub API response for PR lookups to avoid repeated calls within the same event lifecycle. ACL checks now read from already-populated runevent fields instead of fetching independently. Fixes tektoncd#2378 Signed-off-by: Akshay Pant <akpant@redhat.com>
e191f23 to
acaa8bc
Compare
|
/lgtm |
There was a problem hiding this comment.
Congrats @theakshaypant your PR Has been approved 🎉
✅ Pull Request Approved
Approval Status:
- Required Approvals: 1
- Current Approvals: 1
👥 Reviewers Who Approved:
| Reviewer | Permission Level | Approval Status |
|---|---|---|
| @chmouel | admin |
✅ |
📝 Next Steps
- Ensure all required checks pass
- Comply with branch protection rules
- Request a maintainer to merge using the
/mergecommand (or merge it
directly if you have repository permission).
Automated by the PAC Boussole 🧭
📝 Description of the Change
Cache the GitHub API response for PR lookups to avoid repeated calls within the same event lifecycle. ACL checks now read from already-populated runevent fields instead of fetching independently.
🔗 Linked GitHub Issue
Fixes #2378
🧪 Testing Strategy
Ran the same script to fetch duplicates API calls per event-id on the github_ghe test suite and there were no repeated
get_pull_requestcalls.All repeated API calls in the test suite for this PR's CI
🤖 AI Assistance
AI assistance can be used for various tasks, such as code generation,
documentation, or testing.
Please indicate whether you have used AI assistance
for this PR and provide details if applicable.
Important
Slop will be simply rejected, if you are using AI assistance you need to make sure you
understand the code generated and that it meets the project's standards. you
need at least know how to run the code and deploy it (if needed). See
startpaac to make it easy
to deploy and test your code changes.
If the majority of the code in this PR was generated by an AI, please add a
Co-authored-bytrailer to your commit message.For example:
Co-authored-by: Claude noreply@anthropic.com
✅ Submitter Checklist
fix:,feat:) matches the "Type of Change" I selected above.make testandmake lintlocally to check for and fix anyissues. For an efficient workflow, I have considered installing
pre-commit and running
pre-commit installtoautomate these checks.