feat: support commas in PipelineRun annotations

This commit introduces support for using commas within annotations by
allowing users to escape them with the HTML entity `,`. This ensures
compatibility with annotations that rely on comma-separated values while
also permitting the inclusion of literal commas in file paths or branch
names.

This enhancement maintains backward compatibility while enabling more
precise and flexible use of annotations.

**Jira**: https://issues.redhat.com/browse/SRVKP-6790
**Signed-off-by**: Chmouel Boudjnah <[email protected]>

Signed-off-by: Chmouel Boudjnah <[email protected]>

docs/content/docs/guide/authoringprs.md

@@ -85,7 +85,15 @@ provider events target the branch `main` and it's coming from a `[pull_request]`
 Multiple target branch can be specified separated by comma, i.e:
 
 ```yaml
-[main, release-nightly]
+pipelinesascode.tekton.dev/on-target-branch: [main, release-nightly]
+```
+
+If you want to match a branch that has a comma in its name you can html escape
+it with the `,` entity, for example if you want to match main and the branch
+called `release,nightly` you can do this:
+
+```yaml
+pipelinesascode.tekton.dev/on-target-branch: [main, release,nightly]
 ```
 
 You can match on `pull_request` events as above, and you can as well match
@@ -140,7 +148,9 @@ To trigger a `PipelineRun` based on specific path changes in an event, use the
 annotation `pipelinesascode.tekton.dev/on-path-change`.
 
 Multiple paths can be specified, separated by commas. The first glob matching
