From 3e343da1ecc6daf28ed6616783c7a6eff63d44f3 Mon Sep 17 00:00:00 2001 From: Zaki Shaikh Date: Tue, 3 Feb 2026 14:44:16 +0530 Subject: [PATCH 1/2] feat(github): add configurable GitOps command prefix - Users can set a GitOps command prefix in the Repository CR to run / test, / retest, / cancel. - Configure prefix in Repository CR's settings field. - Apply prefix-aware matching for GitHub issue/commit comments, including ok-to-test SHA validation and target selection. - Update repo schema/docs and opscomments helpers for prefixed regex matching and command extraction. - Add provider parse tests and e2e coverage for GitHub prefix flows to validate behavior end-to-end. - Add documentation explaining the feature thoroughly. https://issues.redhat.com/browse/SRVKP-7197 Signed-off-by: Zaki Shaikh --- config/300-repositories.yaml | 5 + .../docs/guides/gitops-commands/advanced.md | 41 ++ pkg/apis/pipelinesascode/v1alpha1/types.go | 5 + pkg/opscomments/comments.go | 151 +++---- pkg/opscomments/comments_test.go | 425 ++++++------------ pkg/provider/bitbucketcloud/parse_payload.go | 2 +- pkg/provider/gitea/parse_payload.go | 2 +- pkg/provider/github/acl.go | 18 +- pkg/provider/github/acl_test.go | 80 +++- pkg/provider/github/github_test.go | 11 +- pkg/provider/github/parse_payload.go | 75 +++- pkg/provider/github/parse_payload_test.go | 188 +++++++- pkg/provider/gitlab/parse_payload.go | 8 +- pkg/provider/provider.go | 26 +- pkg/provider/provider_test.go | 207 ++++++++- test/github_comment_strategy_update_test.go | 6 +- test/github_gitops_commands_prefix_test.go | 115 +++++ test/github_pullrequest_test.go | 2 +- test/github_push_retest_test.go | 2 +- test/pkg/github/crd.go | 2 +- test/pkg/github/pr.go | 2 +- test/pkg/options/options.go | 2 +- 22 files changed, 929 insertions(+), 446 deletions(-) create mode 100644 test/github_gitops_commands_prefix_test.go diff --git a/config/300-repositories.yaml b/config/300-repositories.yaml index cb14aee93b..7bf0a7caf9 100644 --- a/config/300-repositories.yaml +++ b/config/300-repositories.yaml @@ -507,6 +507,11 @@ spec: - update type: string type: object + gitops_command_prefix: + description: |- + GitOpsCommandPrefix configures the prefix for the GitOps command. + This is used to identify the GitOps command in the PipelineRun. + type: string pipelinerun_provenance: description: |- PipelineRunProvenance configures how PipelineRun definitions are fetched. diff --git a/docs/content/docs/guides/gitops-commands/advanced.md b/docs/content/docs/guides/gitops-commands/advanced.md index 6f8837daca..661b5a4f35 100644 --- a/docs/content/docs/guides/gitops-commands/advanced.md +++ b/docs/content/docs/guides/gitops-commands/advanced.md @@ -46,6 +46,47 @@ This ensures the comment is correctly formatted when processed. For a practical example, see the [pac-boussole](https://github.com/openshift-pipelines/pac-boussole) project, which uses the `on-comment` annotation to create a PipelineRun experience similar to [Prow](https://docs.prow.k8s.io/). +## GitOps Command Prefix + +{{< support_matrix github_app="true" github_webhook="true" forgejo="false" gitlab="false" bitbucket_cloud="false" bitbucket_datacenter="false" >}} + +You can configure a custom prefix for GitOps commands in the Repository CR. This allows you to use commands like `/pac test` instead of the standard `/test`. This is useful when both [prow](https://docs.prow.k8s.io/) CI and Pipelines-as-Code are configured on a Repository and making comments causes issues and confusion. + +Please note that custom GitOps commands are excluded from this prefix settings. + +To configure a custom GitOps command prefix, set the `gitops_command_prefix` field in your Repository CR's `settings` section: + +```yaml +apiVersion: "pipelinesascode.tekton.dev/v1alpha1" +kind: Repository +metadata: + name: my-repository + namespace: pipelines-as-code +spec: + url: "https://github.com/organization/repository" + settings: + gitops_command_prefix: "pac" +``` + +Note: Set the prefix as a plain word (e.g. `pac`). The Forward slash (`/`) is added automatically by Pipelines-as-Code. + +With this configuration, you can use the following prefixed commands: + +- `/pac test` - Trigger all matching PipelineRuns +- `/pac test ` - Trigger a specific PipelineRun +- `/pac retest` - Retest failed PipelineRuns +- `/pac retest ` - Retest specific PipelineRun +- `/pac cancel` - Cancel all running PipelineRuns +- `/pac cancel ` - Cancel Specific PipelineRun +- `/pac ok-to-test` - Approve CI for external contributors +- `/pac ok-to-test SHA` - Approve CI for external contributors for a specific SHA + +Example: + +```text +/pac test +``` + ## Cancelling a PipelineRun **What it does:** The `/cancel` command stops running PipelineRuns by commenting on the pull request. diff --git a/pkg/apis/pipelinesascode/v1alpha1/types.go b/pkg/apis/pipelinesascode/v1alpha1/types.go index cbd67db34b..d31cf2f3ba 100644 --- a/pkg/apis/pipelinesascode/v1alpha1/types.go +++ b/pkg/apis/pipelinesascode/v1alpha1/types.go @@ -150,6 +150,11 @@ type Settings struct { // +kubebuilder:validation:Enum=source;default_branch PipelineRunProvenance string `json:"pipelinerun_provenance,omitempty"` + // GitOpsCommandPrefix configures the prefix for the GitOps command. + // This is used to identify the GitOps command in the PipelineRun. + // +optional + GitOpsCommandPrefix string `json:"gitops_command_prefix,omitempty"` + // Policy defines authorization policies for the repository, controlling who can // trigger PipelineRuns under different conditions. // +optional diff --git a/pkg/opscomments/comments.go b/pkg/opscomments/comments.go index d50e95ef23..228d44600f 100644 --- a/pkg/opscomments/comments.go +++ b/pkg/opscomments/comments.go @@ -5,23 +5,11 @@ import ( "regexp" "strings" - "go.uber.org/zap" - - "github.com/openshift-pipelines/pipelines-as-code/pkg/acl" "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/params/info" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype" -) - -var ( - testAllRegex = regexp.MustCompile(`(?m)^/test\s*$`) - retestAllRegex = regexp.MustCompile(`(?m)^/retest\s*$`) - testSingleRegex = regexp.MustCompile(`(?m)^/test[ \t]+\S+`) - retestSingleRegex = regexp.MustCompile(`(?m)^/retest[ \t]+\S+`) - oktotestRegex = regexp.MustCompile(acl.OKToTestCommentRegexp) - cancelAllRegex = regexp.MustCompile(`(?m)^(/cancel)\s*$`) - cancelSingleRegex = regexp.MustCompile(`(?m)^(/cancel)[ \t]+\S+`) + "go.uber.org/zap" ) type EventType string @@ -42,27 +30,49 @@ var ( OkToTestCommentEventType = EventType("ok-to-test-comment") ) -const ( - testComment = "/test" - retestComment = "/retest" - cancelComment = "/cancel" -) +func RetestAllRegex(prefix string) *regexp.Regexp { + return regexp.MustCompile(fmt.Sprintf(`(?m)^\s*%sretest\s*$`, prefix)) +} + +func RetestSingleRegex(prefix string) *regexp.Regexp { + return regexp.MustCompile(fmt.Sprintf(`(?m)^\s*%sretest[ \t]+\S+`, prefix)) +} + +func TestAllRegex(prefix string) *regexp.Regexp { + return regexp.MustCompile(fmt.Sprintf(`(?m)^\s*%stest\s*$`, prefix)) +} + +func TestSingleRegex(prefix string) *regexp.Regexp { + return regexp.MustCompile(fmt.Sprintf(`(?m)^\s*%stest[ \t]+\S+`, prefix)) +} + +func OkToTestRegex(prefix string) *regexp.Regexp { + return regexp.MustCompile(fmt.Sprintf(`(^|\n)\s*%sok-to-test(?:\s+([a-fA-F0-9]{7,40}))?\s*(\r\n|\r|\n|$)`, prefix)) +} + +func CancelAllRegex(prefix string) *regexp.Regexp { + return regexp.MustCompile(fmt.Sprintf(`(?m)^\s*%scancel\s*$`, prefix)) +} + +func CancelSingleRegex(prefix string) *regexp.Regexp { + return regexp.MustCompile(fmt.Sprintf(`(?m)^\s*%scancel[ \t]+\S+`, prefix)) +} -func CommentEventType(comment string) EventType { +func CommentEventType(comment, prefix string) EventType { switch { - case retestAllRegex.MatchString(comment): + case RetestAllRegex(prefix).MatchString(comment): return RetestAllCommentEventType - case retestSingleRegex.MatchString(comment): + case RetestSingleRegex(prefix).MatchString(comment): return RetestSingleCommentEventType - case testAllRegex.MatchString(comment): + case TestAllRegex(prefix).MatchString(comment): return TestAllCommentEventType - case testSingleRegex.MatchString(comment): + case TestSingleRegex(prefix).MatchString(comment): return TestSingleCommentEventType - case oktotestRegex.MatchString(comment): + case OkToTestRegex(prefix).MatchString(comment): return OkToTestCommentEventType - case cancelAllRegex.MatchString(comment): + case CancelAllRegex(prefix).MatchString(comment): return CancelCommentAllEventType - case cancelSingleRegex.MatchString(comment): + case CancelSingleRegex(prefix).MatchString(comment): return CancelCommentSingleEventType default: return NoOpsCommentEventType @@ -70,28 +80,43 @@ func CommentEventType(comment string) EventType { } // SetEventTypeAndTargetPR function will set the event type and target test pipeline run in an event. -func SetEventTypeAndTargetPR(event *info.Event, comment string) { - commentType := CommentEventType(comment) - if commentType == RetestSingleCommentEventType || commentType == TestSingleCommentEventType { - event.TargetTestPipelineRun = GetPipelineRunFromTestComment(comment) +func SetEventTypeAndTargetPR(event *info.Event, comment, prefix string) { + commentType := CommentEventType(comment, prefix) + if commentType == RetestSingleCommentEventType { + typeOfComment := prefix + "retest" + event.TargetTestPipelineRun = getNameFromComment(typeOfComment, comment) + } + if commentType == TestSingleCommentEventType { + typeOfComment := prefix + "test" + event.TargetTestPipelineRun = getNameFromComment(typeOfComment, comment) } if commentType == CancelCommentAllEventType || commentType == CancelCommentSingleEventType { event.CancelPipelineRuns = true } if commentType == CancelCommentSingleEventType { - event.TargetCancelPipelineRun = GetPipelineRunFromCancelComment(comment) + typeOfComment := prefix + "cancel" + event.TargetCancelPipelineRun = getNameFromComment(typeOfComment, comment) } event.EventType = commentType.String() event.TriggerComment = comment } -func IsOkToTestComment(comment string) bool { - return oktotestRegex.MatchString(comment) +func IsOkToTestComment(comment, prefix string) bool { + return OkToTestRegex(prefix).MatchString(comment) +} + +func IsTestRetestComment(comment, prefix string) bool { + return TestSingleRegex(prefix).MatchString(comment) || TestAllRegex(prefix).MatchString(comment) || + RetestSingleRegex(prefix).MatchString(comment) || RetestAllRegex(prefix).MatchString(comment) +} + +func IsCancelComment(comment, prefix string) bool { + return CancelAllRegex(prefix).MatchString(comment) || CancelSingleRegex(prefix).MatchString(comment) } // GetSHAFromOkToTestComment extracts the optional SHA from an /ok-to-test comment. -func GetSHAFromOkToTestComment(comment string) string { - matches := oktotestRegex.FindStringSubmatch(comment) +func GetSHAFromOkToTestComment(comment, prefix string) string { + matches := OkToTestRegex(prefix).FindStringSubmatch(comment) if len(matches) > 2 { return strings.TrimSpace(matches[2]) } @@ -143,17 +168,6 @@ func AnyOpsKubeLabelInSelector() string { OnCommentEventType.String()) } -func GetPipelineRunFromTestComment(comment string) string { - if strings.Contains(comment, testComment) { - return getNameFromComment(testComment, comment) - } - return getNameFromComment(retestComment, comment) -} - -func GetPipelineRunFromCancelComment(comment string) string { - return getNameFromComment(cancelComment, comment) -} - func getNameFromComment(typeOfComment, comment string) string { splitTest := strings.Split(strings.TrimSpace(comment), typeOfComment) if len(splitTest) < 2 { @@ -171,48 +185,3 @@ func getNameFromComment(typeOfComment, comment string) string { // trim spaces return strings.TrimSpace(firstArg[1]) } - -func GetPipelineRunAndBranchNameFromTestComment(comment string) (string, string, error) { - if strings.Contains(comment, testComment) { - return getPipelineRunAndBranchNameFromComment(testComment, comment) - } - return getPipelineRunAndBranchNameFromComment(retestComment, comment) -} - -func GetPipelineRunAndBranchNameFromCancelComment(comment string) (string, string, error) { - return getPipelineRunAndBranchNameFromComment(cancelComment, comment) -} - -// getPipelineRunAndBranchNameFromComment function will take GitOps comment and split the comment -// by /test, /retest or /cancel to return branch name and pipelinerun name. -func getPipelineRunAndBranchNameFromComment(typeOfComment, comment string) (string, string, error) { - var prName, branchName string - splitTest := strings.Split(comment, typeOfComment) - - // after the split get the second part of the typeOfComment (/test, /retest or /cancel) - // as second part can be branch name or pipelinerun name and branch name - // ex: /test branch:nightly, /test prname branch:nightly - if splitTest[1] != "" && strings.Contains(splitTest[1], ":") { - branchData := strings.Split(splitTest[1], ":") - - // make sure no other word is supported other than branch word - if !strings.Contains(branchData[0], "branch") { - return prName, branchName, fmt.Errorf("the GitOps comment%s does not contain a branch word", branchData[0]) - } - branchName = strings.Split(strings.TrimSpace(branchData[1]), " ")[0] - - // if data after the split contains prname then fetch that - prData := strings.Split(strings.TrimSpace(branchData[0]), " ") - if len(prData) > 1 { - prName = strings.TrimSpace(prData[0]) - } - } else { - // get the second part of the typeOfComment (/test, /retest or /cancel) - // as second part contains pipelinerun name - // ex: /test prname - getFirstLine := strings.Split(splitTest[1], "\n") - // trim spaces - prName = strings.TrimSpace(getFirstLine[0]) - } - return prName, branchName, nil -} diff --git a/pkg/opscomments/comments_test.go b/pkg/opscomments/comments_test.go index 88d27710be..e97c4c9b40 100644 --- a/pkg/opscomments/comments_test.go +++ b/pkg/opscomments/comments_test.go @@ -17,6 +17,12 @@ import ( testnewrepo "github.com/openshift-pipelines/pipelines-as-code/pkg/test/repository" ) +const ( + testComment = "/test" + retestComment = "/retest" + cancelComment = "/cancel" +) + func TestLabelsBackwardCompat(t *testing.T) { testCases := []struct { name string @@ -171,6 +177,7 @@ func TestCommentEventTypeTest(t *testing.T) { tests := []struct { name string comment string + prefix string want EventType }{ { @@ -213,11 +220,50 @@ func TestCommentEventTypeTest(t *testing.T) { comment: "/cancel prname", want: CancelCommentSingleEventType, }, + { + name: "test all with prefix", + comment: "/pac test", + prefix: "/pac ", + want: TestAllCommentEventType, + }, + { + name: "test single with prefix", + comment: "/pac test prname", + prefix: "/pac ", + want: TestSingleCommentEventType, + }, + { + name: "retest all with prefix", + comment: "/pac retest", + prefix: "/pac ", + want: RetestAllCommentEventType, + }, + { + name: "retest single with prefix", + comment: "/pac retest prname", + prefix: "/pac ", + want: RetestSingleCommentEventType, + }, + { + name: "cancel all with prefix", + comment: "/pac cancel", + prefix: "/pac ", + want: CancelCommentAllEventType, + }, + { + name: "cancel single with prefix", + comment: "/pac cancel prname", + prefix: "/pac ", + want: CancelCommentSingleEventType, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := CommentEventType(tt.comment) + if tt.prefix == "" { + tt.prefix = "/" + } + got := CommentEventType(tt.comment, tt.prefix) assert.Equal(t, tt.want, got) }) } @@ -230,7 +276,7 @@ func TestSetEventTypeTestPipelineRun(t *testing.T) { wantType string wantTestPr string wantCancelPr string - wantCancel bool + prefix string }{ { name: "no event type", @@ -254,20 +300,60 @@ func TestSetEventTypeTestPipelineRun(t *testing.T) { comment: "/cancel prname", wantType: CancelCommentSingleEventType.String(), wantCancelPr: "prname", - wantCancel: true, }, { - name: "cancel all pr", - comment: "/cancel", - wantType: CancelCommentAllEventType.String(), - wantCancel: true, + name: "cancel all pr", + comment: "/cancel", + wantType: CancelCommentAllEventType.String(), + }, + { + name: "test single pr with prefix", + comment: "/pac test prname", + wantType: TestSingleCommentEventType.String(), + wantTestPr: "prname", + prefix: "/pac ", + }, + { + name: "test all with prefix", + comment: "/pac test", + wantType: TestAllCommentEventType.String(), + prefix: "/pac ", + }, + { + name: "retest single pr with prefix", + comment: "/pac retest prname", + wantType: RetestSingleCommentEventType.String(), + wantTestPr: "prname", + prefix: "/pac ", + }, + { + name: "retest all with prefix", + comment: "/pac retest", + wantType: RetestAllCommentEventType.String(), + prefix: "/pac ", + }, + { + name: "cancel single pr with prefix", + comment: "/pac cancel prname", + wantType: CancelCommentSingleEventType.String(), + wantCancelPr: "prname", + prefix: "/pac ", + }, + { + name: "cancel all with prefix", + comment: "/pac cancel", + wantType: CancelCommentAllEventType.String(), + prefix: "/pac ", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + if tt.prefix == "" { + tt.prefix = "/" + } event := &info.Event{} - SetEventTypeAndTargetPR(event, tt.comment) + SetEventTypeAndTargetPR(event, tt.comment, tt.prefix) assert.Equal(t, tt.wantType, event.EventType) assert.Equal(t, tt.wantTestPr, event.TargetTestPipelineRun) }) @@ -278,6 +364,7 @@ func TestIsOkToTestComment(t *testing.T) { tests := []struct { name string comment string + prefix string want bool }{ { @@ -310,11 +397,20 @@ func TestIsOkToTestComment(t *testing.T) { comment: "/ok-to-test 1234567", want: true, }, + { + name: "valid comment with sha with prefix", + comment: "/pac ok-to-test 1234567", + prefix: "/pac ", + want: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := IsOkToTestComment(tt.comment) + if tt.prefix == "" { + tt.prefix = "/" + } + got := IsOkToTestComment(tt.comment, tt.prefix) assert.Equal(t, tt.want, got) }) } @@ -324,6 +420,7 @@ func TestIsTestRetestComment(t *testing.T) { tests := []struct { name string comment string + prefix string want EventType }{ { @@ -386,303 +483,36 @@ func TestIsTestRetestComment(t *testing.T) { comment: "/ok", want: NoOpsCommentEventType, }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := CommentEventType(tt.comment) - assert.Equal(t, tt.want, got) - }) - } -} - -func TestGetPipelineRunFromComment(t *testing.T) { - tests := []struct { - name string - comment string - want string - }{ - { - name: "test no pipelinerun", - comment: "/test", - want: "", - }, - { - name: "retest no pipelinerun", - comment: "/retest", - want: "", - }, - { - name: "test a pipeline", - comment: "/test abc-01-pr", - want: "abc-01-pr", - }, - { - name: "string before test command", - comment: "abc \n /test abc-01-pr", - want: "abc-01-pr", - }, - { - name: "string after test command", - comment: "/test abc-01-pr \n abc", - want: "abc-01-pr", - }, - { - name: "string before and after test command", - comment: "before \n /test abc-01-pr \n after", - want: "abc-01-pr", - }, - { - name: "retest a pipeline", - comment: "/retest abc-01-pr", - want: "abc-01-pr", - }, - { - name: "string before retest command", - comment: "abc \n /retest abc-01-pr", - want: "abc-01-pr", - }, - { - name: "string after retest command", - comment: "/retest abc-01-pr \n abc", - want: "abc-01-pr", - }, - { - name: "string before and after retest command", - comment: "before \n /retest abc-01-pr \n after", - want: "abc-01-pr", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := GetPipelineRunFromTestComment(tt.comment) - assert.Equal(t, tt.want, got) - }) - } -} - -func TestGetPipelineRunFromCancelComment(t *testing.T) { - tests := []struct { - name string - comment string - want string - }{ - { - name: "cancel all", - comment: "/cancel", - want: "", - }, { - name: "cancel a pipeline", - comment: "/cancel abc-01-pr", - want: "abc-01-pr", - }, - { - name: "string before cancel command", - comment: "abc \n /cancel abc-01-pr", - want: "abc-01-pr", - }, - { - name: "string after cancel command", - comment: "/cancel abc-01-pr \n abc", - want: "abc-01-pr", + name: "valid retest with prefix", + comment: "/pac retest", + prefix: "/pac ", + want: RetestAllCommentEventType, }, { - name: "string before and after cancel command", - comment: "before \n /cancel abc-01-pr \n after", - want: "abc-01-pr", + name: "valid test with prefix", + comment: "/pac test", + prefix: "/pac ", + want: TestAllCommentEventType, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := GetPipelineRunFromCancelComment(tt.comment) + if tt.prefix == "" { + tt.prefix = "/" + } + got := CommentEventType(tt.comment, tt.prefix) assert.Equal(t, tt.want, got) }) } } -func TestGetPipelineRunAndBranchNameFromTestComment(t *testing.T) { - tests := []struct { - name string - comment string - branchName string - prName string - wantError bool - }{ - { - name: "retest all on test branch", - comment: "/retest branch:test", - branchName: "test", - wantError: false, - }, - { - name: "test a pipeline on test branch", - comment: "/test abc-01-pr branch:test", - prName: "abc-01-pr", - branchName: "test", - wantError: false, - }, - { - name: "string for test command before branch name test", - comment: "/test abc-01-pr abc \n branch:test", - prName: "abc-01-pr", - branchName: "test", - wantError: false, - }, - { - name: "string for retest command after branch name test", - comment: "/retest abc-01-pr branch:test \n abc", - prName: "abc-01-pr", - branchName: "test", - wantError: false, - }, - { - name: "string for test command before and after branch name test", - comment: "/test abc-01-pr \n before branch:test \n after", - prName: "abc-01-pr", - branchName: "test", - wantError: false, - }, - { - name: "different word other than branch for retest command", - comment: "/retest invalidname:nightly", - wantError: true, - }, - { - name: "test all", - comment: "/test", - wantError: false, - }, - { - name: "test a pipeline", - comment: "/test abc-01-pr", - prName: "abc-01-pr", - wantError: false, - }, - { - name: "string before retest command", - comment: "abc \n /retest abc-01-pr", - prName: "abc-01-pr", - wantError: false, - }, - { - name: "string after retest command", - comment: "/retest abc-01-pr \n abc", - prName: "abc-01-pr", - wantError: false, - }, - { - name: "string before and after test command", - comment: "before \n /test abc-01-pr \n after", - prName: "abc-01-pr", - wantError: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - prName, branchName, err := GetPipelineRunAndBranchNameFromTestComment(tt.comment) - assert.Equal(t, tt.wantError, err != nil) - assert.Equal(t, tt.branchName, branchName) - assert.Equal(t, tt.prName, prName) - }) - } -} - -func TestGetPipelineRunAndBranchNameFromCancelComment(t *testing.T) { - tests := []struct { - name string - comment string - branchName string - prName string - wantError bool - }{ - { - name: "cancel all pipeline", - comment: "/cancel", - wantError: false, - }, - { - name: "cancel a particular pipeline", - comment: "/cancel abc-01-pr", - prName: "abc-01-pr", - wantError: false, - }, - { - name: "add string before cancel command", - comment: "abc \n /cancel abc-01-pr", - prName: "abc-01-pr", - wantError: false, - }, - { - name: "add string after cancel command", - comment: "/cancel abc-01-pr \n abc", - prName: "abc-01-pr", - wantError: false, - }, - { - name: "add string before and after cancel command", - comment: "before \n /cancel abc-01-pr \n after", - prName: "abc-01-pr", - wantError: false, - }, - { - name: "cancel all on test branch", - comment: "/cancel branch:test", - branchName: "test", - wantError: false, - }, - { - name: "cancel a pipeline on test branch", - comment: "/cancel abc-01-pr branch:test", - prName: "abc-01-pr", - branchName: "test", - wantError: false, - }, - { - name: "string for cancel command before branch name test", - comment: "/cancel abc-01-pr abc \n branch:test", - prName: "abc-01-pr", - branchName: "test", - wantError: false, - }, - { - name: "string for cancel command after branch name test", - comment: "/cancel abc-01-pr branch:test \n abc", - prName: "abc-01-pr", - branchName: "test", - wantError: false, - }, - { - name: "string for cancel command before and after branch name test", - comment: "/cancel abc-01-pr \n before branch:test \n after", - prName: "abc-01-pr", - branchName: "test", - wantError: false, - }, - { - name: "different word other than branch for cancel command", - comment: "/cancel invalidname:nightly", - wantError: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - prName, branchName, err := GetPipelineRunAndBranchNameFromCancelComment(tt.comment) - assert.Equal(t, tt.wantError, err != nil) - assert.Equal(t, tt.branchName, branchName) - assert.Equal(t, tt.prName, prName) - }) - } -} - func TestGetSHAFromOkToTestComment(t *testing.T) { tests := []struct { name string comment string + prefix string want string }{ { @@ -705,10 +535,25 @@ func TestGetSHAFromOkToTestComment(t *testing.T) { comment: "lgtm\n/ok-to-test 1234567\napproved", want: "1234567", }, + { + name: "no sha with prefix", + comment: "/pac ok-to-test", + prefix: "/pac ", + want: "", + }, + { + name: "sha with prefix", + comment: "/pac ok-to-test 1234567", + prefix: "/pac ", + want: "1234567", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := GetSHAFromOkToTestComment(tt.comment) + if tt.prefix == "" { + tt.prefix = "/" + } + got := GetSHAFromOkToTestComment(tt.comment, tt.prefix) assert.Equal(t, tt.want, got) }) } diff --git a/pkg/provider/bitbucketcloud/parse_payload.go b/pkg/provider/bitbucketcloud/parse_payload.go index cf60c0ba43..f8179bc120 100644 --- a/pkg/provider/bitbucketcloud/parse_payload.go +++ b/pkg/provider/bitbucketcloud/parse_payload.go @@ -130,7 +130,7 @@ func (v *Provider) ParsePayload(ctx context.Context, run *params.Run, request *h case provider.Valid(event, pullRequestsCreated): processedEvent.EventType = triggertype.PullRequest.String() case provider.Valid(event, pullRequestsCommentCreated): - opscomments.SetEventTypeAndTargetPR(processedEvent, e.Comment.Content.Raw) + opscomments.SetEventTypeAndTargetPR(processedEvent, e.Comment.Content.Raw, "/") case provider.Valid(event, pullRequestsClosed): processedEvent.EventType = string(triggertype.PullRequestClosed) processedEvent.TriggerTarget = triggertype.PullRequestClosed diff --git a/pkg/provider/gitea/parse_payload.go b/pkg/provider/gitea/parse_payload.go index de899d07d4..0ea5df7da9 100644 --- a/pkg/provider/gitea/parse_payload.go +++ b/pkg/provider/gitea/parse_payload.go @@ -132,7 +132,7 @@ func (v *Provider) ParsePayload(_ context.Context, _ *params.Run, request *http. } processedEvent.TriggerTarget = triggertype.PullRequest if gitEvent.Comment != nil { - opscomments.SetEventTypeAndTargetPR(processedEvent, gitEvent.Comment.Body) + opscomments.SetEventTypeAndTargetPR(processedEvent, gitEvent.Comment.Body, "/") } populateEventFromGiteaPullRequest(processedEvent, gitEvent.PullRequest) if gitEvent.Issue.URL != "" { diff --git a/pkg/provider/github/acl.go b/pkg/provider/github/acl.go index 5adbf9b798..a69a56cded 100644 --- a/pkg/provider/github/acl.go +++ b/pkg/provider/github/acl.go @@ -8,8 +8,10 @@ import ( "github.com/google/go-github/v81/github" "github.com/openshift-pipelines/pipelines-as-code/pkg/acl" + "github.com/openshift-pipelines/pipelines-as-code/pkg/opscomments" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/info" "github.com/openshift-pipelines/pipelines-as-code/pkg/policy" + "github.com/openshift-pipelines/pipelines-as-code/pkg/provider" ) // CheckPolicyAllowing check that policy is allowing the event to be processed @@ -150,7 +152,7 @@ func (v *Provider) aclAllowedOkToTestFromAnOwner(ctx context.Context, event *inf return false, nil } - comments, err := v.GetStringPullRequestComment(ctx, revent, acl.OKToTestCommentRegexp) + comments, err := v.GetStringPullRequestComment(ctx, revent) if err != nil { return false, err } @@ -177,7 +179,10 @@ func (v *Provider) aclAllowedOkToTestCurrentComment(ctx context.Context, revent if err != nil { return false, err } - if acl.MatchRegexp(acl.OKToTestCommentRegexp, comment.GetBody()) { + + gitOpsCommentPrefix := provider.GetGitOpsCommentPrefix(v.repo) + + if opscomments.IsOkToTestComment(comment.GetBody(), gitOpsCommentPrefix) { revent.Sender = comment.User.GetLogin() allowed, err := v.aclCheckAll(ctx, revent) if err != nil { @@ -310,9 +315,9 @@ func (v *Provider) getFileFromDefaultBranch(ctx context.Context, path string, ru return tektonyaml, err } -// GetStringPullRequestComment return the comment if we find a regexp in one of +// GetStringPullRequestComment return the comment if we find an /ok-to-test comment in one of // the comments text of a pull request. -func (v *Provider) GetStringPullRequestComment(ctx context.Context, runevent *info.Event, reg string) ([]*github.IssueComment, error) { +func (v *Provider) GetStringPullRequestComment(ctx context.Context, runevent *info.Event) ([]*github.IssueComment, error) { var ret []*github.IssueComment prNumber, err := convertPullRequestURLtoNumber(runevent.URL) if err != nil { @@ -322,6 +327,9 @@ func (v *Provider) GetStringPullRequestComment(ctx context.Context, runevent *in opt := &github.IssueListCommentsOptions{ ListOptions: github.ListOptions{PerPage: v.PaginedNumber}, } + + gitOpsCommentPrefix := provider.GetGitOpsCommentPrefix(v.repo) + for { comments, resp, err := wrapAPI(v, "list_issue_comments", func() ([]*github.IssueComment, *github.Response, error) { return v.Client().Issues.ListComments(ctx, runevent.Organization, runevent.Repository, @@ -331,7 +339,7 @@ func (v *Provider) GetStringPullRequestComment(ctx context.Context, runevent *in return nil, err } for _, v := range comments { - if acl.MatchRegexp(reg, v.GetBody()) { + if opscomments.IsOkToTestComment(v.GetBody(), gitOpsCommentPrefix) { ret = append(ret, v) } } diff --git a/pkg/provider/github/acl_test.go b/pkg/provider/github/acl_test.go index c7d0de4e8e..611ee8695f 100644 --- a/pkg/provider/github/acl_test.go +++ b/pkg/provider/github/acl_test.go @@ -10,8 +10,10 @@ import ( "github.com/google/go-github/v81/github" "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1" "github.com/openshift-pipelines/pipelines-as-code/pkg/params" + "github.com/openshift-pipelines/pipelines-as-code/pkg/params/clients" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/info" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/settings" + testclient "github.com/openshift-pipelines/pipelines-as-code/pkg/test/clients" ghtesthelper "github.com/openshift-pipelines/pipelines-as-code/pkg/test/github" "go.uber.org/zap" zapobserver "go.uber.org/zap/zaptest/observer" @@ -104,12 +106,13 @@ func TestCheckPolicyAllowing(t *testing.T) { func TestOkToTestComment(t *testing.T) { tests := []struct { - name string - commentsReply string - runevent info.Event - allowed bool - wantErr bool - rememberOkToTest bool + name string + commentsReply string + runevent info.Event + allowed bool + wantErr bool + rememberOkToTest bool + gitOpsCommentPrefix string }{ { name: "good issue comment event", @@ -293,6 +296,26 @@ func TestOkToTestComment(t *testing.T) { wantErr: false, rememberOkToTest: false, }, + { + name: "good issue comment event with custom prefix", + commentsReply: `[{"body": "/pac ok-to-test", "user": {"login": "owner"}}]`, + gitOpsCommentPrefix: "pac", + runevent: info.Event{ + Organization: "owner", + Sender: "nonowner", + EventType: "issue_comment", + Event: &github.IssueCommentEvent{ + Issue: &github.Issue{ + PullRequestLinks: &github.PullRequestLinks{ + HTMLURL: github.Ptr("http://url.com/owner/repo/1"), + }, + }, + }, + }, + allowed: true, + wantErr: false, + rememberOkToTest: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -318,7 +341,9 @@ func TestOkToTestComment(t *testing.T) { observer, _ := zapobserver.New(zap.InfoLevel) logger := zap.New(observer).Sugar() repo := &v1alpha1.Repository{Spec: v1alpha1.RepositorySpec{ - Settings: &v1alpha1.Settings{}, + Settings: &v1alpha1.Settings{ + GitOpsCommandPrefix: tt.gitOpsCommentPrefix, + }, }} pacopts := &info.PacOpts{ Settings: settings.Settings{ @@ -357,12 +382,13 @@ func TestOkToTestCommentSHA(t *testing.T) { requireOkToTestSHA bool pullRequestReply string pullRequestListComment string + gitOpsCommentPrefix string }{ { name: "good issue comment event with sha", commentsReply: `[{"body": "/ok-to-test ABCDEF1", "user": {"login": "owner"}}]`, commentBody: "/ok-to-test ABCDEF1", - pullRequestReply: `{"head": {"sha": "abcdef1234567890"}}`, + pullRequestReply: `{"head": {"sha": "abcdef1234567890"}, "base": {"repo": {"html_url": "http://url.com/owner/repo/1"}}}`, requireOkToTestSHA: true, runevent: info.Event{ Organization: "owner", @@ -383,7 +409,7 @@ func TestOkToTestCommentSHA(t *testing.T) { name: "bad issue comment event with sha", commentsReply: `[{"body": "/ok-to-test 1234567", "user": {"login": "owner"}}]`, commentBody: "/ok-to-test 1234567", - pullRequestReply: `{"head": {"sha": "7654321"}}`, + pullRequestReply: `{"head": {"sha": "7654321"}, "base": {"repo": {"html_url": "http://url.com/owner/repo/1"}}}`, requireOkToTestSHA: true, runevent: info.Event{ Organization: "owner", @@ -404,7 +430,7 @@ func TestOkToTestCommentSHA(t *testing.T) { name: "good issue comment event without sha", commentsReply: `[{"body": "/ok-to-test", "user": {"login": "owner"}}]`, commentBody: "/ok-to-test", - pullRequestReply: `{"head": {"sha": "1234567890"}}`, + pullRequestReply: `{"head": {"sha": "1234567890"}, "base": {"repo": {"html_url": "http://url.com/owner/repo/1"}}}`, requireOkToTestSHA: false, runevent: info.Event{ Organization: "owner", @@ -425,7 +451,7 @@ func TestOkToTestCommentSHA(t *testing.T) { name: "bad issue comment event without sha when required", commentsReply: `[{"body": "/ok-to-test", "user": {"login": "owner"}}]`, commentBody: "/ok-to-test", - pullRequestReply: `{"head": {"sha": "1234567890"}}`, + pullRequestReply: `{"head": {"sha": "1234567890"}, "base": {"repo": {"html_url": "http://url.com/owner/repo/1"}}}`, requireOkToTestSHA: true, runevent: info.Event{ Organization: "owner", @@ -442,6 +468,28 @@ func TestOkToTestCommentSHA(t *testing.T) { allowed: false, wantErr: true, }, + { + name: "good issue comment event with sha and custom prefix", + commentsReply: `[{"body": "/pac ok-to-test ABCDEF1", "user": {"login": "owner"}}]`, + commentBody: "/pac ok-to-test ABCDEF1", + gitOpsCommentPrefix: "pac ", + pullRequestReply: `{"head": {"sha": "abcdef1234567890"}, "base": {"repo": {"html_url": "http://url.com/owner/repo/1"}}}`, + requireOkToTestSHA: true, + runevent: info.Event{ + Organization: "owner", + Sender: "nonowner", + EventType: "issue_comment", + Event: &github.IssueCommentEvent{ + Issue: &github.Issue{ + PullRequestLinks: &github.PullRequestLinks{ + HTMLURL: github.Ptr("http://url.com/owner/repo/1"), + }, + }, + }, + }, + allowed: true, + wantErr: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -469,20 +517,26 @@ func TestOkToTestCommentSHA(t *testing.T) { ctx, _ := rtesting.SetupFakeContext(t) observer, _ := zapobserver.New(zap.InfoLevel) logger := zap.New(observer).Sugar() + tdata := testclient.Data{} repo := &v1alpha1.Repository{Spec: v1alpha1.RepositorySpec{ - Settings: &v1alpha1.Settings{}, + URL: "http://url.com/owner/repo/1", + Settings: &v1alpha1.Settings{ + GitOpsCommandPrefix: tt.gitOpsCommentPrefix, + }, }} + tdata.Repositories = []*v1alpha1.Repository{repo} pacopts := &info.PacOpts{ Settings: settings.Settings{ RequireOkToTestSHA: tt.requireOkToTestSHA, }, } + stdata, _ := testclient.SeedTestData(t, ctx, tdata) gprovider := Provider{ ghClient: fakeclient, repo: repo, Logger: logger, PaginedNumber: 1, - Run: ¶ms.Run{}, + Run: ¶ms.Run{Clients: clients.Clients{PipelineAsCode: stdata.PipelineAsCode}}, pacInfo: pacopts, } diff --git a/pkg/provider/github/github_test.go b/pkg/provider/github/github_test.go index 511194f90b..978dbf8654 100644 --- a/pkg/provider/github/github_test.go +++ b/pkg/provider/github/github_test.go @@ -547,7 +547,6 @@ func TestCheckSenderOrgMembership(t *testing.T) { } func TestGetStringPullRequestComment(t *testing.T) { - regexp := `(^|\n)/retest(\r\n|$)` tests := []struct { name, apiReturn string wantErr bool @@ -557,7 +556,7 @@ func TestGetStringPullRequestComment(t *testing.T) { { name: "Get String from comments", runevent: info.Event{URL: "http://1"}, - apiReturn: `[{"body": "/retest"}]`, + apiReturn: `[{"body": "/ok-to-test"}]`, wantRet: true, }, { @@ -572,14 +571,20 @@ func TestGetStringPullRequestComment(t *testing.T) { fakeclient, mux, _, teardown := ghtesthelper.SetupGH() defer teardown() ctx, _ := rtesting.SetupFakeContext(t) + repo := &v1alpha1.Repository{ + Spec: v1alpha1.RepositorySpec{ + Settings: &v1alpha1.Settings{}, + }, + } gprovider := Provider{ ghClient: fakeclient, + repo: repo, } mux.HandleFunc(fmt.Sprintf("/repos/issues/%s/comments", filepath.Base(tt.runevent.URL)), func(rw http.ResponseWriter, _ *http.Request) { fmt.Fprint(rw, tt.apiReturn) }) - ret, err := gprovider.GetStringPullRequestComment(ctx, &tt.runevent, regexp) + ret, err := gprovider.GetStringPullRequestComment(ctx, &tt.runevent) if tt.wantErr && err == nil { t.Error("We didn't get an error when we wanted one") } diff --git a/pkg/provider/github/parse_payload.go b/pkg/provider/github/parse_payload.go index 4c63502906..eb30bae0c7 100644 --- a/pkg/provider/github/parse_payload.go +++ b/pkg/provider/github/parse_payload.go @@ -16,11 +16,13 @@ import ( github75 "github.com/google/go-github/v75/github" "github.com/google/go-github/v81/github" "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/opscomments" "github.com/openshift-pipelines/pipelines-as-code/pkg/params" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/info" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype" "github.com/openshift-pipelines/pipelines-as-code/pkg/provider" + "github.com/openshift-pipelines/pipelines-as-code/pkg/sort" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" ) @@ -546,7 +548,6 @@ func (v *Provider) handleIssueCommentEvent(ctx context.Context, event *github.Is return info.NewEvent(), fmt.Errorf("issue comment is not coming from a pull_request") } v.userType = event.GetSender().GetType() - opscomments.SetEventTypeAndTargetPR(runevent, event.GetComment().GetBody()) // We are getting the full URL so we have to get the last part to get the PR number, // we don\'t have to care about URL query string/hash and other stuff because @@ -558,23 +559,35 @@ func (v *Provider) handleIssueCommentEvent(ctx context.Context, event *github.Is } v.Logger.Infof("issue_comment: pipelinerun %s on %s/%s#%d has been requested", action, runevent.Organization, runevent.Repository, runevent.PullRequestNumber) - pr, err := v.getPullRequest(ctx, runevent) + processedEvent, err := v.getPullRequest(ctx, runevent) if err != nil { return nil, err } + repo, err := MatchEventURLRepo(ctx, v.Run, processedEvent, "") + if err != nil { + return nil, err + } + if repo == nil { + return nil, fmt.Errorf("no repository found matching URL: %s", processedEvent.URL) + } + + gitOpsCommentPrefix := provider.GetGitOpsCommentPrefix(repo) + + opscomments.SetEventTypeAndTargetPR(runevent, event.GetComment().GetBody(), gitOpsCommentPrefix) + commentBody := event.GetComment().GetBody() - if opscomments.IsOkToTestComment(commentBody) && v.pacInfo.RequireOkToTestSHA { - shaFromCommentRaw := opscomments.GetSHAFromOkToTestComment(commentBody) + if opscomments.IsOkToTestComment(commentBody, gitOpsCommentPrefix) && v.pacInfo.RequireOkToTestSHA { + shaFromCommentRaw := opscomments.GetSHAFromOkToTestComment(commentBody, gitOpsCommentPrefix) if shaFromCommentRaw == "" { v.Logger.Errorf(errSHANotProvided) - if err := v.CreateComment(ctx, runevent, fmt.Sprintf(errSHANotProvidedComment, pr.SHA), ""); err != nil { + if err := v.CreateComment(ctx, runevent, fmt.Sprintf(errSHANotProvidedComment, processedEvent.SHA), ""); err != nil { v.Logger.Errorf("failed to create comment: %v", err) } return info.NewEvent(), errors.New(errSHANotProvided) } shaFromComment := strings.ToLower(shaFromCommentRaw) - prSHALower := strings.ToLower(pr.SHA) + prSHALower := strings.ToLower(processedEvent.SHA) shaLen := len(shaFromCommentRaw) // Validate SHA-1 based on length: @@ -583,27 +596,27 @@ func (v *Provider) handleIssueCommentEvent(ctx context.Context, event *github.Is if shaLen < 40 { // Short SHA: verify it's a valid prefix if !strings.HasPrefix(prSHALower, shaFromComment) { - msg := fmt.Sprintf(errSHAPrefixMismatch, shaFromCommentRaw, pr.SHA) + msg := fmt.Sprintf(errSHAPrefixMismatch, shaFromCommentRaw, processedEvent.SHA) v.Logger.Errorf(msg) if err := v.CreateComment(ctx, runevent, msg, ""); err != nil { v.Logger.Errorf("failed to create comment: %v", err) } - return info.NewEvent(), fmt.Errorf(errSHAPrefixMismatch, shaFromCommentRaw, pr.SHA) + return info.NewEvent(), fmt.Errorf(errSHAPrefixMismatch, shaFromCommentRaw, processedEvent.SHA) } } else if shaLen == 40 { // Full SHA-1: verify exact match if prSHALower != shaFromComment { - msg := fmt.Sprintf(errSHANotMatch, shaFromCommentRaw, pr.SHA) + msg := fmt.Sprintf(errSHANotMatch, shaFromCommentRaw, processedEvent.SHA) v.Logger.Errorf(msg) if err := v.CreateComment(ctx, runevent, msg, ""); err != nil { v.Logger.Errorf("failed to create comment: %v", err) } - return info.NewEvent(), fmt.Errorf(errSHANotMatch, shaFromCommentRaw, pr.SHA) + return info.NewEvent(), fmt.Errorf(errSHANotMatch, shaFromCommentRaw, processedEvent.SHA) } } } - return pr, nil + return processedEvent, nil } func (v *Provider) handleCommitCommentEvent(ctx context.Context, event *github.CommitCommentEvent) (*info.Event, error) { @@ -621,9 +634,20 @@ func (v *Provider) handleCommitCommentEvent(ctx context.Context, event *github.C runevent.HeadURL = runevent.URL runevent.BaseURL = runevent.HeadURL runevent.TriggerTarget = triggertype.Push - opscomments.SetEventTypeAndTargetPR(runevent, event.GetComment().GetBody()) v.userType = event.GetSender().GetType() + repo, err := MatchEventURLRepo(ctx, v.Run, runevent, "") + if err != nil { + return nil, err + } + if repo == nil { + return nil, fmt.Errorf("no repository found matching URL: %s", runevent.URL) + } + + gitOpsCommentPrefix := provider.GetGitOpsCommentPrefix(repo) + + opscomments.SetEventTypeAndTargetPR(runevent, event.GetComment().GetBody(), gitOpsCommentPrefix) + defaultBranch := event.GetRepo().GetDefaultBranch() // Set Event.Repository.DefaultBranch as default branch to runevent.HeadBranch, runevent.BaseBranch @@ -656,17 +680,17 @@ func (v *Provider) handleCommitCommentEvent(ctx context.Context, event *github.C ) // If it is a /test or /retest comment with pipelinerun name figure out the pipelinerun name - if provider.IsTestRetestComment(event.GetComment().GetBody()) { - prName, branchName, tagName, err = provider.GetPipelineRunAndBranchOrTagNameFromTestComment(event.GetComment().GetBody()) + if opscomments.IsTestRetestComment(event.GetComment().GetBody(), gitOpsCommentPrefix) { + prName, branchName, tagName, err = provider.GetPipelineRunAndBranchOrTagNameFromTestComment(event.GetComment().GetBody(), gitOpsCommentPrefix) if err != nil { return runevent, err } runevent.TargetTestPipelineRun = prName } // Check for /cancel comment - if provider.IsCancelComment(event.GetComment().GetBody()) { + if opscomments.IsCancelComment(event.GetComment().GetBody(), gitOpsCommentPrefix) { action = "cancellation" - prName, branchName, tagName, err = provider.GetPipelineRunAndBranchOrTagNameFromCancelComment(event.GetComment().GetBody()) + prName, branchName, tagName, err = provider.GetPipelineRunAndBranchOrTagNameFromCancelComment(event.GetComment().GetBody(), gitOpsCommentPrefix) if err != nil { return runevent, err } @@ -720,7 +744,7 @@ func (v *Provider) handleCommitCommentEvent(ctx context.Context, event *github.C // Check if the specified branch contains the commit if err = v.isHeadCommitOfBranch(ctx, runevent, branchName); err != nil { - if provider.IsCancelComment(event.GetComment().GetBody()) { + if opscomments.IsCancelComment(event.GetComment().GetBody(), gitOpsCommentPrefix) { runevent.CancelPipelineRuns = false } return runevent, err @@ -732,3 +756,20 @@ func (v *Provider) handleCommitCommentEvent(ctx context.Context, event *github.C v.Logger.Infof("github commit_comment: pipelinerun %s on %s/%s#%s has been requested", action, runevent.Organization, runevent.Repository, runevent.SHA) return runevent, nil } + +func MatchEventURLRepo(ctx context.Context, cs *params.Run, event *info.Event, ns string) (*v1alpha1.Repository, error) { + repositories, err := cs.Clients.PipelineAsCode.PipelinesascodeV1alpha1().Repositories(ns).List( + ctx, metav1.ListOptions{}) + if err != nil { + return nil, err + } + sort.RepositorySortByCreationOldestTime(repositories.Items) + for _, repo := range repositories.Items { + repo.Spec.URL = strings.TrimSuffix(repo.Spec.URL, "/") + if repo.Spec.URL == event.URL { + return &repo, nil + } + } + + return nil, nil +} diff --git a/pkg/provider/github/parse_payload_test.go b/pkg/provider/github/parse_payload_test.go index 86f3d9446a..e92265f735 100644 --- a/pkg/provider/github/parse_payload_test.go +++ b/pkg/provider/github/parse_payload_test.go @@ -14,6 +14,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" rtesting "knative.dev/pkg/reconciler/testing" + "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1" "github.com/openshift-pipelines/pipelines-as-code/pkg/params" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/clients" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/info" @@ -76,6 +77,10 @@ var samplePR = github.PullRequest{ SHA: github.Ptr("samplePRsha"), Repo: sampleRepo, }, + Base: &github.PullRequestBranch{ + SHA: github.Ptr("samplePRsha"), + Repo: sampleRepo, + }, } var samplePRAnother = github.PullRequest{ @@ -412,6 +417,8 @@ func TestParsePayLoad(t *testing.T) { isMergeCommit bool skipPushEventForPRCommits bool objectType string + gitopscommentprefix string + wantRepoCRError bool }{ { name: "bad/unknown event", @@ -453,14 +460,14 @@ func TestParsePayLoad(t *testing.T) { wantErrString: "no github client has been initialized", eventType: "issue_comment", triggerTarget: "pull_request", - payloadEventStruct: github.IssueCommentEvent{Action: github.Ptr("created")}, + payloadEventStruct: github.IssueCommentEvent{Action: github.Ptr("created"), Repo: sampleRepo}, }, { name: "bad/issue comment not coming from pull request", eventType: "issue_comment", triggerTarget: "pull_request", githubClient: true, - payloadEventStruct: github.IssueCommentEvent{Action: github.Ptr("created"), Issue: &github.Issue{}}, + payloadEventStruct: github.IssueCommentEvent{Action: github.Ptr("created"), Issue: &github.Issue{}, Repo: sampleRepo}, wantErrString: "issue comment is not coming from a pull_request", }, { @@ -475,6 +482,7 @@ func TestParsePayLoad(t *testing.T) { HTMLURL: github.Ptr("/bad"), }, }, + Repo: sampleRepo, }, wantErrString: "bad pull request number", }, @@ -645,6 +653,72 @@ func TestParsePayLoad(t *testing.T) { shaRet: "samplePRsha", targetPipelinerun: "dummy", }, + { + name: "good/issue comment for retest with prefix", + eventType: "issue_comment", + triggerTarget: triggertype.PullRequest.String(), + githubClient: true, + payloadEventStruct: github.IssueCommentEvent{ + Action: github.Ptr("created"), + Issue: &github.Issue{ + PullRequestLinks: &github.PullRequestLinks{ + HTMLURL: github.Ptr("/777"), + }, + }, + Repo: sampleRepo, + Comment: &github.IssueComment{ + Body: github.Ptr("/pac retest dummy"), + }, + }, + muxReplies: map[string]any{"/repos/owner/reponame/pulls/777": samplePR}, + shaRet: "samplePRsha", + targetPipelinerun: "dummy", + gitopscommentprefix: "pac", + }, + { + name: "good/issue comment for test with prefix", + eventType: "issue_comment", + triggerTarget: triggertype.PullRequest.String(), + githubClient: true, + payloadEventStruct: github.IssueCommentEvent{ + Action: github.Ptr("created"), + Issue: &github.Issue{ + PullRequestLinks: &github.PullRequestLinks{ + HTMLURL: github.Ptr("/777"), + }, + }, + Repo: sampleRepo, + Comment: &github.IssueComment{ + Body: github.Ptr("/pac test dummy"), + }, + }, + muxReplies: map[string]any{"/repos/owner/reponame/pulls/777": samplePR}, + shaRet: "samplePRsha", + targetPipelinerun: "dummy", + gitopscommentprefix: "pac", + }, + { + name: "good/issue comment for cancel with prefix", + eventType: "issue_comment", + triggerTarget: triggertype.PullRequest.String(), + githubClient: true, + payloadEventStruct: github.IssueCommentEvent{ + Action: github.Ptr("created"), + Issue: &github.Issue{ + PullRequestLinks: &github.PullRequestLinks{ + HTMLURL: github.Ptr("/777"), + }, + }, + Repo: sampleRepo, + Comment: &github.IssueComment{ + Body: github.Ptr("/pac cancel dummy"), + }, + }, + muxReplies: map[string]any{"/repos/owner/reponame/pulls/777": samplePR}, + shaRet: "samplePRsha", + targetCancelPipelinerun: "dummy", + gitopscommentprefix: "pac", + }, { name: "good/issue comment for cancel all", eventType: "issue_comment", @@ -691,7 +765,7 @@ func TestParsePayLoad(t *testing.T) { wantErrString: "no github client has been initialized", eventType: "commit_comment", triggerTarget: "push", - payloadEventStruct: github.CommitCommentEvent{Action: github.Ptr("created")}, + payloadEventStruct: github.CommitCommentEvent{Action: github.Ptr("created"), Repo: sampleRepo}, }, { name: "bad/commit comment for event has no repository reference", @@ -738,6 +812,83 @@ func TestParsePayLoad(t *testing.T) { targetPipelinerun: "dummy", wantedBranchName: "main", }, + { + name: "good/commit comment for retest a pr with prefix", + eventType: "commit_comment", + triggerTarget: "push", + githubClient: true, + payloadEventStruct: github.CommitCommentEvent{ + Repo: sampleRepo, + Comment: &github.RepositoryComment{ + CommitID: github.Ptr("samplePRsha"), + HTMLURL: github.Ptr("/777"), + Body: github.Ptr("/pac retest dummy"), + }, + }, + muxReplies: map[string]any{"/repos/owner/reponame/pulls/777": samplePR}, + shaRet: "samplePRsha", + targetPipelinerun: "dummy", + wantedBranchName: "main", + gitopscommentprefix: "pac", + }, + { + name: "good/commit comment for test a pr with prefix", + eventType: "commit_comment", + triggerTarget: "push", + githubClient: true, + payloadEventStruct: github.CommitCommentEvent{ + Repo: sampleRepo, + Comment: &github.RepositoryComment{ + CommitID: github.Ptr("samplePRsha"), + HTMLURL: github.Ptr("/777"), + Body: github.Ptr("/pac test dummy"), + }, + }, + muxReplies: map[string]any{"/repos/owner/reponame/pulls/777": samplePR}, + shaRet: "samplePRsha", + targetPipelinerun: "dummy", + wantedBranchName: "main", + gitopscommentprefix: "pac", + }, + { + name: "good/commit comment for cancel a pr with prefix", + eventType: "commit_comment", + triggerTarget: "push", + githubClient: true, + payloadEventStruct: github.CommitCommentEvent{ + Repo: sampleRepo, + Comment: &github.RepositoryComment{ + CommitID: github.Ptr("samplePRsha"), + HTMLURL: github.Ptr("/777"), + Body: github.Ptr("/pac cancel dummy"), + }, + }, + muxReplies: map[string]any{"/repos/owner/reponame/pulls/777": samplePR}, + shaRet: "samplePRsha", + targetCancelPipelinerun: "dummy", + isCancelPipelineRunEnabled: true, + wantedBranchName: "main", + gitopscommentprefix: "pac", + }, + { + name: "bad/no repository cr matched", + eventType: "commit_comment", + triggerTarget: "push", + githubClient: true, + payloadEventStruct: github.CommitCommentEvent{ + Repo: sampleRepo, + Comment: &github.RepositoryComment{ + CommitID: github.Ptr("samplePRsha"), + HTMLURL: github.Ptr("/777"), + Body: github.Ptr("/pac test dummy"), + }, + }, + muxReplies: map[string]any{"/repos/owner/reponame/pulls/777": samplePR}, + shaRet: "samplePRsha", + gitopscommentprefix: "pac", + wantRepoCRError: true, + wantErrString: "no repository found matching URL: https://github.com/owner/repo", + }, { name: "good/commit comment for retest all", eventType: "commit_comment", @@ -1063,6 +1214,35 @@ func TestParsePayLoad(t *testing.T) { ghClient = nil } + tdata := testclient.Data{} + repo := v1alpha1.Repository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "reponame", + }, + Spec: v1alpha1.RepositorySpec{ + URL: "https://github.com/owner/repo", + }, + } + if !tt.wantRepoCRError { + tdata.Repositories = []*v1alpha1.Repository{&repo} + } + + if len(tdata.Repositories) > 0 && tt.gitopscommentprefix != "" { + tdata.Repositories[0].Spec.Settings = &v1alpha1.Settings{ + GitOpsCommandPrefix: tt.gitopscommentprefix, + } + } + + stdata, _ := testclient.SeedTestData(t, ctx, tdata) + logger, _ := logger.GetLogger() + run := ¶ms.Run{ + Clients: clients.Clients{ + PipelineAsCode: stdata.PipelineAsCode, + Log: logger, + Kube: stdata.Kube, + }, + } + for key, value := range tt.muxReplies { mux.HandleFunc(key, func(rw http.ResponseWriter, _ *http.Request) { bjeez, _ := json.Marshal(value) @@ -1139,7 +1319,6 @@ func TestParsePayLoad(t *testing.T) { }) } - logger, _ := logger.GetLogger() gprovider := Provider{ ghClient: ghClient, Logger: logger, @@ -1150,7 +1329,6 @@ func TestParsePayLoad(t *testing.T) { request := &http.Request{Header: map[string][]string{}} request.Header.Set("X-GitHub-Event", tt.eventType) - run := ¶ms.Run{} bjeez, _ := json.Marshal(tt.payloadEventStruct) jeez := string(bjeez) if tt.jeez != "" { diff --git a/pkg/provider/gitlab/parse_payload.go b/pkg/provider/gitlab/parse_payload.go index 01656a4695..fc23022925 100644 --- a/pkg/provider/gitlab/parse_payload.go +++ b/pkg/provider/gitlab/parse_payload.go @@ -149,7 +149,7 @@ func (v *Provider) ParsePayload(ctx context.Context, run *params.Run, request *h processedEvent.BaseURL = gitEvent.MergeRequest.Target.WebURL processedEvent.HeadURL = gitEvent.MergeRequest.Source.WebURL - opscomments.SetEventTypeAndTargetPR(processedEvent, gitEvent.ObjectAttributes.Note) + opscomments.SetEventTypeAndTargetPR(processedEvent, gitEvent.ObjectAttributes.Note, "/") v.pathWithNamespace = gitEvent.Project.PathWithNamespace processedEvent.Organization, processedEvent.Repository = getOrgRepo(v.pathWithNamespace) processedEvent.TriggerTarget = triggertype.PullRequest @@ -246,7 +246,7 @@ func (v *Provider) handleCommitCommentEvent(ctx context.Context, event *gitlab.C processedEvent.HeadURL = processedEvent.URL processedEvent.BaseURL = processedEvent.URL processedEvent.TriggerTarget = triggertype.Push - opscomments.SetEventTypeAndTargetPR(processedEvent, event.ObjectAttributes.Note) + opscomments.SetEventTypeAndTargetPR(processedEvent, event.ObjectAttributes.Note, "/") // Set Head and Base branch to default_branch of the repo as this comment is made on // a pushed commit. defaultBranch := event.Project.DefaultBranch @@ -269,7 +269,7 @@ func (v *Provider) handleCommitCommentEvent(ctx context.Context, event *gitlab.C // get PipelineRun name from comment if it does contain e.g. `/test pr7` if provider.IsTestRetestComment(event.ObjectAttributes.Note) { - prName, branchName, tagName, err = provider.GetPipelineRunAndBranchOrTagNameFromTestComment(event.ObjectAttributes.Note) + prName, branchName, tagName, err = provider.GetPipelineRunAndBranchOrTagNameFromTestComment(event.ObjectAttributes.Note, "/") if err != nil { return processedEvent, err } @@ -278,7 +278,7 @@ func (v *Provider) handleCommitCommentEvent(ctx context.Context, event *gitlab.C if provider.IsCancelComment(event.ObjectAttributes.Note) { action = "cancellation" - prName, branchName, tagName, err = provider.GetPipelineRunAndBranchOrTagNameFromCancelComment(event.ObjectAttributes.Note) + prName, branchName, tagName, err = provider.GetPipelineRunAndBranchOrTagNameFromCancelComment(event.ObjectAttributes.Note, "/") if err != nil { return processedEvent, err } diff --git a/pkg/provider/provider.go b/pkg/provider/provider.go index 5a131910ca..dad9ec5f62 100644 --- a/pkg/provider/provider.go +++ b/pkg/provider/provider.go @@ -8,6 +8,7 @@ import ( "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1" "github.com/openshift-pipelines/pipelines-as-code/pkg/formatting" + "github.com/openshift-pipelines/pipelines-as-code/pkg/opscomments" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/info" providerstatus "github.com/openshift-pipelines/pipelines-as-code/pkg/provider/status" "gopkg.in/yaml.v2" @@ -109,15 +110,21 @@ func getNameFromComment(typeOfComment, comment string) string { return strings.TrimSpace(getFirstLine[0]) } -func GetPipelineRunAndBranchOrTagNameFromTestComment(comment string) (string, string, string, error) { - if strings.Contains(comment, testComment) { - return getPipelineRunAndBranchOrTagNameFromComment(testComment, comment) +func GetPipelineRunAndBranchOrTagNameFromTestComment(comment, prefix string) (string, string, string, error) { + commentType := opscomments.CommentEventType(comment, prefix) + if commentType == opscomments.TestSingleCommentEventType || commentType == opscomments.TestAllCommentEventType { + typeOfComment := prefix + "test" + return getPipelineRunAndBranchOrTagNameFromComment(typeOfComment, comment) } - return getPipelineRunAndBranchOrTagNameFromComment(retestComment, comment) + // if type of comment is not test single, it is retest single because we've checked the type before + // calling this function. + typeOfComment := prefix + "retest" + return getPipelineRunAndBranchOrTagNameFromComment(typeOfComment, comment) } -func GetPipelineRunAndBranchOrTagNameFromCancelComment(comment string) (string, string, string, error) { - return getPipelineRunAndBranchOrTagNameFromComment(cancelComment, comment) +func GetPipelineRunAndBranchOrTagNameFromCancelComment(comment, prefix string) (string, string, string, error) { + typeOfComment := prefix + "cancel" + return getPipelineRunAndBranchOrTagNameFromComment(typeOfComment, comment) } // getPipelineRunAndBranchOrTagNameFromComment function will take GitOps comment and split the comment @@ -254,3 +261,10 @@ func IsCommentStrategyUpdate(repo *v1alpha1.Repository) bool { return commentStrategy == UpdateCommentStrategy } + +func GetGitOpsCommentPrefix(repo *v1alpha1.Repository) string { + if repo.Spec.Settings != nil && repo.Spec.Settings.GitOpsCommandPrefix != "" { + return fmt.Sprintf(`/%s `, regexp.QuoteMeta(repo.Spec.Settings.GitOpsCommandPrefix)) + } + return "/" +} diff --git a/pkg/provider/provider_test.go b/pkg/provider/provider_test.go index 90a2f4ea08..0b79c36f03 100644 --- a/pkg/provider/provider_test.go +++ b/pkg/provider/provider_test.go @@ -3,6 +3,7 @@ package provider import ( "testing" + "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/info" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/settings" "github.com/openshift-pipelines/pipelines-as-code/pkg/provider/status" @@ -290,6 +291,7 @@ func TestGetPipelineRunAndBranchNameFromTestComment(t *testing.T) { comment string branchName string prName string + prefix string wantError bool }{ { @@ -381,11 +383,38 @@ func TestGetPipelineRunAndBranchNameFromTestComment(t *testing.T) { prName: "abc-01-pr", wantError: false, }, + { + name: "test a pipeline with prefix", + comment: "/pac-test abc-01-pr", + prName: "abc-01-pr", + branchName: "", + prefix: "/pac-", + wantError: false, + }, + { + name: "retest a pipeline with prefix", + comment: "/pac-retest abc-01-pr", + prName: "abc-01-pr", + branchName: "", + prefix: "/pac-", + wantError: false, + }, + { + name: "test a pipeline with prefix and key value", + comment: "/pac-test abc-01-pr key=value", + prName: "abc-01-pr", + branchName: "", + prefix: "/pac-", + wantError: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - prName, branchName, _, err := GetPipelineRunAndBranchOrTagNameFromTestComment(tt.comment) + if tt.prefix == "" { + tt.prefix = "/" + } + prName, branchName, _, err := GetPipelineRunAndBranchOrTagNameFromTestComment(tt.comment, tt.prefix) assert.Equal(t, tt.wantError, err != nil) assert.Equal(t, tt.branchName, branchName) assert.Equal(t, tt.prName, prName) @@ -399,6 +428,7 @@ func TestGetPipelineRunAndBranchNameFromCancelComment(t *testing.T) { comment string branchName string prName string + prefix string wantError bool }{ { @@ -469,11 +499,49 @@ func TestGetPipelineRunAndBranchNameFromCancelComment(t *testing.T) { comment: "/cancel invalidname:nightly", wantError: true, }, + { + name: "cancel all with prefix", + comment: "/pac-cancel", + prefix: "/pac-", + wantError: false, + }, + { + name: "cancel single with prefix", + comment: "/pac-cancel abc-pr", + prName: "abc-pr", + prefix: "/pac-", + wantError: false, + }, + { + name: "cancel with branch and prefix", + comment: "/pac-cancel branch:test", + branchName: "test", + prefix: "/pac-", + wantError: false, + }, + { + name: "cancel single with branch and prefix", + comment: "/pac-cancel abc-pr branch:test", + prName: "abc-pr", + branchName: "test", + prefix: "/pac-", + wantError: false, + }, + { + name: "different prefix cancel single", + comment: "/myteam-cancel xyz-pr", + prName: "xyz-pr", + prefix: "/myteam-", + wantError: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - prName, branchName, _, err := GetPipelineRunAndBranchOrTagNameFromCancelComment(tt.comment) + if tt.prefix == "" { + tt.prefix = "/" + } + prName, branchName, _, err := GetPipelineRunAndBranchOrTagNameFromCancelComment(tt.comment, tt.prefix) assert.Equal(t, tt.wantError, err != nil) assert.Equal(t, tt.branchName, branchName) assert.Equal(t, tt.prName, prName) @@ -664,3 +732,138 @@ func TestSkipCI(t *testing.T) { }) } } + +func TestGetGitOpsCommentPrefix(t *testing.T) { + tests := []struct { + name string + repo *v1alpha1.Repository + want string + }{ + { + name: "no settings returns default prefix", + repo: &v1alpha1.Repository{ + Spec: v1alpha1.RepositorySpec{ + Settings: nil, + }, + }, + want: "/", + }, + { + name: "empty GitOpsCommandPrefix returns default prefix", + repo: &v1alpha1.Repository{ + Spec: v1alpha1.RepositorySpec{ + Settings: &v1alpha1.Settings{ + GitOpsCommandPrefix: "", + }, + }, + }, + want: "/", + }, + { + name: "custom prefix present in comment returns custom prefix", + repo: &v1alpha1.Repository{ + Spec: v1alpha1.RepositorySpec{ + Settings: &v1alpha1.Settings{ + GitOpsCommandPrefix: "pac", + }, + }, + }, + want: "/pac ", + }, + { + name: "custom prefix not present in comment returns default prefix", + repo: &v1alpha1.Repository{ + Spec: v1alpha1.RepositorySpec{ + Settings: &v1alpha1.Settings{ + GitOpsCommandPrefix: "pac", + }, + }, + }, + want: "/pac ", + }, + { + name: "custom prefix with retest command", + repo: &v1alpha1.Repository{ + Spec: v1alpha1.RepositorySpec{ + Settings: &v1alpha1.Settings{ + GitOpsCommandPrefix: "pac", + }, + }, + }, + want: "/pac ", + }, + { + name: "custom prefix with cancel command", + repo: &v1alpha1.Repository{ + Spec: v1alpha1.RepositorySpec{ + Settings: &v1alpha1.Settings{ + GitOpsCommandPrefix: "pac", + }, + }, + }, + want: "/pac ", + }, + { + name: "custom prefix with ok-to-test command", + repo: &v1alpha1.Repository{ + Spec: v1alpha1.RepositorySpec{ + Settings: &v1alpha1.Settings{ + GitOpsCommandPrefix: "pac", + }, + }, + }, + want: "/pac ", + }, + { + name: "multiline comment with custom prefix", + repo: &v1alpha1.Repository{ + Spec: v1alpha1.RepositorySpec{ + Settings: &v1alpha1.Settings{ + GitOpsCommandPrefix: "pac", + }, + }, + }, + want: "/pac ", + }, + { + name: "multiline comment with default prefix only", + repo: &v1alpha1.Repository{ + Spec: v1alpha1.RepositorySpec{ + Settings: &v1alpha1.Settings{ + GitOpsCommandPrefix: "pac", + }, + }, + }, + want: "/pac ", + }, + { + name: "different custom prefix", + repo: &v1alpha1.Repository{ + Spec: v1alpha1.RepositorySpec{ + Settings: &v1alpha1.Settings{ + GitOpsCommandPrefix: "myteam", + }, + }, + }, + want: "/myteam ", + }, + { + name: "comment with both default and custom prefix prefers custom", + repo: &v1alpha1.Repository{ + Spec: v1alpha1.RepositorySpec{ + Settings: &v1alpha1.Settings{ + GitOpsCommandPrefix: "pac", + }, + }, + }, + want: "/pac ", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := GetGitOpsCommentPrefix(tt.repo) + assert.Equal(t, tt.want, got) + }) + } +} diff --git a/test/github_comment_strategy_update_test.go b/test/github_comment_strategy_update_test.go index b0008780dc..529bbd73c2 100644 --- a/test/github_comment_strategy_update_test.go +++ b/test/github_comment_strategy_update_test.go @@ -33,7 +33,7 @@ func TestGithubGHEWebhookCommentStrategyUpdateCELErrorReplacement(t *testing.T) NoStatusCheck: true, } - commentStrategy := v1alpha1.Settings{ + commentStrategy := &v1alpha1.Settings{ Github: &v1alpha1.GithubSettings{ CommentStrategy: provider.UpdateCommentStrategy, }, @@ -151,7 +151,7 @@ func TestGithubGHEWebhookCommentStrategyUpdateMultiplePLRs(t *testing.T) { Webhook: true, } - commentStrategy := v1alpha1.Settings{ + commentStrategy := &v1alpha1.Settings{ Github: &v1alpha1.GithubSettings{ CommentStrategy: provider.UpdateCommentStrategy, }, @@ -259,7 +259,7 @@ func TestGithubGHEWebhookCommentStrategyUpdateMarkerMatchingWithRegexChars(t *te Webhook: true, } - commentStrategy := v1alpha1.Settings{ + commentStrategy := &v1alpha1.Settings{ Github: &v1alpha1.GithubSettings{ CommentStrategy: provider.UpdateCommentStrategy, }, diff --git a/test/github_gitops_commands_prefix_test.go b/test/github_gitops_commands_prefix_test.go new file mode 100644 index 0000000000..1499c4a52e --- /dev/null +++ b/test/github_gitops_commands_prefix_test.go @@ -0,0 +1,115 @@ +//go:build e2e + +package test + +import ( + "context" + "fmt" + "testing" + + "github.com/google/go-github/v81/github" + "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1" + "github.com/openshift-pipelines/pipelines-as-code/pkg/opscomments" + tgithub "github.com/openshift-pipelines/pipelines-as-code/test/pkg/github" + "github.com/openshift-pipelines/pipelines-as-code/test/pkg/options" + twait "github.com/openshift-pipelines/pipelines-as-code/test/pkg/wait" + tektonv1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" + "gotest.tools/v3/assert" + corev1 "k8s.io/api/core/v1" +) + +func TestGithubPullRequestCustomGitOpsCommandPrefix(t *testing.T) { + ctx := context.Background() + customPrefix := "pac" + + g := &tgithub.PRTest{ + Label: "Github test custom GitOps command prefix", + YamlFiles: []string{"testdata/pipelinerun-gitops.yaml"}, + Options: options.E2E{ + Settings: &v1alpha1.Settings{ + GitOpsCommandPrefix: customPrefix, + }, + }, + } + + g.RunPullRequest(ctx, t) + defer g.TearDown(ctx, t) + + customTestComment := fmt.Sprintf("/%s test", customPrefix) + g.Cnx.Clients.Log.Infof("Creating %s comment on PullRequest", customTestComment) + _, _, err := g.Provider.Client().Issues.CreateComment(ctx, + g.Options.Organization, + g.Options.Repo, g.PRNumber, + &github.IssueComment{Body: github.Ptr(customTestComment)}) + assert.NilError(t, err) + + g.Cnx.Clients.Log.Infof("Wait for repository to be updated with custom prefix command") + waitOpts := twait.Opts{ + RepoName: g.TargetNamespace, + Namespace: g.TargetNamespace, + MinNumberStatus: 2, + PollTimeout: twait.DefaultTimeout, + TargetSHA: g.SHA, + } + repo, err := twait.UntilRepositoryUpdated(ctx, g.Cnx.Clients, waitOpts) + assert.NilError(t, err) + + g.Cnx.Clients.Log.Infof("Check if repository status shows succeeded") + assert.Equal(t, corev1.ConditionTrue, repo.Status[len(repo.Status)-1].Conditions[0].Status) + + customTestComment = fmt.Sprintf("/%s test pr-gitops-comment", customPrefix) + g.Cnx.Clients.Log.Infof("Creating %s comment on PullRequest", customTestComment) + _, _, err = g.Provider.Client().Issues.CreateComment(ctx, + g.Options.Organization, + g.Options.Repo, g.PRNumber, + &github.IssueComment{Body: github.Ptr(customTestComment)}) + assert.NilError(t, err) + + twait.Succeeded(ctx, t, g.Cnx, g.Options, twait.SuccessOpt{ + TargetNS: g.TargetNamespace, + OnEvent: opscomments.TestSingleCommentEventType.String(), + NumberofPRMatch: 3, + MinNumberStatus: 3, + SHA: g.SHA, + }) +} + +func TestGithubPullRequestCustomPrefixCancel(t *testing.T) { + ctx := context.Background() + customPrefix := "pac" + + g := &tgithub.PRTest{ + Label: "Github test custom prefix cancel", + YamlFiles: []string{"testdata/pipelinerun-gitops.yaml"}, + Options: options.E2E{ + Settings: &v1alpha1.Settings{ + GitOpsCommandPrefix: customPrefix, + }, + }, + NoStatusCheck: true, + } + + g.RunPullRequest(ctx, t) + defer g.TearDown(ctx, t) + + waitOpts := twait.Opts{ + RepoName: g.TargetNamespace, + Namespace: g.TargetNamespace, + MinNumberStatus: 1, + PollTimeout: twait.DefaultTimeout, + TargetSHA: g.SHA, + } + err := twait.UntilPipelineRunCreated(ctx, g.Cnx.Clients, waitOpts) + assert.NilError(t, err) + // Cancel with custom prefix + customCancelComment := fmt.Sprintf("/%s cancel", customPrefix) + g.Cnx.Clients.Log.Infof("Creating %s comment on PullRequest", customCancelComment) + _, _, err = g.Provider.Client().Issues.CreateComment(ctx, + g.Options.Organization, + g.Options.Repo, g.PRNumber, + &github.IssueComment{Body: github.Ptr(customCancelComment)}) + assert.NilError(t, err) + + err = twait.UntilPipelineRunHasReason(ctx, g.Cnx.Clients, tektonv1.PipelineRunReasonCancelled, waitOpts) + assert.NilError(t, err) +} diff --git a/test/github_pullrequest_test.go b/test/github_pullrequest_test.go index 64da6efbc2..d7ec7967c7 100644 --- a/test/github_pullrequest_test.go +++ b/test/github_pullrequest_test.go @@ -669,7 +669,7 @@ func TestGithubGHEWebhookDisableCommentsOnPR(t *testing.T) { Webhook: true, } - commentStrategy := v1alpha1.Settings{ + commentStrategy := &v1alpha1.Settings{ Github: &v1alpha1.GithubSettings{ CommentStrategy: "disable_all", }, diff --git a/test/github_push_retest_test.go b/test/github_push_retest_test.go index 33ec4cb10c..99d52576d4 100644 --- a/test/github_push_retest_test.go +++ b/test/github_push_retest_test.go @@ -243,7 +243,7 @@ func TestGithubGHEPullRequestRetestPullRequestNumberSubstitution(t *testing.T) { t.Errorf("Repository %s not found in %s", opts.Organization, opts.Repo) } - if g.Options.Settings.Github != nil { + if g.Options.Settings != nil && g.Options.Settings.Github != nil { opts.Settings = g.Options.Settings } err = tgithub.CreateCRD(ctx, t, repoinfo, runcnx, opts, ghcnx, targetNS) diff --git a/test/pkg/github/crd.go b/test/pkg/github/crd.go index 0984e171a2..554d02c69d 100644 --- a/test/pkg/github/crd.go +++ b/test/pkg/github/crd.go @@ -23,7 +23,7 @@ func CreateCRD(ctx context.Context, t *testing.T, repoinfo *github.Repository, r }, Spec: v1alpha1.RepositorySpec{ URL: repoinfo.GetHTMLURL(), - Settings: &opts.Settings, + Settings: opts.Settings, }, } diff --git a/test/pkg/github/pr.go b/test/pkg/github/pr.go index cf06540bfd..1f68d6af26 100644 --- a/test/pkg/github/pr.go +++ b/test/pkg/github/pr.go @@ -168,7 +168,7 @@ func (g *PRTest) RunPullRequest(ctx context.Context, t *testing.T) { } } - if g.Options.Settings.Github != nil { + if g.Options.Settings != nil { opts.Settings = g.Options.Settings } err = CreateCRD(ctx, t, repoinfo, runcnx, opts, ghcnx, targetNS) diff --git a/test/pkg/options/options.go b/test/pkg/options/options.go index 5fe83a7fce..67f565f8f4 100644 --- a/test/pkg/options/options.go +++ b/test/pkg/options/options.go @@ -10,7 +10,7 @@ type E2E struct { UserName string Password string LightweightTag bool - Settings v1alpha1.Settings + Settings *v1alpha1.Settings } var ( From 1415cb9b0b5048405dac3bf4e17c426c672cf9be Mon Sep 17 00:00:00 2001 From: Zaki Shaikh Date: Mon, 9 Mar 2026 12:11:34 +0530 Subject: [PATCH 2/2] fix: use dynamic prefix in retest error Convert ErrNoFailedPipelineToRetest from a sentinel error variable to a custom error type that accepts the gitops command prefix. This ensures the error message displays the correct command syntax (e.g. /pac retest) based on the repository's configured prefix. Signed-off-by: Zaki Shaikh Assisted-by: Claude Opus 4.6 (via Claude Code) --- .../docs/guides/gitops-commands/advanced.md | 2 ++ pkg/apis/pipelinesascode/v1alpha1/types.go | 4 ++++ .../pipelinesascode/v1alpha1/types_test.go | 4 ++++ pkg/matcher/annotation_matcher.go | 20 ++++++++++++++----- pkg/matcher/annotation_matcher_test.go | 9 +++++++-- pkg/pipelineascode/match.go | 4 +++- pkg/provider/github/acl.go | 4 +++- pkg/provider/github/detect.go | 17 +++++++++------- pkg/provider/provider.go | 11 +++++----- test/pkg/github/pr.go | 12 ++++------- 10 files changed, 58 insertions(+), 29 deletions(-) diff --git a/docs/content/docs/guides/gitops-commands/advanced.md b/docs/content/docs/guides/gitops-commands/advanced.md index 661b5a4f35..2c0deb03a9 100644 --- a/docs/content/docs/guides/gitops-commands/advanced.md +++ b/docs/content/docs/guides/gitops-commands/advanced.md @@ -87,6 +87,8 @@ Example: /pac test ``` +You can also configure GitOps command prefix in [Global Repository CR]({{< relref "/docs/operations/global-repository-settings" >}}) so that it will be applied to all Repository CRs those are not defining their own prefix. + ## Cancelling a PipelineRun **What it does:** The `/cancel` command stops running PipelineRuns by commenting on the pull request. diff --git a/pkg/apis/pipelinesascode/v1alpha1/types.go b/pkg/apis/pipelinesascode/v1alpha1/types.go index d31cf2f3ba..d9915626e7 100644 --- a/pkg/apis/pipelinesascode/v1alpha1/types.go +++ b/pkg/apis/pipelinesascode/v1alpha1/types.go @@ -228,6 +228,10 @@ func (s *Settings) Merge(newSettings *Settings) { if newSettings.AIAnalysis != nil && s.AIAnalysis == nil { s.AIAnalysis = newSettings.AIAnalysis } + + if newSettings.GitOpsCommandPrefix != "" && s.GitOpsCommandPrefix == "" { + s.GitOpsCommandPrefix = newSettings.GitOpsCommandPrefix + } } type Policy struct { diff --git a/pkg/apis/pipelinesascode/v1alpha1/types_test.go b/pkg/apis/pipelinesascode/v1alpha1/types_test.go index 937759084f..183aaab2c4 100644 --- a/pkg/apis/pipelinesascode/v1alpha1/types_test.go +++ b/pkg/apis/pipelinesascode/v1alpha1/types_test.go @@ -58,6 +58,7 @@ func TestMergeSpecs(t *testing.T) { Policy: &Policy{ OkToTest: []string{"ok1", "ok2"}, }, + GitOpsCommandPrefix: "pac", }, // Initialize as needed GitProvider: gp, // Initialize as needed Incomings: incomings, @@ -71,6 +72,7 @@ func TestMergeSpecs(t *testing.T) { Policy: &Policy{ OkToTest: []string{"ok1", "ok2"}, }, + GitOpsCommandPrefix: "pac", }, Incomings: incomings, GitProvider: gp, @@ -87,6 +89,7 @@ func TestMergeSpecs(t *testing.T) { Policy: &Policy{ OkToTest: []string{"ok1", "ok2"}, }, + GitOpsCommandPrefix: "pac", }, // Initialize as needed GitProvider: &GitProvider{}, // Initialize as needed }, @@ -110,6 +113,7 @@ func TestMergeSpecs(t *testing.T) { Policy: &Policy{ OkToTest: []string{"ok1", "ok2"}, }, + GitOpsCommandPrefix: "pac", }, Incomings: incomings, GitProvider: gp, diff --git a/pkg/matcher/annotation_matcher.go b/pkg/matcher/annotation_matcher.go index 73e450e2a7..26c9443eeb 100644 --- a/pkg/matcher/annotation_matcher.go +++ b/pkg/matcher/annotation_matcher.go @@ -2,7 +2,6 @@ package matcher import ( "context" - "errors" "fmt" "regexp" "strings" @@ -37,9 +36,20 @@ const ( maxCommentLogLength = 160 ) -// ErrNoFailedPipelineToRetest is returned when /retest or /ok-to-test is used -// but all matching pipelines have already succeeded for this commit. -var ErrNoFailedPipelineToRetest = errors.New("All PipelineRuns for this commit have already succeeded. Use `/retest ` to re-run a specific pipeline or `/test` to re-run all pipelines.") // nolint:revive,staticcheck +// NoFailedPipelineToRetestError is an error type returned when /retest or +// /ok-to-test is used but all matching pipelines have already succeeded for +// this commit. The underlying string value is the gitops comment prefix used +// to construct the user-facing error message with the correct command syntax. +type NoFailedPipelineToRetestError string + +func (e NoFailedPipelineToRetestError) Error() string { + return fmt.Sprintf("All PipelineRuns for this commit have already succeeded. Use `%sretest ` to re-run a specific pipeline or `%stest` to re-run all pipelines.", string(e), string(e)) +} + +func (e NoFailedPipelineToRetestError) Is(target error) bool { + _, ok := target.(NoFailedPipelineToRetestError) + return ok +} // prunBranch is value from annotations and baseBranch is event.Base value from event. func branchMatch(prunBranch, baseBranch string) bool { @@ -426,7 +436,7 @@ func MatchPipelinerunByAnnotation(ctx context.Context, logger *zap.SugaredLogger logger.Debugf("MatchPipelinerunByAnnotation: filtering successful templates for event_type=%s", event.EventType) filtered := filterSuccessfulTemplates(ctx, logger, cs, event, repo, matchedPRs) if len(filtered) == 0 { - return nil, ErrNoFailedPipelineToRetest + return nil, NoFailedPipelineToRetestError(provider.GetGitOpsCommentPrefix(repo)) } return filtered, nil } diff --git a/pkg/matcher/annotation_matcher_test.go b/pkg/matcher/annotation_matcher_test.go index 554d09c18c..c1dca5e08c 100644 --- a/pkg/matcher/annotation_matcher_test.go +++ b/pkg/matcher/annotation_matcher_test.go @@ -2281,7 +2281,12 @@ func TestMatchPipelinerunByAnnotation(t *testing.T) { wantErrNoFailedPipelineToRetest: true, repo: &v1alpha1.Repository{ ObjectMeta: metav1.ObjectMeta{Name: "test-repo", Namespace: "test-ns"}, - Spec: v1alpha1.RepositorySpec{URL: "https://github.com/org/repo"}, + Spec: v1alpha1.RepositorySpec{ + URL: "https://github.com/org/repo", + Settings: &v1alpha1.Settings{ + GitOpsCommandPrefix: "pac", + }, + }, }, seedData: &testclient.Data{ PipelineRuns: []*tektonv1.PipelineRun{ @@ -2335,7 +2340,7 @@ func TestMatchPipelinerunByAnnotation(t *testing.T) { matches, err := MatchPipelinerunByAnnotation(ctx, logger, tt.args.pruns, cs, &tt.args.runevent, &ghprovider.Provider{}, eventEmitter, repo, true) if tt.wantErrNoFailedPipelineToRetest { assert.Assert(t, err != nil, "expected ErrNoFailedPipelineToRetest") - assert.Assert(t, errors.Is(err, ErrNoFailedPipelineToRetest), "expected ErrNoFailedPipelineToRetest, got: %v", err) + assert.Assert(t, errors.Is(err, NoFailedPipelineToRetestError("/pac ")), "expected ErrNoFailedPipelineToRetest, got: %v", err) assert.Equal(t, len(matches), 0, "expected no matches when all pipelines already succeeded") return } diff --git a/pkg/pipelineascode/match.go b/pkg/pipelineascode/match.go index 43a3a9a1c4..74473dbf7b 100644 --- a/pkg/pipelineascode/match.go +++ b/pkg/pipelineascode/match.go @@ -14,6 +14,7 @@ import ( "github.com/openshift-pipelines/pipelines-as-code/pkg/matcher" "github.com/openshift-pipelines/pipelines-as-code/pkg/opscomments" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype" + "github.com/openshift-pipelines/pipelines-as-code/pkg/provider" providerstatus "github.com/openshift-pipelines/pipelines-as-code/pkg/provider/status" "github.com/openshift-pipelines/pipelines-as-code/pkg/resolve" "github.com/openshift-pipelines/pipelines-as-code/pkg/secrets" @@ -268,8 +269,9 @@ func (p *PacRun) getPipelineRunsFromRepo(ctx context.Context, repo *v1alpha1.Rep var matchedPRs []matcher.Match if p.event.TargetTestPipelineRun == "" { if matchedPRs, err = matcher.MatchPipelinerunByAnnotation(ctx, p.logger, pipelineRuns, p.run, p.event, p.vcx, p.eventEmitter, repo, true); err != nil { + prefix := provider.GetGitOpsCommentPrefix(repo) // Check if all pipelines have already succeeded - post comment so user gets feedback - if errors.Is(err, matcher.ErrNoFailedPipelineToRetest) { + if errors.Is(err, matcher.NoFailedPipelineToRetestError(prefix)) { p.logger.Infof("RepositoryAllPipelinesSucceeded: %s", err.Error()) p.eventEmitter.EmitMessage(nil, zap.InfoLevel, "RepositoryAllPipelinesSucceeded", err.Error()) if commentErr := p.vcx.CreateComment(ctx, p.event, err.Error(), ""); commentErr != nil { diff --git a/pkg/provider/github/acl.go b/pkg/provider/github/acl.go index a69a56cded..fd3237eb72 100644 --- a/pkg/provider/github/acl.go +++ b/pkg/provider/github/acl.go @@ -81,8 +81,10 @@ func (v *Provider) IsAllowed(ctx context.Context, event *info.Event) (bool, erro Logger: v.Logger, } + prefix := provider.GetGitOpsCommentPrefix(v.repo) + // Try to detect a policy rule allowing this - tType, _ := v.detectTriggerTypeFromPayload("", event.Event) + tType, _ := v.detectTriggerTypeFromPayload("", event.Event, prefix) policyAllowed, policyReason := aclPolicy.IsAllowed(ctx, tType) switch policyAllowed { diff --git a/pkg/provider/github/detect.go b/pkg/provider/github/detect.go index 7c5c363832..90cb903708 100644 --- a/pkg/provider/github/detect.go +++ b/pkg/provider/github/detect.go @@ -6,6 +6,7 @@ import ( "net/http" "github.com/google/go-github/v81/github" + "github.com/openshift-pipelines/pipelines-as-code/pkg/opscomments" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype" "github.com/openshift-pipelines/pipelines-as-code/pkg/provider" "go.uber.org/zap" @@ -45,7 +46,9 @@ func (v *Provider) Detect(req *http.Request, payload string, logger *zap.Sugared } _ = json.Unmarshal([]byte(payload), &eventInt) - eType, errReason := v.detectTriggerTypeFromPayload(eventType, eventInt) + // at this moment we don't any info about repository CR so passing "/" as gitOpsCommentPrefix + // if its valid event then that will be done in ParsePayload function ahead. + eType, errReason := v.detectTriggerTypeFromPayload(eventType, eventInt, "/") if eType != "" { return setLoggerAndProceed(true, "", nil) } @@ -56,7 +59,7 @@ func (v *Provider) Detect(req *http.Request, payload string, logger *zap.Sugared // detectTriggerTypeFromPayload will detect the event type from the payload, // filtering out the events that are not supported. // first arg will get the event type and the second one will get an error string explaining why it's not supported. -func (v *Provider) detectTriggerTypeFromPayload(ghEventType string, eventInt any) (triggertype.Trigger, string) { +func (v *Provider) detectTriggerTypeFromPayload(ghEventType string, eventInt any, gitOpsCommentPrefix string) (triggertype.Trigger, string) { switch event := eventInt.(type) { case *github.PushEvent: if event.GetPusher() != nil { @@ -76,13 +79,13 @@ func (v *Provider) detectTriggerTypeFromPayload(ghEventType string, eventInt any if event.GetAction() == "created" && event.GetIssue().IsPullRequest() && event.GetIssue().GetState() == "open" { - if provider.IsTestRetestComment(event.GetComment().GetBody()) { + if opscomments.IsTestRetestComment(event.GetComment().GetBody(), gitOpsCommentPrefix) { return triggertype.Retest, "" } - if provider.IsOkToTestComment(event.GetComment().GetBody()) { + if opscomments.IsOkToTestComment(event.GetComment().GetBody(), gitOpsCommentPrefix) { return triggertype.OkToTest, "" } - if provider.IsCancelComment(event.GetComment().GetBody()) { + if opscomments.IsCancelComment(event.GetComment().GetBody(), gitOpsCommentPrefix) { return triggertype.Cancel, "" } } @@ -99,10 +102,10 @@ func (v *Provider) detectTriggerTypeFromPayload(ghEventType string, eventInt any return "", fmt.Sprintf("check_run: unsupported action \"%s\"", event.GetAction()) case *github.CommitCommentEvent: if event.GetAction() == "created" { - if provider.IsTestRetestComment(event.GetComment().GetBody()) { + if opscomments.IsTestRetestComment(event.GetComment().GetBody(), gitOpsCommentPrefix) { return triggertype.Retest, "" } - if provider.IsCancelComment(event.GetComment().GetBody()) { + if opscomments.IsCancelComment(event.GetComment().GetBody(), gitOpsCommentPrefix) { return triggertype.Cancel, "" } // Here, the `/ok-to-test` command is ignored because it is intended for pull requests. diff --git a/pkg/provider/provider.go b/pkg/provider/provider.go index dad9ec5f62..f58f27d7d8 100644 --- a/pkg/provider/provider.go +++ b/pkg/provider/provider.go @@ -112,13 +112,14 @@ func getNameFromComment(typeOfComment, comment string) string { func GetPipelineRunAndBranchOrTagNameFromTestComment(comment, prefix string) (string, string, string, error) { commentType := opscomments.CommentEventType(comment, prefix) + typeOfComment := "" if commentType == opscomments.TestSingleCommentEventType || commentType == opscomments.TestAllCommentEventType { - typeOfComment := prefix + "test" - return getPipelineRunAndBranchOrTagNameFromComment(typeOfComment, comment) + typeOfComment = prefix + "test" + } else { + // if type of comment is not test single, it is retest single because we've checked the type before + // calling this function. + typeOfComment = prefix + "retest" } - // if type of comment is not test single, it is retest single because we've checked the type before - // calling this function. - typeOfComment := prefix + "retest" return getPipelineRunAndBranchOrTagNameFromComment(typeOfComment, comment) } diff --git a/test/pkg/github/pr.go b/test/pkg/github/pr.go index 1f68d6af26..65eb9b50fc 100644 --- a/test/pkg/github/pr.go +++ b/test/pkg/github/pr.go @@ -133,11 +133,9 @@ func (g *PRTest) RunPullRequest(ctx context.Context, t *testing.T) { assert.NilError(t, err) g.Logger = runcnx.Clients.Log g.Cnx = runcnx - preSettings := g.Options.Settings + settings := g.Options.Settings g.Options = opts - if preSettings.Github != nil { - g.Options.Settings = preSettings - } + g.Options.Settings = settings g.Provider = ghcnx g.TargetNamespace = targetNS g.CommitTitle = fmt.Sprintf("Testing %s with Github APPS integration on %s", g.Label, targetNS) @@ -265,9 +263,7 @@ func (g *PRTest) RunPushRequest(ctx context.Context, t *testing.T) { g.Cnx = runcnx preSettings := g.Options.Settings g.Options = opts - if preSettings.Github != nil { - g.Options.Settings = preSettings - } + g.Options.Settings = preSettings g.Provider = ghcnx g.TargetNamespace = targetNS g.PRNumber = -1 @@ -306,7 +302,7 @@ func (g *PRTest) RunPushRequest(ctx context.Context, t *testing.T) { } } - if g.Options.Settings.Github != nil { + if g.Options.Settings != nil && g.Options.Settings.Github != nil { opts.Settings = g.Options.Settings } err = CreateCRD(ctx, t, repoinfo, runcnx, opts, ghcnx, targetNS)