fix: /ok-to-test /retest pipelineruns should not be created if last sha successful #2048
Conversation
waveywaves
force-pushed
the
execute-non-successful
branch
9 times, most recently
from
fa2a595 to
72e4aca
Compare
|
/cancel |
waveywaves
force-pushed
the
execute-non-successful
branch
2 times, most recently
from
579fd77 to
7508871
Compare
waveywaves
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
waveywaves
changed the title
|
/retest |
waveywaves
force-pushed
the
execute-non-successful
branch
5 times, most recently
from
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 |
| @@ -10,7 +10,7 @@ The advantage of using a `GitOps command` is that it provides a journal of all t | |||
|
|
|||
| ## GitOps Commands on Pull Requests | |||
|
|
|||
| For example, when you are on a Pull Request, you may want to restart all your PipelineRuns. To do so, you can add a comment on your Pull Request starting with `/retest`, and all PipelineRuns attached to that Pull Request will be restarted. | |||
| For example, when you are on a Pull Request, you may want to restart failed PipelineRuns. To do so, you can add a comment on your Pull Request starting with `/retest`, and all **failed** PipelineRuns attached to that Pull Request will be restarted. If all previous PipelineRuns for the same commit were successful, no new PipelineRuns will be created to avoid unnecessary duplication. | |||
There was a problem hiding this comment.
| For example, when you are on a Pull Request, you may want to restart failed PipelineRuns. To do so, you can add a comment on your Pull Request starting with `/retest`, and all **failed** PipelineRuns attached to that Pull Request will be restarted. If all previous PipelineRuns for the same commit were successful, no new PipelineRuns will be created to avoid unnecessary duplication. | |
| For example, when you are on a Pull Request, you may want to trigger failed PipelineRuns. To do so, you can add a comment on your Pull Request starting with `/retest`, and all **failed** PipelineRuns attached to that Pull Request will be triggered. If all previous PipelineRuns for the same commit were successful, no new PipelineRuns will be triggered to avoid unnecessary duplication. |
There was a problem hiding this comment.
@waveywaves here and all other places in docs you've updated, I see you're mentioning only /retest command but /test does the same. do you mind adding it as well in examples and wherever needed?
There was a problem hiding this comment.
Mostly because this PR is only related to /retest, I have updated all the tests to use /test instead of /retest whereever required. You want examples of of both /test and /retest through the repo is what you are saying ?
| @@ -255,12 +270,12 @@ Here are the possible event types: | |||
|
|
|||
| * `test-all-comment`: The event is a single `/test` that would test every matched PipelineRun. | |||
| * `test-comment`: The event is a `/test <PipelineRun>` comment that would test a specific PipelineRun. | |||
| * `retest-all-comment`: The event is a single `/retest` that would retest every matched PipelineRun. | |||
| * `retest-all-comment`: The event is a single `/retest` that would retest every matched **failed** PipelineRun. If a successful PipelineRun already exists for the same commit, no new PipelineRun will be created. | |||
There was a problem hiding this comment.
| * `retest-all-comment`: The event is a single `/retest` that would retest every matched **failed** PipelineRun. If a successful PipelineRun already exists for the same commit, no new PipelineRun will be created. | |
| * `retest-all-comment`: The event is a single `/retest` that would retest every matched **failed** PipelineRun. If a successful PipelineRun already exists for the same commit, no new PipelineRun will be triggered. |
| @@ -72,8 +72,10 @@ get_tests() { | |||
| mapfile -t testfiles < <(find test/ -maxdepth 1 -name '*.go') | |||
| ghglabre="Github|Gitlab|Bitbucket" | |||
| if [[ ${target} == "providers" ]]; then | |||
| # echo "TestGithubMaxKeepRuns" | |||
There was a problem hiding this comment.
@waveywaves I think this is added mistakenly?
There was a problem hiding this comment.
I added this when I was trying to test individually in the CI, I'll remove this in the very end
|
/test linters |
zakisk
force-pushed
the
execute-non-successful
branch
from
e1d965f to
53877c2
Compare
|
/test linters |
|
@waveywaves linters is failing due to docs changes, please address them! |
|
@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 |
waveywaves
force-pushed
the
execute-non-successful
branch
5 times, most recently
from
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. |
/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 <[email protected]> Assisted-by: Claude-4-Sonnet (via Cursor)
waveywaves
force-pushed
the
execute-non-successful
branch
from
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.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
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.
| // If all templates were skipped, return a reference to the first successful one | ||
| if len(filteredPRs) == 0 && anySkipped && len(successfulTemplates) > 0 { | ||
| // Return any successful run as reference | ||
| for _, successfulPR := range successfulTemplates { | ||
| return []Match{{PipelineRun: successfulPR, Repo: repo}} | ||
| } | ||
| } |
There was a problem hiding this comment.
The current logic of returning an existing successful PipelineRun when all potential runs are skipped seems to have an unintended side effect. The calling function startPR will attempt to re-create this existing PipelineRun, which will fail with an "already exists" error. This will ultimately result in a failure status being reported on the commit, which is not the desired behavior of simply skipping the run.
To fix this, you should return an empty slice, which will prevent any new PipelineRun from being created and avoid the erroneous failure status. While this means the user won't get explicit feedback that the run was skipped, it correctly implements the primary goal of preventing duplicate runs without causing a failure.
// If all templates were skipped, return an empty slice to prevent new runs.
if len(filteredPRs) == 0 && anySkipped {
logger.Infof("all matched pipelineruns for sha %s have already succeeded, skipping", event.SHA)
return []Match{}
}Co-authored-by: Copilot <[email protected]>
|
/retest |
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
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)