fix: /ok-to-test /retest pipelineruns should not be created if last sha successful

[waveywaves]

Changes

when executing /ok-to-test or /retest gitops commands, check whether
the last pipelinerun created for the same SHA was successful. If the
run was successful, skip new pipelinerun creation

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:

  • GitHub App
  • GitHub Webhook
  • Gitea/Forgejo
  • GitLab
  • Bitbucket Cloud
  • Bitbucket Data Center

  (update the provider documentation accordingly)

[waveywaves]

/cancel

[Copilot]

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

pkg/pipelineascode/pipelineascode.go:196

• [nitpick] Consider renaming the variable 'existingPRs' (later in the block) to 'existingRuns' or similar to more clearly indicate that it represents a collection of PipelineRun objects.

labelSelector := fmt.Sprintf("%s=%s", keys.SHA, formatting.CleanValueKubernetes(p.event.SHA))

[waveywaves]

/retest

[waveywaves]

cc @chmouel seems like the provider e2e tests are not running :/ wanted to test them, not sure what's up
(none of the e2e tests are, they are being skipped)

[waveywaves]

cc @zakisk

[zakisk]

cc @chmouel seems like the provider e2e tests are not running :/ wanted to test them, not sure what's up (none of the e2e tests are, they are being skipped)

it may be happening due to changes @chmouel is making.

[chmouel]

@zakisk i think you created a utility function for that?

[zakisk]

not me, but there is func for this UntilPipelineRunCreated

[chmouel]

🤖 AI Review provided by gemini:gemini-2.5-pro-exp-03-25:

Okay, let's carefully analyze the provided diffs.

Overall Summary:

The core change introduces logic (checkForExistingSuccessfulPipelineRun in annotation_matcher.go) to prevent redundant PipelineRun creation when using /ok-to-test or /retest commands if a successful PipelineRun already exists for the target commit SHA. The tests have been updated accordingly, including adding a new unit test and modifying integration tests to potentially test specific retest functionality (/retest <pr-name>) and improve robustness.

Detailed Analysis:

1. pkg/matcher/annotation_matcher.go

   • New Functionality: The checkForExistingSuccessfulPipelineRun function is added.
     • Purpose: Correctly identifies if the event is a /retest or /ok-to-test and if a SHA is present.
     • Logic: It lists existing PipelineRuns matching the SHA label, finds the latest one based on CreationTimestamp, and checks if its Succeeded condition is True.
     • Return Value: Returns the existing successful PipelineRun if found, otherwise nil.
     • Integration: The MatchPipelinerunByAnnotation function now calls this new check. If a successful PR is found by the new function, it's returned immediately, effectively skipping the creation of a new one.
   • Imports: Necessary imports (formatting, metav1, apis) were added for the new functionality.
   • Potential Issues/Improvements:
     • Error Handling: The error from cs.Clients.Tekton.TektonV1().PipelineRuns(...).List(...) is checked (if err == nil), but if an error does occur, it's silently ignored, and the function returns nil. This means if listing fails, a new PipelineRun will be created, which might be unexpected. Consider logging the error:

       // Inside checkForExistingSuccessfulPipelineRun
       existingPRs, err := cs.Clients.Tekton.TektonV1().PipelineRuns(repo.GetNamespace()).List(ctx, metav1.ListOptions{
           LabelSelector: labelSelector,
       })
       if err != nil {
           // Add logging here
           logger.Warnf("Failed to list existing PipelineRuns for SHA %s in namespace %s: %v", event.SHA, repo.GetNamespace(), err)
           return nil // Keep original behavior, but now it's logged
       }
       if len(existingPRs.Items) > 0 {
         // ... rest of the logic
       }

     • Idiomatic Go: The code seems idiomatic. The logic for finding the latest PR by iterating and comparing CreationTimestamp is clear and correct. Using apis.ConditionSucceeded and IsTrue() is the standard way to check Tekton results.

