fix: don't restart successful pipelineruns on /retest /ok-to-test

/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)

Author: waveywaves

docs/content/_index.md

@@ -35,7 +35,7 @@ tracking using a Git workflow.
 
 <--->
 
-- Pull-request "*GitOps*" actions through comments with  `/retest`, `/test <pipeline-name>` and so on.
+- Pull-request "*GitOps*" actions through comments with `/retest` (reruns failed pipelines), `/test <pipeline-name>` (force rerun specific pipeline) and so on.
 
 - Automatic Task resolution in Pipelines (local Tasks, Artifact Hub, and remote URLs)
 

docs/content/docs/guide/gitops_commands.md

@@ -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.
 
 Example:
 
@@ -21,6 +21,23 @@ failure is not with your PR but seems to be an infrastructure issue.
 /retest
 ```
 
+The `/retest` command will only create new PipelineRuns if:
+
+- Previously **failed** PipelineRuns for the same commit, OR
+- No PipelineRun has been run for the same commit yet
+
+If a successful PipelineRun already exists for the same commit, `/retest` will **skip** triggering a new PipelineRun to avoid unnecessary duplication.
+
+**To force a rerun regardless of previous status**, use:
+
+```text
+/retest <pipelinerun-name>
+```
+
+This will always trigger a new PipelineRun, even if previous runs were successful.
+
+Similar to `/retest`, the `/ok-to-test` command will only trigger new PipelineRuns if no successful PipelineRun already exists for the same commit. This prevents duplicate runs when repository owners repeatedly test the same commit by `/test` and `/retest` command.
+
 If you have multiple `PipelineRun` and you want to target a specific `PipelineRun`, you can use the `/test` command followed by the specific PipelineRun name to restart it. Example:
 
 ```text
@@ -241,10 +258,10 @@ You can pass those `key=value` pairs anywhere in your comment, and they will be
 
 There are different formats that can be accepted, allowing you to pass values with spaces or newlines:
 
-* key=value
-* key="a value"
-* key="another \"value\" defined"
-* key="another
+- key=value
+- key="a value"
+- key="another \"value\" defined"
+- key="another
   value with newline"
 
 ## Event Type Annotation and Dynamic Variables
@@ -253,14 +270,14 @@ The `pipeline.tekton.dev/event-type` annotation indicates the type of GitOps com
 
 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-comment`: The event is a `/retest <PipelineRun>` that would retest a specific PipelineRun.
-* `on-comment`: The event is coming from a custom comment that would trigger a PipelineRun.
-* `cancel-all-comment`: The event is a single `/cancel` that would cancel every matched PipelineRun.
-* `cancel-comment`: The event is a `/cancel <PipelineRun>` that would cancel a specific PipelineRun.
-* `ok-to-test-comment`: The event is a `/ok-to-test` that would allow running the CI for an unauthorized user.
+- `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 **failed** PipelineRun. If a successful PipelineRun already exists for the same commit, no new PipelineRun will be created.
+- `retest-comment`: The event is a `/retest <PipelineRun>` that would retest a specific PipelineRun.
+- `on-comment`: The event is coming from a custom comment that would trigger a PipelineRun.
+- `cancel-all-comment`: The event is a single `/cancel` that would cancel every matched PipelineRun.
+- `cancel-comment`: The event is a `/cancel <PipelineRun>` that would cancel a specific PipelineRun.
+- `ok-to-test-comment`: The event is a `/ok-to-test` that would allow running the CI for an unauthorized user. If a successful PipelineRun already exists for the same commit, no new PipelineRun will be created.
 
 If a repository owner comments `/ok-to-test` on a pull request from an external contributor but no PipelineRun **matches** the `pull_request` event (or the repository has no `.tekton` directory), Pipelines-as-Code sets a **neutral** commit status. This indicates that no PipelineRun was matched, allowing other workflows—such as auto-merge—to proceed without being blocked.
 
