fix: /ok-to-test /retest pipelineruns should not restart successful pipelineruns

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

- Add checkForExistingSuccessfulPipelineRun logic to prevent creating duplicate
  PipelineRuns when /retest or /ok-to-test commands are used and a
  successful PipelineRun already exists for the same SHA
- Fix cancel-in-progress flow by short-circuiting PullRequestClosed events
  to avoid interference with matching logic
- Ensure /retest <pipelinerun-name> commands always create new PipelineRuns
  as explicitly requested by users

- Add GetOriginalPipelineRunName helper function to eliminate duplicate
  logic across test files for finding PipelineRun names
- Fix multiple tests that incorrectly used plain /retest expecting multiple
  PipelineRuns: now use /retest <name> for forced reruns
- Update max-keep-runs tests to use specific PipelineRun names for proper
  testing of cleanup functionality
- Fix timing issues in tests by adding proper waits for Repository CRD updates

Author: waveywaves

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,51 @@ func MatchPipelinerunByAnnotation(ctx context.Context, logger *zap.SugaredLogger
 	}
 
 	if len(matchedPRs) > 0 {
+		if existingPR := checkForExistingSuccessfulPipelineRun(ctx, logger, cs, event, repo); existingPR != nil {
+			return []Match{{PipelineRun: existingPR, Repo: repo}}, nil
+		}
 		return matchedPRs, nil
 	}
 
 	return nil, fmt.Errorf("%s", buildAvailableMatchingAnnotationErr(event, pruns))
 }
 
+// checkForExistingSuccessfulPipelineRun checks if there's an existing successful PipelineRun for the same SHA
+// when executing /ok-to-test or /retest gitops commands.
+func checkForExistingSuccessfulPipelineRun(ctx context.Context, logger *zap.SugaredLogger, cs *params.Run, event *info.Event, repo *apipac.Repository) *tektonv1.PipelineRun {
+	// Only apply this logic to /retest (without specific PR name) and /ok-to-test commands
+	// /retest <pipelinerun-name> should always create a new run since user explicitly requested it
+	if (event.EventType == opscomments.RetestAllCommentEventType.String() ||
+		event.EventType == opscomments.OkToTestCommentEventType.String()) &&
+		event.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 nil
+		}
+		if len(existingPRs.Items) > 0 {
+			var lastRun tektonv1.PipelineRun
+			lastRun = existingPRs.Items[0]
+
+			for _, pr := range existingPRs.Items {
+				if pr.CreationTimestamp.After(lastRun.CreationTimestamp.Time) {
+					lastRun = pr
+				}
+			}
+
+			if lastRun.Status.GetCondition(apis.ConditionSucceeded).IsTrue() {
+				logger.Infof("skipping creation of new pipelinerun for sha %s as the last pipelinerun '%s' has already succeeded",
+					event.SHA, lastRun.Name)
+				return &lastRun
+			}
+		}
+	}
+	return nil
+}
+
 func buildAvailableMatchingAnnotationErr(event *info.Event, pruns []*tektonv1.PipelineRun) string {
 	errmsg := "available annotations of the PipelineRuns annotations in .tekton/ dir:"
 	for _, prun := range pruns {

pkg/matcher/annotation_matcher_test.go

@@ -30,7 +30,10 @@ import (
 	zapobserver "go.uber.org/zap/zaptest/observer"
 	"gotest.tools/v3/assert"
 	"gotest.tools/v3/golden"
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"knative.dev/pkg/apis"
+	knativeduckv1 "knative.dev/pkg/apis/duck/v1"
 	rtesting "knative.dev/pkg/reconciler/testing"
 )
 
@@ -2599,3 +2602,130 @@ func TestGetName(t *testing.T) {
 		})
 	}
 }
