feat(github): add update comment strategy for GitHub#2481
Conversation
🔍 PR Lint Feedback
|
Summary of ChangesHello @theakshaypant, 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 enhancement for GitHub integration by implementing an "update" comment strategy. This new feature consolidates PipelineRun status updates into a single, continuously updated comment on a pull request, rather than generating a new comment for each status change or re-execution. This change aims to improve the user experience by reducing notification volume and keeping the pull request conversation cleaner. 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
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new 'update' comment strategy for GitHub, allowing a single comment per PipelineRun to be updated, which helps reduce notification noise. However, a potential Markdown/HTML injection vulnerability exists because the user-controlled PipelineRun name is used without sanitization in HTML comment markers and the visible comment body, which could allow an attacker to inject malicious content or break out of the comment structure. Additionally, the review identified duplicated logic in pkg/provider/github/status.go, opportunities to improve E2E test stability by replacing time.Sleep with polling, and minor inconsistencies in regular expressions.
| defer g.TearDown(ctx, t) | ||
|
|
||
| g.Cnx.Clients.Log.Infof("Waiting for CEL error comment to be created") | ||
| time.Sleep(15 * time.Second) |
There was a problem hiding this comment.
Using time.Sleep can lead to flaky tests, especially in E2E environments where timing can vary. It would be more robust to use a polling mechanism with a timeout to wait for the expected condition (e.g., a comment to be created). The project's twait package might have helpers for this, or you could implement a simple polling loop. This applies to other time.Sleep calls in this file as well.
theakshaypant
left a comment
There was a problem hiding this comment.
Have not duplicated the code in 2446 so expect the CI to fail.
50e0920 to
bb9236d
Compare
bb9236d to
76c3181
Compare
|
/retest go-testing |
|
/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 🧭
|
can you update the PR description properly (ie as a sentence)? that would help the release note |
Done. Will keep that in mind for the future. |
|
For reference, e2e tests cover the scenarios described in #2446 (comment) |
Implement "update" comment strategy for GitHub pull requests that updates a single comment per PipelineRun instead of creating new comments on every trigger. Jira: https://issues.redhat.com/browse/SRVKP-10453 Signed-off-by: Akshay Pant <akshay.akshaypant@gmail.com>
fc71e3a to
65b8af8
Compare
|
/retest |
1 similar comment
|
/retest |
|
/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: 2
👥 Reviewers Who Approved:
| Reviewer | Permission Level | Approval Status |
|---|---|---|
| @zakisk | admin |
✅ |
| @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 🧭
| // 1. A CEL error comment is posted for a PLR | ||
| // 2. After fixing the CEL error with a new commit, the same comment is updated with success status | ||
| // 3. Only one comment exists. | ||
| func TestGithubCommentStrategyUpdateCELErrorReplacement(t *testing.T) { |
There was a problem hiding this comment.
@theakshaypant the test you've added is failing in E2E
There was a problem hiding this comment.
Needed a change after the rebase, fixed in 1e50ff9
Add E2E tests for update comment strategy: CEL error replacement, multiple PLRs, and regex character handling. Add UpdateFilesInRef helper for updating files in existing branches. Signed-off-by: Akshay Pant <akshay.akshaypant@gmail.com> Assisted-by: Claude Sonnet 4.5 (via Claude Code
Add documentation for update comment strategy option. This strategy creates a single comment per PipelineRun that gets updated with new status and commit SHA on re-execution. Signed-off-by: Akshay Pant <akshay.akshaypant@gmail.com>
65b8af8 to
7b1084b
Compare
📝 Description of the Change
This PR adds an
updatecomment strategy for GitHub pull requests. When thsi comment strategy is used, PAC creates a single comment per PipelineRun and update it with the latest status and commit SHA on re-execution.Changes:
updatecomment strategy.👨🏻 Linked Jira
https://issues.redhat.com/browse/SRVKP-10453
🔗 Linked GitHub Issue
N/A
🚀 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.