2. pkg/matcher/annotation_matcher_test.go

   • New Test: TestCheckForExistingSuccessfulPipelineRun is added.
     • Coverage: It correctly sets up scenarios with a successful PR and an older failed PR for the same SHA.
     • Test Cases: It tests the relevant event types (/retest, /ok-to-test), a non-matching SHA, and an irrelevant event type.
     • Assertions: Uses assert correctly to check if a PR is found when expected and verifies it's the correct (successful) one.
   • Potential Issues/Improvements:
     • The test is well-structured and covers the main logic paths of the new function. No obvious issues.

3. test/gitea_test.go

   • Refactoring/Fixes:
     • Removed unused import sort.
     • The time.Sleep comments are just cosmetic changes.
     • In TestGiteaConfigCancelInProgress, the code removing sort.PipelineRunSortByStartTime and changing the expected number of PRs suggests the test logic was adapted, perhaps because the order isn't guaranteed or needed anymore.
     • Resetting cancelledPr = 0 before the second loop is a good practice for clarity and correctness.
     • Fetching a fresh list of PRs (prs, err = topts.ParamsRun...) before the final assertion is also a good improvement for test robustness, ensuring the state being checked is current.
     • Spelling Mistake Fixed: "tweo" corrected to "two" in the assertion message. assert.Equal(t, cancelledPr, 2, "two prs should have been canceled").
   • Potential Issues/Improvements:
     • time.Sleep calls are often necessary in integration tests but can sometimes lead to flakiness if timing is tight. Using explicit waits (twait.Until...) is generally preferred if a suitable condition can be formulated.

4. test/github_config_maxkeepruns_test.go

   • Behavior Change: The test now specifically targets a single PipelineRun for retesting.
     • It waits, lists existing PRs, finds one using the keys.OriginalPRName annotation, and then issues a /retest <original-pr-name> command.
   • Potential Issues/Improvements:
     • This change likely reflects an update to test the functionality of retesting a specific run rather than just a general /retest. This is a valid test scenario change.
     • The time.Sleep(5 * time.Second) is used to wait for the initial PR. Similar to the Gitea test, an explicit wait condition might be more robust if feasible.
     • The logic assumes a PR with the annotation will be found. This is usually safe in a controlled test flow but relies on the initial run starting correctly.

5. test/gitlab_merge_request_test.go

   • Behavior Change: Mirrors the change in the GitHub test.
     • It now finds an existing PR using keys.OriginalPRName and issues a /retest <original-pr-name> command via a GitLab note.
     • Crucially, the twait.SuccessOpt.OnEvent is updated from opscomments.RetestAllCommentEventType.String() to opscomments.RetestSingleCommentEventType.String(). This correctly aligns the waiting logic with the specific retest command being sent.
   • Potential Issues/Improvements:
     • Same points as the GitHub test regarding the use of specific retest and the time.Sleep.

Conclusion:

• Bugs: No obvious functional bugs were found in the core logic introduced in annotation_matcher.go or the accompanying unit test. A potential improvement would be to log the error if listing existing PipelineRuns fails in checkForExistingSuccessfulPipelineRun.
• Spelling Mistakes: One spelling mistake ("tweo") was identified and fixed in test/gitea_test.go.
• Code Improvements/Idiomatic Code:
  • The new Go code is generally idiomatic.
  • The test refactoring in test/gitea_test.go (resetting counter, re-fetching list) improves robustness.
  • The integration tests were updated to use specific /retest <pr-name> commands, which is likely intentional to test that feature. Using explicit waits instead of time.Sleep could be a future improvement for test stability, although sleeps are common in such tests.
  • Updating the expected event type in the GitLab test (RetestSingleCommentEventType) was a necessary and correct change alongside the command modification.

The changes appear to implement the intended feature correctly and improve test coverage and robustness, with only minor potential improvements suggested (error logging, potential sleep optimization).

[chmouel]

/e2e

[zakisk]

@chmouel again need an /e2e comment? E2E is skipped 🤔

[zakisk]

/e2e

[waveywaves]

/e2e

[waveywaves]

so the test is failing because github max keep runs is expecting the max number of pipelineruns to be 1, but there are two that it's seeing. The change in the test that I have made is that instead of running a / retest I am running "/ retest + name of the pipelinerun" failed, that shouldn't be causing any issues.