fix(github): resolve pull_request_number on retest for pushed commits#2473
Conversation
Summary of ChangesHello @zakisk, 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 fixes a critical bug where the Highlights
Changelog
Activity
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
|
There was a problem hiding this comment.
Code Review
This pull request fixes an issue where the pull_request_number variable was not available in /retest comments on pushed commits. The changes look good and include necessary unit and end-to-end tests. I have one suggestion to improve the robustness of the implementation against potential API delays.
pkg/provider/github/parse_payload.go
Outdated
| prs, err := v.getPullRequestsWithCommit(ctx, runevent.SHA, runevent.Organization, runevent.Repository, false) | ||
| if err != nil { | ||
| v.Logger.Warnf("Error getting pull requests associated with the commit in this commit comment event: %v", err) | ||
| } | ||
| if len(prs) > 0 { | ||
| runevent.PullRequestNumber = *prs[0].Number | ||
| } |
There was a problem hiding this comment.
To improve robustness and prevent potential flakiness, I have a couple of suggestions for this block:
-
Consider enabling the retry mechanism in
getPullRequestsWithCommitby passingtruefor theisMergeCommitparameter. A/retestis often used on merged commits, and the GitHub API can have a propagation delay. The associated E2E test even includes atime.Sleep(10 * time.Second)to account for this, which this change could help eliminate. The retry logic only engages if no PRs are found initially, so the performance impact is minimal. -
It's safer to use the
GetNumber()method to access the pull request number. This avoids a potential panic ifprs[0].Numberisniland is consistent with how other parts of the codebase access this value.
| prs, err := v.getPullRequestsWithCommit(ctx, runevent.SHA, runevent.Organization, runevent.Repository, false) | |
| if err != nil { | |
| v.Logger.Warnf("Error getting pull requests associated with the commit in this commit comment event: %v", err) | |
| } | |
| if len(prs) > 0 { | |
| runevent.PullRequestNumber = *prs[0].Number | |
| } | |
| prs, err := v.getPullRequestsWithCommit(ctx, runevent.SHA, runevent.Organization, runevent.Repository, true) | |
| if err != nil { | |
| v.Logger.Warnf("Error getting pull requests associated with the commit in this commit comment event: %v", err) | |
| } | |
| if len(prs) > 0 { | |
| runevent.PullRequestNumber = prs[0].GetNumber() | |
| } |
| g.Logger.Infof("Pull request %d has been merged", g.PRNumber) | ||
|
|
||
| // wait for API to reflect this PR in response | ||
| time.Sleep(10 * time.Second) |
There was a problem hiding this comment.
can't you add a wait for loop instead?
There was a problem hiding this comment.
what do you mean by wait loop? checking whether pull-request is in API or not??
There was a problem hiding this comment.
yes correct, with a timer to don't loop forever wiating it appears
- The {{ pull_request_number }} variable was not substituted
when a /retest command was issued on a pushed commit (e.g.
a commit resulting from a PR merge) because the commit
comment handler in ParsePayload did not fetch the
associated pull requests for the commit SHA.
- Fetch PRs via getPullRequestsWithCommit in
handleCommitCommentEvent so the PullRequestNumber is set
on the event before variable substitution runs.
- Add a unit test for commit_comment events to verify the
PullRequestNumber is populated from the associated PR.
- Add an e2e test that merges a PR, issues /retest on the
merged commit, and asserts the PipelineRun logs contain
the correct pull request number.
- Add a new testdata PipelineRun fixture that echoes the
{{ pull_request_number }} variable for validation.
https://issues.redhat.com/browse/SRVKP-10662
Signed-off-by: Zaki Shaikh <zashaikh@redhat.com>
6272ceb to
a0cac19
Compare
|
the e2e test I added in this PR is also flaky so fixing it (don't want another flake test) |
https://issues.redhat.com/browse/SRVKP-10662
📝 Description of the Change
👨🏻 Linked Jira
🔗 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.