fix: Avoid webhook feedback loop on no-ops comment events (#2504)

Added logic to prevent errors from no-ops comment events from creating a
feedback loop. When a "no-ops" comment event occurs, the MetadataResolve
error is now skipped, preventing the creation of a comment that would
re-trigger the same event.

The test case for Gitea was updated to account for this change. It now
waits for a short period to allow any potential feedback loops to settle
and then asserts that only a single failure comment was posted, rather
than multiple duplicates.

Jira: https://issues.redhat.com/browse/SRVKP-10912

Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
Co-authored-by: Zaki Shaikh <zashaikh@redhat.com>

Author: chmouel

pkg/pipelineascode/match.go

@@ -252,6 +252,12 @@ func (p *PacRun) getPipelineRunsFromRepo(ctx context.Context, repo *v1alpha1.Rep
 	p.debugf("getPipelineRunsFromRepo: pipelineRuns count=%d", len(pipelineRuns))
 	pipelineRuns, err = resolve.MetadataResolve(pipelineRuns)
 	if err != nil && len(pipelineRuns) == 0 {
+		// Don't report errors for no-ops comment events to avoid a webhook feedback loop:
+		// reporting creates a comment, which triggers another webhook, which hits the same error.
+		if p.event.EventType == opscomments.NoOpsCommentEventType.String() {
+			p.logger.Infof("skipping MetadataResolve error for no-ops comment event: %s", err)
+			return nil, nil
+		}
 		p.eventEmitter.EmitMessage(repo, zap.ErrorLevel, "FailedToResolvePipelineRunMetadata", err.Error())
 		return nil, err
 	}

pkg/pipelineascode/match_test.go

@@ -316,13 +316,26 @@ func TestGetPipelineRunsFromRepo(t *testing.T) {
 		},
 	}
 
+	noOpsCommentEvent := &info.Event{
+		SHA:           "principale",
+		Organization:  "organizationes",
+		Repository:    "lagaffe",
+		URL:           "https://service/documentation",
+		HeadBranch:    "main",
+		BaseBranch:    "main",
+		Sender:        "fantasio",
+		EventType:     opscomments.NoOpsCommentEventType.String(),
+		TriggerTarget: "pull_request",
+	}
+
 	tests := []struct {
 		name                  string
 		repositories          *v1alpha1.Repository
 		tektondir             string
 		expectedNumberOfPruns int
 		event                 *info.Event
 		logSnippet            string
+		wantErr               bool
 	}{
 		{
 			name: "more than one pipelinerun in .tekton dir",
@@ -422,6 +435,33 @@ func TestGetPipelineRunsFromRepo(t *testing.T) {
 			expectedNumberOfPruns: 0,
 			event:                 okToTestEvent,
 		},
+		{
+			name: "same name pipelineruns error on regular event",
+			repositories: &v1alpha1.Repository{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "testrepo",
+					Namespace: "test",
+				},
+				Spec: v1alpha1.RepositorySpec{},
+			},
+			tektondir: "testdata/same_name_pipelineruns",
+			event:     pullRequestEvent,
+			wantErr:   true,
+		},
+		{
+			name: "same name pipelineruns skipped on no-ops comment event",
+			repositories: &v1alpha1.Repository{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "testrepo",
+					Namespace: "test",
+				},
+				Spec: v1alpha1.RepositorySpec{},
+			},
+			tektondir:             "testdata/same_name_pipelineruns",
+			expectedNumberOfPruns: 0,
+			event:                 noOpsCommentEvent,
+			logSnippet:            "skipping MetadataResolve error for no-ops comment event",
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -464,6 +504,10 @@ func TestGetPipelineRunsFromRepo(t *testing.T) {
 			p := NewPacs(tt.event, vcx, cs, pacInfo, k8int, logger, nil)
 			p.eventEmitter = events.NewEventEmitter(stdata.Kube, logger)
 			matchedPRs, err := p.getPipelineRunsFromRepo(ctx, tt.repositories)
+			if tt.wantErr {
+				assert.Assert(t, err != nil, "expected an error but got nil")
+				return
+			}
 			assert.NilError(t, err)
 			matchedPRNames := make([]string, len(matchedPRs))
 			for i := range matchedPRs {

pkg/pipelineascode/testdata/same_name_pipelineruns/.tekton/run.yaml

@@ -0,0 +1,33 @@
+apiVersion: tekton.dev/v1beta1
+kind: PipelineRun
+metadata:
+  name: duplicate-pr
+  annotations:
+    pipelinesascode.tekton.dev/on-target-branch: "[main]"
+    pipelinesascode.tekton.dev/on-event: "[pull_request]"
+spec:
+  pipelineSpec:
+    tasks:
+      - name: max
+        taskSpec:
+          steps:
+            - name: success
+              image: registry.access.redhat.com/ubi9/ubi-minimal
+              script: 'exit 0'
+---
+apiVersion: tekton.dev/v1beta1
+kind: PipelineRun
+metadata:
+  name: duplicate-pr
+  annotations:
+    pipelinesascode.tekton.dev/on-target-branch: "[main]"
+    pipelinesascode.tekton.dev/on-event: "[pull_request]"
+spec:
+  pipelineSpec:
+    tasks:
+      - name: max
+        taskSpec:
+          steps:
+            - name: success
+              image: registry.access.redhat.com/ubi9/ubi-minimal
+              script: 'exit 0'

test/gitea_test.go

@@ -1016,11 +1016,28 @@ func TestGiteaPipelineRunWithSameName(t *testing.T) {
 		ExpectEvents:   true,
 		Regexp:         regexp.MustCompile(".*found multiple pipelinerun in .tekton with the same name*"),
 	}
-	ctx, f := tgitea.TestPR(t, topts)
+	_, f := tgitea.TestPR(t, topts)
 	defer f()
-	errre := regexp.MustCompile("found multiple pipelinerun in .tekton with the same name")
-	maxLines := int64(1000)
-	assert.NilError(t, twait.RegexpMatchingInControllerLog(ctx, topts.ParamsRun, *errre, 10, "controller", &maxLines))
+
+	// Wait for any webhook feedback loop to settle, then verify only 1 failure
+	// comment was posted (not duplicates from re-triggered no-op comment events).
+	time.Sleep(10 * time.Second)
+
+	comments, _, err := topts.GiteaCNX.Client().ListRepoIssueComments(
+		topts.PullRequest.Base.Repository.Owner.UserName,
+		topts.PullRequest.Base.Repository.Name,
+		forgejo.ListIssueCommentOptions{})
+	assert.NilError(t, err)
+
+	failureRe := regexp.MustCompile("found multiple pipelinerun in .tekton with the same name")
+	var failureCount int
+	for _, comment := range comments {
+		if failureRe.MatchString(comment.Body) {
+			failureCount++
+		}
+	}
+	assert.Equal(t, failureCount, 1,
+		"expected 1 failure comment but found %d", failureCount)
 }
 
 // TestGiteaProvenanceForDefaultBranch tests the provenance feature of the PipelineRun.