+
+func TestCheckForExistingSuccessfulPipelineRun(t *testing.T) {
+	ctx, _ := rtesting.SetupFakeContext(t)
+	logger := zap.NewExample().Sugar()
+
+	repo := &v1alpha1.Repository{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-repo",
+			Namespace: "test-ns",
+		},
+	}
+
+	// Create a successful PipelineRun
+	pr := &tektonv1.PipelineRun{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-pr",
+			Namespace: "test-ns",
+			Labels: map[string]string{
+				keys.SHA: "test-sha",
+			},
+			CreationTimestamp: metav1.Now(),
+		},
+		Status: tektonv1.PipelineRunStatus{
+			Status: knativeduckv1.Status{
+				Conditions: knativeduckv1.Conditions{
+					apis.Condition{
+						Type:   apis.ConditionSucceeded,
+						Status: corev1.ConditionTrue,
+					},
+				},
+			},
+		},
+	}
+
+	// Create a failed PipelineRun with the same SHA but older
+	earlierTime := metav1.NewTime(time.Now().Add(-1 * time.Hour))
+	failedPR := &tektonv1.PipelineRun{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "failed-pr",
+			Namespace: "test-ns",
+			Labels: map[string]string{
+				keys.SHA: "test-sha",
+			},
+			CreationTimestamp: earlierTime,
+		},
+		Status: tektonv1.PipelineRunStatus{
+			Status: knativeduckv1.Status{
+				Conditions: knativeduckv1.Conditions{
+					apis.Condition{
+						Type:   apis.ConditionSucceeded,
+						Status: corev1.ConditionFalse,
+					},
+				},
+			},
+		},
+	}
+
+	// Setup test clients
+	tdata := testclient.Data{
+		PipelineRuns: []*tektonv1.PipelineRun{pr, failedPR},
+		Repositories: []*v1alpha1.Repository{repo},
+	}
+	stdata, _ := testclient.SeedTestData(t, ctx, tdata)
+
+	cs := &params.Run{
+		Clients: clients.Clients{
+			Log:    logger,
+			Tekton: stdata.Pipeline,
+			Kube:   stdata.Kube,
+		},
+	}
+
+	tests := []struct {
+		name      string
+		eventType string
+		sha       string
+		wantPR    bool
+	}{
+		{
+			name:      "Retest command with matching SHA should find successful PR",
+			eventType: opscomments.RetestAllCommentEventType.String(),
+			sha:       "test-sha",
+			wantPR:    true,
+		},
+		{
+			name:      "Ok-to-test command with matching SHA should find successful PR",
+			eventType: opscomments.OkToTestCommentEventType.String(),
+			sha:       "test-sha",
+			wantPR:    true,
+		},
+		{
+			name:      "Retest command with non-matching SHA should not find PR",
+			eventType: opscomments.RetestAllCommentEventType.String(),
+			sha:       "other-sha",
+			wantPR:    false,
+		},
+		{
+			name:      "Different event type should not find PR",
+			eventType: opscomments.TestAllCommentEventType.String(),
+			sha:       "test-sha",
+			wantPR:    false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			event := &info.Event{
+				EventType: tt.eventType,
+				SHA:       tt.sha,
+			}
+
+			foundPR := checkForExistingSuccessfulPipelineRun(ctx, logger, cs, event, repo)
+
+			if tt.wantPR && foundPR == nil {
+				t.Errorf("Expected to find a successful PipelineRun, but got nil")
+			}
+
+			if !tt.wantPR && foundPR != nil {
+				t.Errorf("Expected not to find a PipelineRun, but found %s", foundPR.Name)
+			}
+
+			if tt.wantPR && foundPR != nil {
+				assert.Equal(t, "test-pr", foundPR.Name)
+			}
+		})
+	}
+}

pkg/pipelineascode/pipelineascode.go

