fix: /ok-to-test /retest pipelineruns should not be created if last sha successful#2048
Conversation
fa2a595 to
72e4aca
Compare
|
/cancel |
579fd77 to
7508871
Compare
|
/retest |
8f5c0ab to
b44e208
Compare
|
cc @chmouel seems like the provider e2e tests are not running :/ wanted to test them, not sure what's up |
|
cc @zakisk |
|
@waveywaves I've tested it and it is not working as expected. I had 4 failed PipelineRuns in my test PR when I did Screencast.From.2025-08-12.15-44-20.mp4 |
4cb77c7 to
332b54e
Compare
|
@zakisk thank you for testing this, I have updated the PR and it works as expected now. Let me know what you think. |
4c32c59 to
092c22b
Compare
Screen.Recording.2025-08-15.at.7.54.16.PM.mov |
|
@zakisk do check the video uploaded above, it's the expected behavior you can see for only a failing pipelinerun being retriggered, let me know what you think |
|
/gemini review |
There was a problem hiding this comment.
Pull Request Overview
This PR implements a fix to prevent creating duplicate PipelineRuns when using /ok-to-test or /retest commands if a successful PipelineRun already exists for the same SHA, while maintaining the ability to force reruns with specific PipelineRun names.
- Added logic to check for existing successful PipelineRuns before creating new ones for
/retestand/ok-to-testcommands - Fixed the cancel-in-progress flow by short-circuiting PullRequestClosed events to avoid interference with matching logic
- Updated tests to use specific PipelineRun names for forced reruns and improved test reliability
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/matcher/annotation_matcher.go | Implemented filterSuccessfulTemplates logic to prevent duplicate runs for successful commits |
| pkg/matcher/annotation_matcher_test.go | Added comprehensive tests for the new checkForExistingSuccessfulPipelineRun functionality |
| pkg/pipelineascode/pipelineascode.go | Modified PullRequestClosed event handling to short-circuit matching logic |
| test/*.go | Updated test files to use /test instead of /retest or specific PipelineRun names for forced reruns |
| docs/content/docs/guide/*.md | Updated documentation to clarify new /retest behavior |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Code Review
This pull request introduces a significant improvement by preventing duplicate PipelineRuns for /retest and /ok-to-test commands when a successful run for the same SHA already exists. The changes are well-implemented across the board, with corresponding updates to documentation and tests to reflect the new behavior. The refactoring to handle PullRequestClosed events earlier is a nice cleanup.
I've found one potential issue in the filterSuccessfulTemplates function where returning an existing successful PipelineRun could lead to an incorrect failure status. I've left a specific comment with a suggestion to address this. Otherwise, the changes look great.
|
/retest |
/retest and /ok-to-test should only restart failed pipelines; the changes in the PR ensure that we do not match successful pipelineruns during invocation of these gitops commands. The tests have also been updated to use /test instead wherever /retest is being used to restart successful pipelines Signed-off-by: Vibhav Bobade <vibhav.bobde@gmail.com> Assisted-by: Claude-4-Sonnet (via Cursor)
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly implements the logic to prevent /retest and /ok-to-test commands from creating new PipelineRuns if a successful one already exists for the same SHA. The changes to the core logic in pkg/matcher/annotation_matcher.go are sound, and the accompanying documentation updates are clear and comprehensive. The test suite has also been updated to reflect this new behavior, which is great. I have two suggestions: one is to improve the unit testing for the new filtering logic to make it more robust, and the other is to fix a minor typo in a newly introduced constant.
Replace simplified test helper with comprehensive unit tests for the actual production function. This addresses PR feedback about inadequate test coverage of the core filtering logic. Changes: - Remove checkForExistingSuccessfulPipelineRun helper function - Remove TestCheckForExistingSuccessfulPipelineRun test - Add comprehensive TestFilterSuccessfulTemplates covering: * Multiple templates with different success/failure states * Per-template filtering based on most recent successful runs * Event type restrictions (retest/ok-to-test only) * Edge cases: empty SHA, different SHA, missing template names * All templates filtered resulting in empty list Coverage improvements: - filterSuccessfulTemplates function: 84.0% → 92.0% - Overall matcher package: 87.4% → 87.8% 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
|
I have tested it and it works nicely chmouel/scratchmyback#377 I have reviewed it and it look fine to me I have improved the testing in this commit via claude/gemin review and pushed the commit to that pull request |
|
/lgtm |
There was a problem hiding this comment.
Congrats @waveywaves 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 🧭
|
/retest |
|
/merge |
✅ PR Successfully Merged
Approvals Summary:
Thank you @waveywaves for your valuable contribution! 🎉 Automated by the PAC Boussole 🧭 |
Changes
/retest and /ok-to-test should only restart failed pipelines; the
changes in the PR ensure that we do not match successful pipelineruns
during invocation of these gitops commands. The tests have also been
updated to use /test instead wherever /retest is being used to restart
successful pipelines
fixes #1959
Jira: https://issues.redhat.com/browse/SRVKP-7236
Submitter Checklist
📝 Ensure your commit message is clear and informative. Refer to the How to write a git commit message guide. Include the commit message in the PR body rather than linking to an external site (e.g., Jira ticket).
♽ Run make test lint before submitting a PR to avoid unnecessary CI processing. Consider installing pre-commit and running pre-commit install in the repository root for an efficient workflow.
✨ We use linters to maintain clean and consistent code. Run make lint before submitting a PR. Some linters offer a --fix mode, executable with make fix-linters (ensure markdownlint and golangci-lint are installed).
📖 Document any user-facing features or changes in behavior.
🧪 While 100% coverage isn't required, we encourage unit tests for code changes where possible.
🎁 If feasible, add an end-to-end test. See README for details.
🔎 Address any CI test flakiness before merging, or provide a valid reason to bypass it (e.g., token rate limitations).
If adding a provider feature, fill in the following details:
(update the provider documentation accordingly)