@@ -272,12 +289,12 @@ Note: This neutral check-run status functionality is only supported on GitHub.
 
 When using the `{{ event_type }}` [dynamic variable]({{< relref "/docs/guide/authoringprs.md#dynamic-variables" >}}) for the following event types:
 
-* `test-all-comment`
-* `test-comment`
-* `retest-all-comment`
-* `retest-comment`
-* `cancel-all-comment`
-* `ok-to-test-comment`
+- `test-all-comment`
+- `test-comment`
+- `retest-all-comment`
+- `retest-comment`
+- `cancel-all-comment`
+- `ok-to-test-comment`
 
 The dynamic variable will return `pull_request` as the event type instead of the specific categorized GitOps command type. This is to handle backward compatibility with previous releases for users relying on this dynamic variable.
 

docs/content/docs/guide/policy.md

@@ -23,7 +23,7 @@ or other supported Git providers (currently GitHub and Gitea).
   to trigger the CI for a pull request by commenting `/ok-to-test`. This enables
   CI to run on pull requests submitted by contributors who are not collaborators
   of the repository or organization. It also applies to `/test` and `/retest`
-  commands. This action takes precedence over the `pull_request` action.
+  commands. Note that `/retest` will only trigger failed PipelineRuns. This action takes precedence over the `pull_request` action.
 
 ## Configuring Policies in the Repository CR
 

hack/gh-workflow-ci.sh