@@ -55,13 +55,21 @@ func NewPacs(event *info.Event, vcx provider.Interface, run *params.Run, pacInfo
 }
 
 func (p *PacRun) Run(ctx context.Context) error {
-	matchedPRs, repo, err := p.matchRepoPR(ctx)
-	if repo != nil && p.event.TriggerTarget == triggertype.PullRequestClosed {
-		if err := p.cancelAllInProgressBelongingToClosedPullRequest(ctx, repo); err != nil {
-			return fmt.Errorf("error cancelling in progress pipelineRuns belonging to pull request %d: %w", p.event.PullRequestNumber, err)
+	// For PullRequestClosed events, skip matching logic and go straight to cancellation
+	if p.event.TriggerTarget == triggertype.PullRequestClosed {
+		repo, err := p.verifyRepoAndUser(ctx)
+		if err != nil {
+			return err
+		}
+		if repo != nil {
+			if err := p.cancelAllInProgressBelongingToClosedPullRequest(ctx, repo); err != nil {
+				return fmt.Errorf("error cancelling in progress pipelineRuns belonging to pull request %d: %w", p.event.PullRequestNumber, err)
+			}
 		}
 		return nil
 	}
+
+	matchedPRs, repo, err := p.matchRepoPR(ctx)
 	if err != nil {
 		createStatusErr := p.vcx.CreateStatus(ctx, p.event, provider.StatusOpts{
 			Status:     CompletedStatus,

test/gitea_gitops_commands_test.go

@@ -192,16 +192,22 @@ func TestGiteaRetestAll(t *testing.T) {
 	assert.NilError(t, secret.Create(ctx, topts.ParamsRun, map[string]string{"secret": "SHHHHHHH"}, topts.TargetNS, "pac-secret"))
 	topts.TargetEvent = triggertype.PullRequest.String()
 	topts.YAMLFiles = map[string]string{
-		".tekton/pr.yaml":      "testdata/pipelinerun.yaml",
-		".tekton/nomatch.yaml": "testdata/pipelinerun-nomatch.yaml",
+		".tekton/pr.yaml":        "testdata/pipelinerun.yaml",
+		".tekton/pr-second.yaml": "testdata/pipelinerun-clone.yaml", // Use a matching PipelineRun instead of nomatch
 	}
 	_, f := tgitea.TestPR(t, topts)
 	defer f()
-	tgitea.PostCommentOnPullRequest(t, topts, "/retest")
+
+	// Wait for initial PipelineRuns to complete, then get one to retest specifically
+	time.Sleep(5 * time.Second)
+	pipelineRunName, err := twait.GetOriginalPipelineRunName(context.Background(), topts.ParamsRun.Clients, topts.TargetNS, topts.PullRequest.Head.Sha)
+	assert.NilError(t, err)
+
+	tgitea.PostCommentOnPullRequest(t, topts, "/retest "+pipelineRunName)
 	waitOpts := twait.Opts{
 		RepoName:        topts.TargetNS,
 		Namespace:       topts.TargetNS,
-		MinNumberStatus: 2,
+		MinNumberStatus: 3, // 2 initial + 1 retest
 		PollTimeout:     twait.DefaultTimeout,
 	}
 
@@ -214,6 +220,6 @@ func TestGiteaRetestAll(t *testing.T) {
 			rt = true
 		}
 	}
-	assert.Assert(t, rt, "should have a retest all comment event in status")
-	assert.Equal(t, len(repo.Status), 2, "should have only 2 status")
+	assert.Assert(t, rt, "should have a retest comment event in status")
+	assert.Equal(t, len(repo.Status), 3, "should have 3 statuses: 2 initial + 1 retest")
 }

test/gitea_params_test.go

@@ -314,7 +314,15 @@ func TestGiteaParamsOnRepoCR(t *testing.T) {
 	_, f := tgitea.TestPR(t, topts)
 	defer f()
 
-	repo, err := topts.ParamsRun.Clients.PipelineAsCode.PipelinesascodeV1alpha1().Repositories(topts.TargetNS).Get(context.Background(), topts.TargetNS, metav1.GetOptions{})
+	// Wait for Repository status to be updated
+	waitOpts := twait.Opts{
+		RepoName:        topts.TargetNS,
+		Namespace:       topts.TargetNS,
+		MinNumberStatus: 1,
+		PollTimeout:     twait.DefaultTimeout,
+		TargetSHA:       "",
+	}
+	repo, err := twait.UntilRepositoryUpdated(context.Background(), topts.ParamsRun.Clients, waitOpts)
 	assert.NilError(t, err)
 	assert.Assert(t, len(repo.Status) != 0)
 	assert.NilError(t,