fix: Do not return error when PipelineRun patching is failed

when PipelineRun patch is failed in startPR func, PaC was
returning error which was creating a failed check on git
provider though PipelineRun was running and it was making
user confused, this commit fixes that.

https://issues.redhat.com/browse/SRVKP-9345

Signed-off-by: Zaki Shaikh <[email protected]>

Author: zakisk

pkg/pipelineascode/pipelineascode.go

@@ -310,9 +310,11 @@ func (p *PacRun) startPR(ctx context.Context, match matcher.Match) (*tektonv1.Pi
 	if len(patchAnnotations) > 0 || len(patchLabels) > 0 {
 		pr, err = action.PatchPipelineRun(ctx, p.logger, whatPatching, p.run.Clients.Tekton, pr, getMergePatch(patchAnnotations, patchLabels))
 		if err != nil {
-			// we still return the created PR with error, and allow caller to decide what to do with the PR, and avoid
-			// unneeded SIGSEGV's
-			return pr, fmt.Errorf("cannot patch pipelinerun %s: %w", pr.GetGenerateName(), err)
+			// if PipelineRun patch is failed then do not return error, just log the error
+			// because its a false negative and on startPR return a failed check is being created
+			// due to this.
+			p.logger.Errorf("cannot patch pipelinerun %s: %w", pr.GetGenerateName(), err)
+			return pr, nil
 		}
 		currentReason := ""
 		if len(pr.Status.GetConditions()) > 0 {

pkg/pipelineascode/pipelineascode_startpr_test.go

@@ -765,31 +765,29 @@ func TestStartPR_PatchBehavior(t *testing.T) {
 		patchErrorMsg       string
 		expectError         bool
 		expectErrorContains []string
+		expectedLogSnippet  string
 		verifyAnnotations   bool // whether to verify annotations were set correctly
 	}{
 		{
-			name:               "successful patch - all annotations set",
-			simulatePatchError: false,
-			expectError:        false,
-			verifyAnnotations:  true,
+			name:              "successful patch - all annotations set",
+			verifyAnnotations: true,
 		},
 		{
-			name:                "patch failure - PR still returned with error",
-			simulatePatchError:  true,
-			patchErrorMsg:       "etcd unavailable",
-			expectError:         true,
-			expectErrorContains: []string{"cannot patch pipelinerun", "etcd unavailable"},
-			verifyAnnotations:   false,
+			name:               "patch failure - PR still returned with error",
+			simulatePatchError: true,
+			patchErrorMsg:      "etcd unavailable",
+			expectedLogSnippet: "cannot patch pipelinerun",
+			verifyAnnotations:  false,
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
+			observer, log := zapobserver.New(zap.InfoLevel)
+			logger := zap.New(observer).Sugar()
 			if tt.simulatePatchError {
 				// Need custom reactor to simulate patch failure
 				ctx, _ := rtesting.SetupFakeContext(t)
-				observer, _ := zapobserver.New(zap.InfoLevel)
-				logger := zap.New(observer).Sugar()
 
 				stdata, _ := testclient.SeedTestData(t, ctx, testclient.Data{
 					Namespaces: []*corev1.Namespace{
@@ -889,6 +887,11 @@ func TestStartPR_PatchBehavior(t *testing.T) {
 				assert.NilError(t, err)
 				assert.Assert(t, pr != nil, "PipelineRun should be returned")
 
+				if tt.expectedLogSnippet != "" {
+					logmsg := log.FilterMessageSnippet(tt.expectedLogSnippet).TakeAll()
+					assert.Assert(t, len(logmsg) > 0, "log messages", logmsg, tt.expectedLogSnippet)
+				}
+
 				if tt.verifyAnnotations {
 					state, hasState := pr.GetAnnotations()[keys.State]
 					assert.Assert(t, hasState, "State annotation should be patched")