-the files changes in the PR will trigger the `PipelineRun`.
+the files changes in the PR will trigger the `PipelineRun`. If you want to match
+a file or path that has a comma you can html escape it with the `,` html
+entity.
 
 You still need to specify the event type and target branch. If you have a [CEL
 expression](#matching-pipelinerun-by-path-change) the `on-path-change`

pkg/matcher/annotation_matcher.go

@@ -71,13 +71,15 @@ func getAnnotationValues(annotation string) ([]string, error) {
 
 	// if it's not an array then it would be a single string
 	if !strings.HasPrefix(annotation, "[") {
-		return []string{annotation}, nil
+		// replace , with comma so users can have comma in the annotation
+		annot := strings.ReplaceAll(annotation, ",", ",")
+		return []string{annot}, nil
 	}
 
 	// Split all tasks by comma and make sure to trim spaces in there
 	split := strings.Split(re.FindStringSubmatch(annotation)[1], ",")
 	for i := range split {
-		split[i] = strings.TrimSpace(split[i])
+		split[i] = strings.TrimSpace(strings.ReplaceAll(split[i], ",", ","))
 	}
 
 	if split[0] == "" {

pkg/matcher/annotation_matcher_test.go

@@ -527,6 +527,116 @@ func TestMatchPipelinerunAnnotationAndRepositories(t *testing.T) {
 				},
 			},
 		},
+		{ //nolint:dupl
+			name:       "match/on-path-change-ignore/with commas",
+			wantLog:    "Skipping pipelinerun with name: pipeline-target-ns",
+			wantPRName: pipelineTargetNSName,
+			args: annotationTestArgs{
+				fileChanged: []struct {
+					FileName    string
+					Status      string
+					NewFile     bool
+					RenamedFile bool
+					DeletedFile bool
+				}{
+					{
+						FileName:    "doc/gen,foo,bar.md",
+						Status:      "added",
+						NewFile:     true,
+						RenamedFile: false,
+						DeletedFile: false,
+					},
+				},
+				pruns: []*tektonv1.PipelineRun{
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name: pipelineTargetNSName,
+							Annotations: map[string]string{
+								keys.OnTargetBranch: mainBranch,
+								keys.OnEvent:        "[pull_request]",
+								keys.OnPathChange:   "[doc/gen,*]",
+							},
+						},
+					},
+				},
+				runevent: info.Event{
+					URL:               targetURL,
+					TriggerTarget:     "pull_request",
+					EventType:         "pull_request",
+					BaseBranch:        mainBranch,
+					HeadBranch:        "unittests",
+					PullRequestNumber: 1000,
+					Organization:      "mylittle",
+					Repository:        "pony",
+				},
+				data: testclient.Data{
+					Repositories: []*v1alpha1.Repository{
+						testnewrepo.NewRepo(
+							testnewrepo.RepoTestcreationOpts{
+								Name:             "test-good",
+								URL:              targetURL,
+								InstallNamespace: targetNamespace,
+							},
+						),
+					},
+				},
+			},
+		},
+		{ //nolint:dupl
+			name:    "ignored/on-path-change-ignore/no path change",
+			wantLog: "Skipping pipelinerun with name: pipeline-target-ns",
+			wantErr: true,
+			args: annotationTestArgs{
+				fileChanged: []struct {
+					FileName    string
+					Status      string
+					NewFile     bool
+					RenamedFile bool
+					DeletedFile bool
+				}{
+					{
+						FileName:    "foo/generated/gen.md",
+						Status:      "added",
+						NewFile:     true,
+						RenamedFile: false,
+						DeletedFile: false,
+					},
+				},
+				pruns: []*tektonv1.PipelineRun{
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name: pipelineTargetNSName,
+							Annotations: map[string]string{
+								keys.OnTargetBranch: mainBranch,
+								keys.OnEvent:        "[pull_request]",
+								keys.OnPathChange:   "[doc/***]",
+							},
+						},
+					},
+				},
+				runevent: info.Event{
+					URL:               targetURL,
+					TriggerTarget:     "pull_request",
+					EventType:         "pull_request",
+					BaseBranch:        mainBranch,
+					HeadBranch:        "unittests",
+					PullRequestNumber: 1000,
+					Organization:      "mylittle",
+					Repository:        "pony",
+				},
+				data: testclient.Data{
+					Repositories: []*v1alpha1.Repository{
+						testnewrepo.NewRepo(
+							testnewrepo.RepoTestcreationOpts{
+								Name:             "test-good",
+								URL:              targetURL,
+								InstallNamespace: targetNamespace,
+							},
+						),
+					},
+				},
+			},
+		},
 		{
 			name:    "ignored/on-path-change-ignore/include and ignore path",
 			wantLog: "Skipping pipelinerun with name: pipeline-target-ns",
@@ -1791,6 +1901,22 @@ func Test_getAnnotationValues(t *testing.T) {
 			want:    []string{"foo"},
 			wantErr: false,
 		},
+		{
+			name: "get-annotation-string-html-encoded-comma-list",
+			args: args{
+				annotation: "[foo,,bar]",
+			},
+			want:    []string{"foo,", "bar"},
+			wantErr: false,
+		},
+		{
+			name: "get-annotation-string-html-encoded-comma",
+			args: args{
+				annotation: "foo,bar",
+			},
+			want:    []string{"foo,bar"},
+			wantErr: false,
+		},
 		{
 			name: "get-annotation-multiples",
 			args: args{

test/gitea_test.go

@@ -680,6 +680,19 @@ func TestGiteaOnPathChange(t *testing.T) {
 	defer f()
 }
 
+func TestGiteaBranchWithComma(t *testing.T) {
+	topts := &tgitea.TestOpts{
+		TargetEvent:   triggertype.PullRequest.String(),
+		DefaultBranch: "branch,with,comma",
+		YAMLFiles: map[string]string{
+			".tekton/pr.yaml": "testdata/pipelinerun-target-branch-with-comma.yaml",
+		},
+		CheckForStatus: "success",
+	}
+	_, f := tgitea.TestPR(t, topts)
+	defer f()
+}
+
 // TestGiteaOnPathChangeIgnore will test that pipelinerun is not triggered when
 // a path is ignored but all other will do.
 func TestGiteaOnPathChangeIgnore(t *testing.T) {

test/pkg/gitea/scm.go

@@ -106,7 +106,7 @@ func GetIssueTimeline(ctx context.Context, topts *TestOpts) (Timelines, error) {
 	return tls, nil
 }
 
-func CreateGiteaRepo(giteaClient *gitea.Client, user, name, hookURL string, onOrg bool, logger *zap.SugaredLogger) (*gitea.Repository, error) {
+func CreateGiteaRepo(giteaClient *gitea.Client, user, name, defaultBranch, hookURL string, onOrg bool, logger *zap.SugaredLogger) (*gitea.Repository, error) {
 	var repo *gitea.Repository
 	var err error
 	// Create a new repo
@@ -131,9 +131,10 @@ func CreateGiteaRepo(giteaClient *gitea.Client, user, name, hookURL string, onOr
 	} else {
 		logger.Infof("Creating gitea repository %s for user %s", name, user)
 		repo, _, err = giteaClient.AdminCreateRepo(user, gitea.CreateRepoOption{
-			Name:        name,
-			Description: "This is a repo it's a wonderful thing",
-			AutoInit:    true,
+			Name:          name,
+			Description:   "This is a repo it's a wonderful thing",
+			AutoInit:      true,
+			DefaultBranch: defaultBranch,
 		})
 	}
 	if err != nil {

test/pkg/gitea/test.go

@@ -32,6 +32,7 @@ import (
 )
 
 type TestOpts struct {
+	TargetRepoName        string
 	StatusOnlyLatest      bool
 	OnOrg                 bool
 	NoPullRequestCreation bool
@@ -102,10 +103,20 @@ func TestPR(t *testing.T, topts *TestOpts) (context.Context, func()) {
 	if topts.TargetRefName == "" {
 		topts.TargetRefName = names.SimpleNameGenerator.RestrictLengthWithRandomSuffix("pac-e2e-test")
 		topts.TargetNS = topts.TargetRefName
-		assert.NilError(t, pacrepo.CreateNS(ctx, topts.TargetNS, topts.ParamsRun))
+	}
+	if err := pacrepo.CreateNS(ctx, topts.TargetNS, topts.ParamsRun); err != nil {
+		t.Logf("error creating namespace %s: %v", topts.TargetNS, err)
+	}
+
+	if topts.TargetRepoName == "" {
+		topts.TargetRepoName = topts.TargetRefName
 	}
 
-	repoInfo, err := CreateGiteaRepo(topts.GiteaCNX.Client, topts.Opts.Organization, topts.TargetRefName, hookURL, topts.OnOrg, topts.ParamsRun.Clients.Log)
+	if topts.DefaultBranch == "" {
+		topts.DefaultBranch = options.MainBranch
+	}
+
+	repoInfo, err := CreateGiteaRepo(topts.GiteaCNX.Client, topts.Opts.Organization, topts.TargetRepoName, topts.DefaultBranch, hookURL, topts.OnOrg, topts.ParamsRun.Clients.Log)
 	assert.NilError(t, err)
 	topts.Opts.Repo = repoInfo.Name
 	topts.Opts.Organization = repoInfo.Owner.UserName
@@ -179,7 +190,7 @@ func TestPR(t *testing.T, topts *TestOpts) (context.Context, func()) {
 		if topts.PullRequest, _, err = topts.GiteaCNX.Client.CreatePullRequest(topts.Opts.Organization, repoInfo.Name, gitea.CreatePullRequestOption{
 			Title: "Test Pull Request - " + topts.TargetRefName,
 			Head:  topts.TargetRefName,
-			Base:  options.MainBranch,
+			Base:  topts.DefaultBranch,
 		}); err == nil {
 			break
 		}
@@ -256,8 +267,11 @@ func NewPR(t *testing.T, topts *TestOpts) func() {
 		topts.TargetNS = topts.TargetRefName
 		assert.NilError(t, pacrepo.CreateNS(ctx, topts.TargetNS, topts.ParamsRun))
 	}
+	if topts.TargetRepoName == "" {
+		topts.TargetRepoName = topts.TargetRefName
+	}
 
-	repoInfo, err := GetGiteaRepo(topts.GiteaCNX.Client, topts.Opts.Organization, topts.TargetRefName, topts.ParamsRun.Clients.Log)
+	repoInfo, err := GetGiteaRepo(topts.GiteaCNX.Client, topts.Opts.Organization, topts.TargetRepoName, topts.ParamsRun.Clients.Log)
 	assert.NilError(t, err)
 	topts.Opts.Repo = repoInfo.Name
 	topts.Opts.Organization = repoInfo.Owner.UserName

test/testdata/pipelinerun-target-branch-with-comma.yaml

@@ -0,0 +1,20 @@
+---
+apiVersion: tekton.dev/v1beta1
+kind: PipelineRun
+metadata:
+  name: "\\ .PipelineName //"
+  annotations:
+    pipelinesascode.tekton.dev/target-namespace: "\\ .TargetNamespace //"
+    pipelinesascode.tekton.dev/on-target-branch: "[ branch,with,comma ]"
+    pipelinesascode.tekton.dev/on-event: "[\\ .TargetEvent //]"
+spec:
+  pipelineSpec:
+    tasks:
+      - name: task
+        taskSpec:
+          steps:
+            - name: success
+              image: registry.access.redhat.com/ubi9/ubi-micro
+              script: |
+                echo "I am a such a good booooy"
+                exit 0