feat: Implement GraphQL batch fetching for .tekton files#2423
feat: Implement GraphQL batch fetching for .tekton files#2423chmouel merged 2 commits intotektoncd:mainfrom
Conversation
Summary of ChangesHello @chmouel, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant optimization to how Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
🔍 PR Lint Feedback
|
|
using a library for graphql library was actually more code than doing json dump and string, i tried to make it as compact as possible after running via LLM |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant performance improvement by replacing multiple REST API calls with a single batched GraphQL query to fetch .tekton files. However, a potential GraphQL injection vulnerability was identified in the construction of the GraphQL query where the ref variable is not properly escaped. This should be addressed by ensuring all variables inserted into the query string are correctly escaped for GraphQL string literals. Additionally, a critical bug in a test mock prevents compilation, and some Go 1.22-specific syntax in tests might affect compatibility with older Go versions.
b9946cd to
dd0fa67
Compare
| assert.NilError(t, err) | ||
|
|
||
| // Register handler once with all collected files (only if we have files) | ||
| if len(allFiles) > 0 { |
There was a problem hiding this comment.
this is complicated just to get tests done but i am not sure how to simplify it
dd0fa67 to
f891612
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements GraphQL batch fetching for .tekton directory files to replace individual REST API calls, reducing API request count and improving performance. The change introduces a new GraphQL client that fetches multiple YAML files in a single request using GitHub's GraphQL API with query aliases.
Changes:
- Introduced GraphQL batch fetching mechanism that reduces API calls from N+2 to 3 (where N is the number of YAML files)
- Added comprehensive GraphQL client with batching support (max 50 files per batch)
- Updated test infrastructure to mock GraphQL endpoints and validate the new functionality
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/provider/github/graphql.go | New GraphQL client implementation with endpoint construction, query building, and batch fetching logic |
| pkg/provider/github/github.go | Modified concatAllYamlFiles to use GraphQL batch fetching instead of individual REST API calls |
| pkg/provider/github/graphql_test.go | Unit tests for GraphQL endpoint construction, query building, and batch fetching |
| pkg/provider/github/github_test.go | Updated existing tests to reflect reduced API call counts and added GraphQL-specific integration tests |
| pkg/test/github/github.go | Enhanced test helper with GraphQL endpoint mocking and query parsing logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
9be2762 to
d0e3668
Compare
|
Follow-up since last review
|
|
we you take look on above comment we can final this |
|
Thanks @theakshaypant for catching the redundant logging and the rate limit header suggestion. Both have been implemented. |
|
I've addressed your concerns:
The metrics remain as a request counter (consistent with other providers), but the rate limit headers are logged separately to show actual consumption. |
|
I want to merge this after we do a release so we can dogfood it properly so let's wait a bit |
This commit introduces the use of GitHub's GraphQL API for fetching multiple `.tekton` directory files simultaneously. Previously, each file was fetched individually using REST API calls. The new implementation leverages GraphQL batching to retrieve all necessary YAML files in a single request, significantly reducing the number of API calls and improving performance. This change also necessitates new test cases to cover the GraphQL client functionality and ensures compatibility with GitHub Enterprise instances. Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
|
/retest |
|
/test |
|
/test go-testing |
Critical bug fix: Merged PRs were being incorrectly detected as "closed", causing tickets to transition to "In Progress" instead of "On QA". Problem: - pr.GetMerged() boolean is not reliably populated when listing PRs - Merged PRs have state="closed", so they were caught by the closed check - Example: tektoncd/pipelines-as-code#2423 detected as "closed" not "merged" Solution: - Check pr.MergedAt timestamp first (always set for merged PRs) - Only treat as closed if state="closed" AND MergedAt is nil/zero - More reliable detection using timestamp instead of boolean field Impact: - Merged PRs now correctly transition tickets to "On QA" - Closed (unmerged) PRs still correctly transition to "In Progress" Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Critical bug fix: Merged PRs were being incorrectly detected as "closed", causing tickets to transition to "In Progress" instead of "On QA". Problem: - pr.GetMerged() boolean is not reliably populated when listing PRs - Merged PRs have state="closed", so they were caught by the closed check - Example: tektoncd/pipelines-as-code#2423 detected as "closed" not "merged" Solution: - Check pr.MergedAt timestamp first (always set for merged PRs) - Only treat as closed if state="closed" AND MergedAt is nil/zero - More reliable detection using timestamp instead of boolean field Impact: - Merged PRs now correctly transition tickets to "On QA" - Closed (unmerged) PRs still correctly transition to "In Progress" Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This commit introduces the use of GitHub's GraphQL API for fetching multiple
.tektondirectory files simultaneously. Previously, each file was fetched individually using REST API calls.The new implementation leverages GraphQL batching to retrieve all necessary YAML files in a single request, significantly reducing the number of API calls and improving performance. This change also necessitates new test cases to cover the GraphQL client functionality and ensures compatibility with GitHub Enterprise instances.
📝 Description of the Change
👨🏻 Linked Jira
https://redhat.atlassian.net/browse/SRVKP-11470
🔗 Linked GitHub Issue
Fixes #
🚀 Type of Change
fix:)feat:)feat!:,fix!:)docs:)chore:)refactor:)enhance:)deps:)🧪 Testing Strategy
🤖 AI Assistance
If you have used AI assistance, please provide the following details:
Which LLM was used?
Extent of AI Assistance:
Important
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: Gemini gemini@google.com
Co-authored-by: ChatGPT noreply@chatgpt.com
Co-authored-by: Claude noreply@anthropic.com
Co-authored-by: Cursor noreply@cursor.com
Co-authored-by: Copilot Copilot@users.noreply.github.com
**💡You can use the script
./hack/add-llm-coauthor.shto automatically addthese co-author trailers to your commits.
✅ 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.