Warn user when on-cel-expression is used with on-target-branch/on-event

This patch warns users in both the `user events namespaces`
and `controller logs`when using `on-event` and `on-target-branch`
annotations along side `on-cel-expression`

Hence warning users would avoid further confusion

Fixes: openshift-pipelines#1974

Signed-off-by: PuneetPunamiya <ppunamiy@redhat.com>

Author: PuneetPunamiya

pkg/matcher/annotation_matcher.go

@@ -11,6 +11,7 @@ import (
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys"
 	apipac "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/events"
 	"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"
@@ -149,7 +150,42 @@ func getName(prun *tektonv1.PipelineRun) string {
 	return name
 }
 
-func MatchPipelinerunByAnnotation(ctx context.Context, logger *zap.SugaredLogger, pruns []*tektonv1.PipelineRun, cs *params.Run, event *info.Event, vcx provider.Interface) ([]Match, error) {
+// checkPipelineRunAnnotation checks if the Pipelinerun has
+// `on-event`/`on-target-branch annotations` with `on-cel-expression`
+// and if present then warns the user that `on-cel-expression` will take precedence.
+func checkPipelineRunAnnotation(prun *tektonv1.PipelineRun, eventEmitter *events.EventEmitter, repo *apipac.Repository) {
+	// Define the annotations to check in a slice for easy iteration
+	checks := []struct {
+		key   string
+		value string
+	}{
+		{"on-event", prun.GetObjectMeta().GetAnnotations()[keys.OnEvent]},
+		{"on-target-branch", prun.GetObjectMeta().GetAnnotations()[keys.OnTargetBranch]},
+	}
+
+	// Preallocate the annotations slice with the exact capacity needed
+	annotations := make([]string, 0, len(checks))
+
+	// Iterate through each check and append the key if the value is non-empty
+	for _, check := range checks {
+		if check.value != "" {
+			annotations = append(annotations, check.key)
+		}
+	}
+
+	prName := getName(prun)
+	if len(annotations) > 0 {
+		ignoredAnnotations := strings.Join(annotations, ", ")
+		msg := fmt.Sprintf(
+			"Warning: The Pipelinerun '%s' has 'on-cel-expression' defined along with [%s] annotation(s). The 'on-cel-expression' will take precedence and these annotations will be ignored",
+			prName,
+			ignoredAnnotations,
+		)
+		eventEmitter.EmitMessage(repo, zap.WarnLevel, "RespositoryTakesOnCelExpressionPrecedence", msg)
+	}
+}
+
+func MatchPipelinerunByAnnotation(ctx context.Context, logger *zap.SugaredLogger, pruns []*tektonv1.PipelineRun, cs *params.Run, event *info.Event, vcx provider.Interface, eventEmitter *events.EventEmitter, repo *apipac.Repository) ([]Match, error) {
 	matchedPRs := []Match{}
 	infomsg := fmt.Sprintf("matching pipelineruns to event: URL=%s, target-branch=%s, source-branch=%s, target-event=%s",
 		event.URL,
@@ -236,6 +272,8 @@ func MatchPipelinerunByAnnotation(ctx context.Context, logger *zap.SugaredLogger
 		}
 
 		if celExpr, ok := prun.GetObjectMeta().GetAnnotations()[keys.OnCelExpression]; ok {
+			checkPipelineRunAnnotation(prun, eventEmitter, repo)
+
 			out, err := celEvaluate(ctx, celExpr, event, vcx)
 			if err != nil {
 				logger.Errorf("there was an error evaluating the CEL expression, skipping: %v", err)

pkg/matcher/annotation_matcher_test.go

@@ -14,6 +14,7 @@ import (
 	"github.com/jonboulle/clockwork"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/events"
 	"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/clients"
@@ -1414,9 +1415,10 @@ func runTest(ctx context.Context, t *testing.T, tt annotationTest, vcx provider.
 		Info:    info.Info{},
 	}
 
+	eventEmitter := events.NewEventEmitter(cs.Kube, logger)
 	matches, err := MatchPipelinerunByAnnotation(ctx, logger,
 		tt.args.pruns,
-		client, &tt.args.runevent, vcx,
+		client, &tt.args.runevent, vcx, eventEmitter, nil,
 	)
 
 	if tt.wantLog != "" {
@@ -1533,6 +1535,7 @@ func TestMatchPipelinerunByAnnotation(t *testing.T) {
 		wantErr    bool
 		wantPrName string
 		wantLog    []string
+		logLevel   int
 	}{
 		{
 			name: "good-match-with-only-one",
@@ -1609,6 +1612,38 @@ func TestMatchPipelinerunByAnnotation(t *testing.T) {
 			wantErr:    false,
 			wantPrName: pipelineCel.GetName(),
 		},
+		{
+			name: "cel-expression-takes-precedence-over-annotations",
+			args: args{
+				pruns: []*tektonv1.PipelineRun{
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name: "pipeline-on-cel-test",
+							Annotations: map[string]string{
+								keys.OnEvent:         "[pull_request]",
+								keys.OnTargetBranch:  "[main]",
+								keys.OnCelExpression: `event == "pull_request" && target_branch == "main" && source_branch == "warn-for-cel"`,
+							},
+						},
+					},
+				},
+				runevent: info.Event{
+					URL:               "https://hello/moto",
+					TriggerTarget:     "pull_request",
+					EventType:         "pull_request",
+					BaseBranch:        "main",
+					HeadBranch:        "warn-for-cel",
+					PullRequestNumber: 10,
+					Request: &info.Request{
+						Header: http.Header{},
+					},
+				},
+			},
+			wantErr: false,
+			wantLog: []string{
+				`Warning: The Pipelinerun 'pipeline-on-cel-test' has 'on-cel-expression' defined along with [on-event, on-target-branch] annotation(s). The 'on-cel-expression' will take precedence and these annotations will be ignored`,
+			},
+		},
 		{
 			name: "no-match-on-label",
 			args: args{
@@ -1914,7 +1949,9 @@ func TestMatchPipelinerunByAnnotation(t *testing.T) {
 				Clients: clients.Clients{},
 				Info:    info.Info{},
 			}
-			matches, err := MatchPipelinerunByAnnotation(ctx, logger, tt.args.pruns, cs, &tt.args.runevent, &ghprovider.Provider{})
+
+			eventEmitter := events.NewEventEmitter(cs.Clients.Kube, logger)
+			matches, err := MatchPipelinerunByAnnotation(ctx, logger, tt.args.pruns, cs, &tt.args.runevent, &ghprovider.Provider{}, eventEmitter, nil)
 			if (err != nil) != tt.wantErr {
 				t.Errorf("MatchPipelinerunByAnnotation() error = %v, wantErr %v", err, tt.wantErr)
 				return

pkg/pipelineascode/match.go

@@ -217,7 +217,7 @@ func (p *PacRun) getPipelineRunsFromRepo(ctx context.Context, repo *v1alpha1.Rep
 			return nil, err
 		}
 		// Don't fail or do anything if we don't have a match yet, we will do it properly later in this function
-		_, _ = matcher.MatchPipelinerunByAnnotation(ctx, p.logger, rtypes.PipelineRuns, p.run, p.event, p.vcx)
+		_, _ = matcher.MatchPipelinerunByAnnotation(ctx, p.logger, rtypes.PipelineRuns, p.run, p.event, p.vcx, p.eventEmitter, repo)
 	}
 	// Replace those {{var}} placeholders user has in her template to the run.Info variable
 	allTemplates := p.makeTemplate(ctx, repo, rawTemplates)
@@ -248,7 +248,7 @@ func (p *PacRun) getPipelineRunsFromRepo(ctx context.Context, repo *v1alpha1.Rep
 	// Match the PipelineRun with annotation
 	var matchedPRs []matcher.Match
 	if p.event.TargetTestPipelineRun == "" {
-		if matchedPRs, err = matcher.MatchPipelinerunByAnnotation(ctx, p.logger, pipelineRuns, p.run, p.event, p.vcx); err != nil {
+		if matchedPRs, err = matcher.MatchPipelinerunByAnnotation(ctx, p.logger, pipelineRuns, p.run, p.event, p.vcx, p.eventEmitter, repo); err != nil {
 			// Don't fail when you don't have a match between pipeline and annotations
 			p.eventEmitter.EmitMessage(nil, zap.WarnLevel, "RepositoryNoMatch", err.Error())
 			// In a scenario where an external user submits a pull request and the repository owner uses the
@@ -336,7 +336,7 @@ func (p *PacRun) getPipelineRunsFromRepo(ctx context.Context, repo *v1alpha1.Rep
 		}}, nil
 	}
 
-	matchedPRs, err = matcher.MatchPipelinerunByAnnotation(ctx, p.logger, pipelineRuns, p.run, p.event, p.vcx)
+	matchedPRs, err = matcher.MatchPipelinerunByAnnotation(ctx, p.logger, pipelineRuns, p.run, p.event, p.vcx, p.eventEmitter, repo)
 	if err != nil {
 		// Don't fail when you don't have a match between pipeline and annotations
 		p.eventEmitter.EmitMessage(nil, zap.WarnLevel, "RepositoryNoMatch", err.Error())