diff --git a/docs/content/docs/guides/event-matching/comment-and-label.md b/docs/content/docs/guides/event-matching/comment-and-label.md index 4fd5db4660..4aa2982c58 100644 --- a/docs/content/docs/guides/event-matching/comment-and-label.md +++ b/docs/content/docs/guides/event-matching/comment-and-label.md @@ -36,6 +36,8 @@ The `on-comment` annotation follows the pull_request [Policy]({{< relref "/docs/ {{< callout type="info" >}} The `on-comment` annotation works with pull_request events. For push events, Pipelines-as-Code supports it only [when targeting the main branch without arguments]({{< relref "/docs/guides/gitops-commands/push-commands" >}}). + +Avoid using Pipelines-as-Code's built-in [GitOps commands]({{< relref "/docs/guides/gitops-commands" >}}) — such as `/test`, `/retest`, `/cancel`, or `/ok-to-test` — as `on-comment` patterns. These commands are processed before `on-comment` matching and can lead to unexpected behavior. {{< /callout >}} ## Matching on pull request labels diff --git a/pkg/opscomments/comments.go b/pkg/opscomments/comments.go index 228d44600f..4d253ba03e 100644 --- a/pkg/opscomments/comments.go +++ b/pkg/opscomments/comments.go @@ -182,6 +182,9 @@ func getNameFromComment(typeOfComment, comment string) string { return "" } - // trim spaces - return strings.TrimSpace(firstArg[1]) + name := strings.TrimSpace(firstArg[1]) + if strings.Contains(name, "=") { + return "" + } + return name } diff --git a/pkg/opscomments/comments_test.go b/pkg/opscomments/comments_test.go index e97c4c9b40..b5a092f85d 100644 --- a/pkg/opscomments/comments_test.go +++ b/pkg/opscomments/comments_test.go @@ -86,6 +86,18 @@ func TestGetNameFromFunction(t *testing.T) { expected: "prname", commentType: testComment, }, + { + name: "key value arg is not a pipeline name", + comment: "/test custom1=value", + expected: "", + commentType: testComment, + }, + { + name: "key value args without pipeline name", + comment: "/test foo=bar hello=moto", + expected: "", + commentType: testComment, + }, { name: "get name from cancel comment", comment: "/cancel prname", @@ -295,6 +307,12 @@ func TestSetEventTypeTestPipelineRun(t *testing.T) { wantType: TestSingleCommentEventType.String(), wantTestPr: "prname", }, + { + name: "test with key value arg treated as test all", + comment: "/test custom1=value", + wantType: TestSingleCommentEventType.String(), + wantTestPr: "", + }, { name: "cancel single pr", comment: "/cancel prname", diff --git a/pkg/provider/provider.go b/pkg/provider/provider.go index af47891a62..dc78f93fa7 100644 --- a/pkg/provider/provider.go +++ b/pkg/provider/provider.go @@ -108,8 +108,18 @@ func getNameFromComment(typeOfComment, comment string) string { splitTest := strings.Split(comment, typeOfComment) // now get the first line getFirstLine := strings.Split(splitTest[1], "\n") - // trim spaces - return strings.TrimSpace(getFirstLine[0]) + + // and the first argument + firstArg := strings.Split(getFirstLine[0], " ") + if len(firstArg) < 2 { + return "" + } + + name := strings.TrimSpace(firstArg[1]) + if strings.Contains(name, "=") { + return "" + } + return name } func GetPipelineRunAndBranchOrTagNameFromTestComment(comment, prefix string) (string, string, string, error) { @@ -159,6 +169,9 @@ func getPipelineRunAndBranchOrTagNameFromComment(typeOfComment, comment string) prData := strings.Split(strings.TrimSpace(branchData[0]), " ") if len(prData) > 1 { prName = strings.TrimSpace(prData[0]) + if strings.Contains(prName, "=") { + prName = "" + } } } else { // get the second part of the typeOfComment (/test, /retest or /cancel) @@ -168,6 +181,9 @@ func getPipelineRunAndBranchOrTagNameFromComment(typeOfComment, comment string) // trim spaces // adapt for the comment contains the key=value pair prName = strings.Split(strings.TrimSpace(getFirstLine[0]), " ")[0] + if strings.Contains(prName, "=") { + prName = "" + } } return prName, branchName, tagName, nil } diff --git a/pkg/provider/provider_test.go b/pkg/provider/provider_test.go index 1ee0e7c47e..9a5cc87be2 100644 --- a/pkg/provider/provider_test.go +++ b/pkg/provider/provider_test.go @@ -215,6 +215,11 @@ func TestGetPipelineRunFromComment(t *testing.T) { comment: "before \n /test abc-01-pr \n after", want: "abc-01-pr", }, + { + name: "test with key value is not a pipeline name", + comment: "/test custom1=value", + want: "", + }, { name: "retest a pipeline", comment: "/retest abc-01-pr", @@ -261,6 +266,11 @@ func TestGetPipelineRunFromCancelComment(t *testing.T) { comment: "/cancel abc-01-pr", want: "abc-01-pr", }, + { + name: "cancel with key value is not a pipeline name", + comment: "/cancel custom1=value", + want: "", + }, { name: "string before cancel command", comment: "abc \n /cancel abc-01-pr", @@ -366,6 +376,19 @@ func TestGetPipelineRunAndBranchNameFromTestComment(t *testing.T) { branchName: "", wantError: false, }, + { + name: "test with only key value is not a pipeline name", + comment: "/test custom1=value", + prName: "", + wantError: false, + }, + { + name: "test with key value and branch is not a pipeline name", + comment: "/test custom1=value branch:nightly", + prName: "", + branchName: "nightly", + wantError: false, + }, { name: "string before retest command", comment: "abc \n /retest abc-01-pr", diff --git a/test/gitea_gitops_commands_test.go b/test/gitea_gitops_commands_test.go index a7bfc1fdc8..c86132c94f 100644 --- a/test/gitea_gitops_commands_test.go +++ b/test/gitea_gitops_commands_test.go @@ -130,6 +130,57 @@ func TestGiteaOnCommentAnnotation(t *testing.T) { assert.NilError(t, err) } +// TestGiteaOnCommentTestOverride tests that a PipelineRun with +// on-comment: "^/test.*" is triggered when posting "/test custom1=value". +// This verifies that key=value arguments are not mistaken for a PipelineRun +// name, which would bypass the on-comment annotation matching entirely. +func TestGiteaOnCommentTestOverride(t *testing.T) { + var err error + ctx := context.Background() + topts := &tgitea.TestOpts{ + TargetRefName: names.SimpleNameGenerator.RestrictLengthWithRandomSuffix("pac-e2e-test"), + RepoCRParams: &[]v1alpha1.Params{ + { + Name: "custom1", + Value: "default", + }, + }, + } + topts.TargetNS = topts.TargetRefName + topts.ParamsRun, topts.Opts, topts.GiteaCNX, err = tgitea.Setup(ctx) + assert.NilError(t, err, fmt.Errorf("cannot do gitea setup: %w", err)) + ctx, err = cctx.GetControllerCtxInfo(ctx, topts.ParamsRun) + assert.NilError(t, err) + assert.NilError(t, pacrepo.CreateNS(ctx, topts.TargetNS, topts.ParamsRun)) + 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/on-comment-test-override.yaml": "testdata/pipelinerun-on-comment-test-override.yaml", + } + topts.ExpectEvents = false + _, f := tgitea.TestPR(t, topts) + defer f() + + tgitea.PostCommentOnPullRequest(t, topts, "/test custom1=overridden") + waitOpts := twait.Opts{ + RepoName: topts.TargetNS, + Namespace: topts.TargetNS, + MinNumberStatus: 1, + PollTimeout: twait.DefaultTimeout, + } + repo, err := twait.UntilRepositoryUpdated(context.Background(), topts.ParamsRun.Clients, waitOpts) + assert.NilError(t, err) + assert.Equal(t, len(repo.Status), 1, "should have exactly 1 status") + assert.Equal(t, *repo.Status[0].EventType, opscomments.OnCommentEventType.String(), + "should have matched via on-comment annotation, not built-in /test handler") + + last := repo.Status[0] + err = twait.RegexpMatchingInPodLog(context.Background(), topts.ParamsRun, topts.TargetNS, + fmt.Sprintf("tekton.dev/pipelineRun=%s", last.PipelineRunName), "step-task", + *regexp.MustCompile("custom1 is overridden"), "", 2, nil) + assert.NilError(t, err) +} + // TestGiteaTestPipelineRunExplicitlyWithTestComment will test a pipelinerun // even if it hasn't matched when we are doing a /test comment. func TestGiteaTestPipelineRunExplicitlyWithTestComment(t *testing.T) { diff --git a/test/testdata/pipelinerun-on-comment-test-override.yaml b/test/testdata/pipelinerun-on-comment-test-override.yaml new file mode 100644 index 0000000000..a620018177 --- /dev/null +++ b/test/testdata/pipelinerun-on-comment-test-override.yaml @@ -0,0 +1,19 @@ +--- +apiVersion: tekton.dev/v1beta1 +kind: PipelineRun +metadata: + name: "on-comment-test-override" + annotations: + pipelinesascode.tekton.dev/target-namespace: "\\ .TargetNamespace //" + pipelinesascode.tekton.dev/on-comment: "^/test.*" +spec: + pipelineSpec: + tasks: + - name: task + taskSpec: + steps: + - name: task + image: registry.access.redhat.com/ubi10/ubi-micro + script: | + echo "event_type is {{ event_type }}" + echo "custom1 is {{ custom1 }}"