@@ -74,8 +74,10 @@ get_tests() {
   mapfile -t testfiles < <(find test/ -maxdepth 1 -name '*.go')
   ghglabre="Github|Gitlab|Bitbucket"
   if [[ ${target} == "providers" ]]; then
+    # echo "TestGithubMaxKeepRuns"
     grep -hioP "^func Test.*(${ghglabre})(\w+)\(" "${testfiles[@]}" | sed -e 's/func[ ]*//' -e 's/($//'
-  elif [[ ${target} == "gitea_others" ]]; then
+    elif [[ ${target} == "gitea_others" ]]; then
+    # echo "TestGiteaParamsOnRepoCR"
     grep -hioP '^func Test(\w+)\(' "${testfiles[@]}" | grep -iPv "(${ghglabre})" | sed -e 's/func[ ]*//' -e 's/($//'
   else
     echo "Invalid target: ${target}"

pkg/matcher/annotation_matcher.go

@@ -11,6 +11,7 @@ import (
 	apipac "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
 	pacerrors "github.com/openshift-pipelines/pipelines-as-code/pkg/errors"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/events"
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/formatting"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/opscomments"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/params"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/params/info"
@@ -21,6 +22,8 @@ import (
 	"github.com/google/cel-go/common/types"
 	tektonv1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
 	"go.uber.org/zap"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"knative.dev/pkg/apis"
 )
 
 const (
@@ -366,12 +369,120 @@ func MatchPipelinerunByAnnotation(ctx context.Context, logger *zap.SugaredLogger
 	}
 
 	if len(matchedPRs) > 0 {
+		// Filter out templates that already have successful PipelineRuns for /retest and /ok-to-test
+		if event.EventType == opscomments.RetestAllCommentEventType.String() ||
+			event.EventType == opscomments.OkToTestCommentEventType.String() {
+			return filterSuccessfulTemplates(ctx, logger, cs, event, repo, matchedPRs), nil
+		}
 		return matchedPRs, nil
 	}
 
 	return nil, fmt.Errorf("%s", buildAvailableMatchingAnnotationErr(event, pruns))
 }
 
+// filterSuccessfulTemplates filters out templates that already have successful PipelineRuns
+// when executing /ok-to-test or /retest gitops commands, implementing per-template checking.
+func filterSuccessfulTemplates(ctx context.Context, logger *zap.SugaredLogger, cs *params.Run, event *info.Event, repo *apipac.Repository, matchedPRs []Match) []Match {
+	if event.SHA == "" {
+		return matchedPRs
+	}
+
+	// Get all existing PipelineRuns for this SHA
+	labelSelector := fmt.Sprintf("%s=%s", keys.SHA, formatting.CleanValueKubernetes(event.SHA))
+	existingPRs, err := cs.Clients.Tekton.TektonV1().PipelineRuns(repo.GetNamespace()).List(ctx, metav1.ListOptions{
+		LabelSelector: labelSelector,
+	})
+	if err != nil {
+		logger.Errorf("failed to list existing PipelineRuns for SHA %s: %v", event.SHA, err)
+		return matchedPRs // Return all templates if we can't check
+	}
+
+	// Create a map of template names to their most recent successful run
+	successfulTemplates := make(map[string]*tektonv1.PipelineRun)
+
+	for i := range existingPRs.Items {
+		pr := &existingPRs.Items[i]
+
+		// Get the original template name this PipelineRun came from
+		originalPRName, ok := pr.GetAnnotations()[keys.OriginalPRName]
+		if !ok {
+			originalPRName, ok = pr.GetLabels()[keys.OriginalPRName]
+		}
+		if !ok {
+			continue // Skip PipelineRuns without template identification
+		}
+
+		// Check if this PipelineRun succeeded
+		if pr.Status.GetCondition(apis.ConditionSucceeded).IsTrue() {
+			// Keep the most recent successful run for each template
+			if existing, exists := successfulTemplates[originalPRName]; !exists ||
+				pr.CreationTimestamp.After(existing.CreationTimestamp.Time) {
+				successfulTemplates[originalPRName] = pr
+			}
+		}
+	}
+
+	// Filter out templates that have successful runs
+	var filteredPRs []Match
+	var anySkipped bool
+
+	for _, match := range matchedPRs {
+		templateName := getName(match.PipelineRun)
+
+		if successfulPR, hasSuccessfulRun := successfulTemplates[templateName]; hasSuccessfulRun {
+			logger.Infof("skipping template '%s' for sha %s as it already has a successful pipelinerun '%s'",
+				templateName, event.SHA, successfulPR.Name)
+			anySkipped = true
+		} else {
+			filteredPRs = append(filteredPRs, match)
+		}
+	}
+
+	// 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}}
+		}
+	}
+
+	return filteredPRs
+}
+
+// checkForExistingSuccessfulPipelineRun is a helper function for testing.
+// It checks if there's an existing successful PipelineRun for the same SHA.
+func checkForExistingSuccessfulPipelineRun(ctx context.Context, logger *zap.SugaredLogger, cs *params.Run, event *info.Event, repo *apipac.Repository) *tektonv1.PipelineRun {
+	// Only check for /retest and /ok-to-test commands
+	if event.EventType != opscomments.RetestAllCommentEventType.String() &&
+		event.EventType != opscomments.OkToTestCommentEventType.String() ||
+		event.SHA == "" {
+		return nil
+	}
+
+	labelSelector := fmt.Sprintf("%s=%s", keys.SHA, formatting.CleanValueKubernetes(event.SHA))
+	existingPRs, err := cs.Clients.Tekton.TektonV1().PipelineRuns(repo.GetNamespace()).List(ctx, metav1.ListOptions{
+		LabelSelector: labelSelector,
+	})
+	if err != nil {
+		logger.Errorf("failed to list existing PipelineRuns for SHA %s: %v", event.SHA, err)
+		return nil
+	}
+
+	// Find the most recent successful PipelineRun
+	var mostRecentSuccessfulPR *tektonv1.PipelineRun
+	for i := range existingPRs.Items {
+		pr := &existingPRs.Items[i]
+		if pr.Status.GetCondition(apis.ConditionSucceeded).IsTrue() {
+			if mostRecentSuccessfulPR == nil ||
+				pr.CreationTimestamp.After(mostRecentSuccessfulPR.CreationTimestamp.Time) {
+				mostRecentSuccessfulPR = pr
+			}
+		}
+	}
+
+	return mostRecentSuccessfulPR
+}
+
 func buildAvailableMatchingAnnotationErr(event *info.Event, pruns []*tektonv1.PipelineRun) string {
 	errmsg := "available annotations of the PipelineRuns annotations in .tekton/ dir:"
 	for _, prun := range pruns {