diff --git a/docs/content/docs/guide/gitops_commands.md b/docs/content/docs/guide/gitops_commands.md index ee2d1df994..36d33f936c 100644 --- a/docs/content/docs/guide/gitops_commands.md +++ b/docs/content/docs/guide/gitops_commands.md @@ -70,6 +70,9 @@ This means: 3. `/retest branch:test` 4. `/test branch:test` +Please note that the `/ok-to-test` command does not work on pushed commits, as it is specifically intended for pull requests to manage authorization. Since only authorized users are allowed to send `GitOps` commands on pushed commits, +there is no need to use the `ok-to-test` command in this context. + To issue a `GitOps` comment on a pushed commit, you can follow these steps: 1. Go to your repository. diff --git a/pkg/provider/bitbucketserver/parse_payload.go b/pkg/provider/bitbucketserver/parse_payload.go index a7b594e014..b3e9ee9672 100644 --- a/pkg/provider/bitbucketserver/parse_payload.go +++ b/pkg/provider/bitbucketserver/parse_payload.go @@ -159,6 +159,11 @@ func (v *Provider) ParsePayload(_ context.Context, _ *params.Run, request *http. processedEvent.EventType = triggertype.Push.String() processedEvent.Organization = e.Repository.Project.Key processedEvent.Repository = e.Repository.Slug + + if len(e.Changes) == 0 { + return nil, fmt.Errorf("push event contains no commits under 'changes'; cannot proceed") + } + processedEvent.SHA = e.Changes[0].ToHash processedEvent.URL = e.Repository.Links.Self[0].Href processedEvent.BaseBranch = e.Changes[0].RefID diff --git a/pkg/provider/bitbucketserver/parse_payload_test.go b/pkg/provider/bitbucketserver/parse_payload_test.go index e208a5d910..6e33fa6041 100644 --- a/pkg/provider/bitbucketserver/parse_payload_test.go +++ b/pkg/provider/bitbucketserver/parse_payload_test.go @@ -612,9 +612,16 @@ func TestParsePayload(t *testing.T) { { name: "good/push", eventType: "repo:refs_changed", - payloadEvent: bbv1test.MakePushEvent(ev1), + payloadEvent: bbv1test.MakePushEvent(ev1, []types.PushRequestEventChange{{ToHash: ev1.SHA, RefID: "base"}}), expEvent: ev1, }, + { + name: "bad/changes are empty in push", + eventType: "repo:refs_changed", + payloadEvent: bbv1test.MakePushEvent(ev1, []types.PushRequestEventChange{}), + expEvent: ev1, + wantErrSubstr: "push event contains no commits under 'changes'; cannot proceed", + }, { name: "good/comment ok-to-test", eventType: "pr:comment:added", diff --git a/pkg/provider/bitbucketserver/test/test.go b/pkg/provider/bitbucketserver/test/test.go index dd0f7d5749..a68476a42b 100644 --- a/pkg/provider/bitbucketserver/test/test.go +++ b/pkg/provider/bitbucketserver/test/test.go @@ -330,7 +330,7 @@ func MakePREvent(event *info.Event, comment string) *types.PullRequestEvent { return pr } -func MakePushEvent(event *info.Event) *types.PushRequestEvent { +func MakePushEvent(event *info.Event, changes []types.PushRequestEventChange) *types.PushRequestEvent { iii, _ := strconv.Atoi(event.AccountID) return &types.PushRequestEvent{ @@ -357,11 +357,6 @@ func MakePushEvent(event *info.Event) *types.PushRequestEvent { }, }, }, - Changes: []types.PushRequestEventChange{ - { - ToHash: event.SHA, - RefID: "base", - }, - }, + Changes: changes, } } diff --git a/pkg/provider/github/detect.go b/pkg/provider/github/detect.go index f757079cd1..84f6eae696 100644 --- a/pkg/provider/github/detect.go +++ b/pkg/provider/github/detect.go @@ -102,12 +102,13 @@ func (v *Provider) detectTriggerTypeFromPayload(ghEventType string, eventInt any if provider.IsTestRetestComment(event.GetComment().GetBody()) { return triggertype.Retest, "" } - if provider.IsOkToTestComment(event.GetComment().GetBody()) { - return triggertype.OkToTest, "" - } if provider.IsCancelComment(event.GetComment().GetBody()) { return triggertype.Cancel, "" } + // Here, the `/ok-to-test` command is ignored because it is intended for pull requests. + // For unauthorized users, it has no relevance to pushed commits, as only authorized users + // are allowed to run CI on pushed commits. Therefore, the `ok-to-test` command holds no significance in this context. + // However, it is left to be processed by the `on-comment` annotation rather than returning an error. } return triggertype.Comment, "" } diff --git a/pkg/provider/github/detect_test.go b/pkg/provider/github/detect_test.go index 4646880c21..5b0bda364f 100644 --- a/pkg/provider/github/detect_test.go +++ b/pkg/provider/github/detect_test.go @@ -295,6 +295,19 @@ func TestProvider_Detect(t *testing.T) { isGH: true, processReq: true, }, + { + name: "commit comment Event with /ok-to-test being ignore as GitOps command on pushed commits", + event: github.CommitCommentEvent{ + Action: github.Ptr("created"), + Installation: &github.Installation{ + ID: &idd, + }, + Comment: &github.RepositoryComment{Body: github.Ptr("/ok-to-test")}, + }, + eventType: "commit_comment", + isGH: true, + processReq: true, + }, } for _, tt := range tests { diff --git a/pkg/provider/gitlab/acl_test.go b/pkg/provider/gitlab/acl_test.go index 9c94c61b17..8f0c5fbcae 100644 --- a/pkg/provider/gitlab/acl_test.go +++ b/pkg/provider/gitlab/acl_test.go @@ -108,7 +108,7 @@ func TestIsAllowed(t *testing.T) { thelp.MuxDisallowUserID(mux, tt.fields.targetProjectID, tt.allowMemberID) } if tt.ownerFile != "" { - thelp.MuxGetFile(mux, tt.fields.targetProjectID, "OWNERS", tt.ownerFile) + thelp.MuxGetFile(mux, tt.fields.targetProjectID, "OWNERS", tt.ownerFile, false) } if tt.commentContent != "" { thelp.MuxDiscussionsNote(mux, tt.fields.targetProjectID, diff --git a/pkg/provider/gitlab/gitlab.go b/pkg/provider/gitlab/gitlab.go index ee23855cb4..2c75711378 100644 --- a/pkg/provider/gitlab/gitlab.go +++ b/pkg/provider/gitlab/gitlab.go @@ -295,16 +295,16 @@ func (v *Provider) GetTektonDir(_ context.Context, event *info.Event, path, prov } } - return v.concatAllYamlFiles(nodes, event) + return v.concatAllYamlFiles(nodes, revision) } // concatAllYamlFiles concat all yaml files from a directory as one big multi document yaml string. -func (v *Provider) concatAllYamlFiles(objects []*gitlab.TreeNode, runevent *info.Event) (string, error) { +func (v *Provider) concatAllYamlFiles(objects []*gitlab.TreeNode, revision string) (string, error) { var allTemplates string for _, value := range objects { if strings.HasSuffix(value.Name, ".yaml") || strings.HasSuffix(value.Name, ".yml") { - data, _, err := v.getObject(value.Path, runevent.HeadBranch, v.sourceProjectID) + data, _, err := v.getObject(value.Path, revision, v.sourceProjectID) if err != nil { return "", err } diff --git a/pkg/provider/gitlab/gitlab_test.go b/pkg/provider/gitlab/gitlab_test.go index 924b985d8e..b2d4c16943 100644 --- a/pkg/provider/gitlab/gitlab_test.go +++ b/pkg/provider/gitlab/gitlab_test.go @@ -399,6 +399,8 @@ func TestGetTektonDir(t *testing.T) { args args wantStr string wantErr string + wantTreeAPIErr bool + wantFilesAPIErr bool wantClient bool prcontent string filterMessageSnippet string @@ -472,6 +474,38 @@ func TestGetTektonDir(t *testing.T) { wantClient: true, wantStr: "kind: PipelineRun", }, + { + name: "list tekton dir tree api call error", + prcontent: strings.TrimPrefix(string(samplePR), "---"), + args: args{ + path: ".tekton", + event: &info.Event{ + HeadBranch: "main", + }, + }, + fields: fields{ + sourceProjectID: 100, + }, + wantClient: true, + wantTreeAPIErr: true, + wantErr: "failed to list .tekton dir", + }, + { + name: "get file raw api call error", + prcontent: strings.TrimPrefix(string(samplePR), "---"), + args: args{ + path: ".tekton", + event: &info.Event{ + HeadBranch: "main", + }, + }, + fields: fields{ + sourceProjectID: 100, + }, + wantClient: true, + wantFilesAPIErr: true, + wantErr: "failed to get filename from api pr.yaml dir", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -493,7 +527,7 @@ func TestGetTektonDir(t *testing.T) { muxbranch = tt.args.event.DefaultBranch } if tt.args.path != "" && tt.prcontent != "" { - thelp.MuxListTektonDir(t, mux, tt.fields.sourceProjectID, muxbranch, tt.prcontent) + thelp.MuxListTektonDir(t, mux, tt.fields.sourceProjectID, muxbranch, tt.prcontent, tt.wantTreeAPIErr, tt.wantFilesAPIErr) } defer tearDown() } @@ -528,7 +562,7 @@ func TestGetFileInsideRepo(t *testing.T) { sourceProjectID: 10, Client: client, } - thelp.MuxListTektonDir(t, mux, v.sourceProjectID, event.HeadBranch, content) + thelp.MuxListTektonDir(t, mux, v.sourceProjectID, event.HeadBranch, content, false, false) got, err := v.GetFileInsideRepo(ctx, event, "pr.yaml", "") assert.NilError(t, err) assert.Equal(t, content, got) diff --git a/pkg/provider/gitlab/test/test.go b/pkg/provider/gitlab/test/test.go index b1a33ee28e..396c28d37a 100644 --- a/pkg/provider/gitlab/test/test.go +++ b/pkg/provider/gitlab/test/test.go @@ -66,8 +66,12 @@ func MuxDisallowUserID(mux *http.ServeMux, projectID, userID int) { }) } -func MuxListTektonDir(_ *testing.T, mux *http.ServeMux, pid int, ref, prs string) { +func MuxListTektonDir(_ *testing.T, mux *http.ServeMux, pid int, ref, prs string, wantTreeAPIErr, wantFilesAPIErr bool) { mux.HandleFunc(fmt.Sprintf("/projects/%d/repository/tree", pid), func(rw http.ResponseWriter, r *http.Request) { + if wantTreeAPIErr { + rw.WriteHeader(http.StatusUnauthorized) + return + } if r.URL.Query().Get("pagination") == "keyset" { if r.URL.Query().Get("ref") == ref { if r.URL.Query().Get("page_token") != "page2" { @@ -87,8 +91,8 @@ func MuxListTektonDir(_ *testing.T, mux *http.ServeMux, pid int, ref, prs string } }) - MuxGetFile(mux, pid, "pr.yaml", prs) - MuxGetFile(mux, pid, "random.yaml", `foo:bar`) + MuxGetFile(mux, pid, "pr.yaml", prs, wantFilesAPIErr) + MuxGetFile(mux, pid, "random.yaml", `foo:bar`, wantTreeAPIErr) } func MuxDiscussionsNoteEmpty(mux *http.ServeMux, pid, mrID int) { @@ -120,8 +124,12 @@ func MuxDiscussionsNote(mux *http.ServeMux, pid, mrID int, author string, author }) } -func MuxGetFile(mux *http.ServeMux, pid int, fname, content string) { +func MuxGetFile(mux *http.ServeMux, pid int, fname, content string, wantErr bool) { mux.HandleFunc(fmt.Sprintf("/projects/%d/repository/files/%s/raw", pid, fname), func(rw http.ResponseWriter, _ *http.Request) { + if wantErr { + rw.WriteHeader(http.StatusUnauthorized) + return + } fmt.Fprint(rw, content) }) } diff --git a/test/gitea_test.go b/test/gitea_test.go index d245e6e1bc..19e3b5c9aa 100644 --- a/test/gitea_test.go +++ b/test/gitea_test.go @@ -14,9 +14,7 @@ import ( "code.gitea.io/sdk/gitea" "github.com/google/go-github/v68/github" - "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" "github.com/tektoncd/pipeline/pkg/names" - yaml "gopkg.in/yaml.v2" "gotest.tools/v3/assert" "gotest.tools/v3/env" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -30,7 +28,6 @@ import ( tknpaclist "github.com/openshift-pipelines/pipelines-as-code/pkg/cmd/tknpac/list" tknpacresolve "github.com/openshift-pipelines/pipelines-as-code/pkg/cmd/tknpac/resolve" "github.com/openshift-pipelines/pipelines-as-code/pkg/git" - "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/settings" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype" @@ -869,22 +866,6 @@ func TestGiteaErrorSnippetWithSecret(t *testing.T) { tgitea.WaitForPullRequestCommentMatch(t, topts) } -// TestGiteaNotExistingClusterTask checks that the pipeline run fails if the clustertask does not exist -// This will test properly if we error the reason in UI see bug #1160. -func TestGiteaNotExistingClusterTask(t *testing.T) { - topts := &tgitea.TestOpts{ - Regexp: regexp.MustCompile(`.*clustertasks.tekton.dev "foo-bar" not found`), - TargetEvent: triggertype.PullRequest.String(), - YAMLFiles: map[string]string{ - ".tekton/pr.yaml": "testdata/failures/not-existing-clustertask.yaml", - }, - CheckForStatus: "failure", - ExpectEvents: false, - } - _, f := tgitea.TestPR(t, topts) - defer f() -} - // TestGiteaBadLinkOfTask checks that we fail properly with the error from the // tekton pipelines controller. We check on the UI interface that we display // and inside the pac controller. @@ -1128,65 +1109,6 @@ func TestGiteaPushToTagGreedy(t *testing.T) { assert.NilError(t, err) } -// TestGiteaClusterTasks is a test to verify that we can use cluster tasks with PaaC. -func TestGiteaClusterTasks(t *testing.T) { - // we need to verify sure to create clustertask before pushing the files - // so we have to create a new client and do more manual things we get for free in TestPR - topts := &tgitea.TestOpts{ - TargetEvent: "pull_request, push", - YAMLFiles: map[string]string{ - ".tekton/prcluster.yaml": "testdata/pipelinerunclustertasks.yaml", - }, - ExpectEvents: false, - } - topts.TargetRefName = names.SimpleNameGenerator.RestrictLengthWithRandomSuffix("pac-e2e-test") - topts.TargetNS = topts.TargetRefName - - // create first the cluster tasks - ctname := fmt.Sprintf(".tekton/%s.yaml", topts.TargetNS) - newyamlFiles := map[string]string{ctname: "testdata/clustertask.yaml"} - entries, err := payload.GetEntries(newyamlFiles, topts.TargetNS, "main", "pull_request", map[string]string{}) - assert.NilError(t, err) - //nolint: staticcheck - ct := v1beta1.ClusterTask{} - assert.NilError(t, yaml.Unmarshal([]byte(entries[ctname]), &ct)) - ct.Name = "clustertask-" + topts.TargetNS - - run := params.New() - ctx := context.Background() - assert.NilError(t, run.Clients.NewClients(ctx, &run.Info)) - // TODO(chmou): this is for v1beta1, we need to figure out a way how to do that on v1 - _, err = run.Clients.Tekton.TektonV1beta1().ClusterTasks().Create(context.TODO(), &ct, metav1.CreateOptions{}) - assert.NilError(t, err) - assert.NilError(t, pacrepo.CreateNS(ctx, topts.TargetNS, run)) - run.Clients.Log.Infof("%s has been created", ct.GetName()) - defer (func() { - assert.NilError(t, topts.ParamsRun.Clients.Tekton.TektonV1beta1().ClusterTasks().Delete(context.TODO(), ct.Name, metav1.DeleteOptions{})) - run.Clients.Log.Infof("%s is deleted", ct.GetName()) - })() - - // start PR - _, f := tgitea.TestPR(t, topts) - defer f() - - // wait for it - waitOpts := twait.Opts{ - RepoName: topts.TargetNS, - Namespace: topts.TargetNS, - // 0 means 1 🙃 (we test for >, while we actually should do >=, but i - // need to go all over the code to verify it's not going to break - // anything else) - MinNumberStatus: 0, - PollTimeout: twait.DefaultTimeout, - TargetSHA: topts.PullRequest.Head.Sha, - } - _, err = twait.UntilRepositoryUpdated(context.Background(), topts.ParamsRun.Clients, waitOpts) - assert.NilError(t, err) - - topts.CheckForStatus = "success" - tgitea.WaitForStatus(t, topts, topts.TargetRefName, "", true) -} - // Local Variables: // compile-command: "go test -tags=e2e -v -run TestGiteaPush ." // End: diff --git a/test/testdata/clustertask.yaml b/test/testdata/clustertask.yaml deleted file mode 100644 index 09e543cfa5..0000000000 --- a/test/testdata/clustertask.yaml +++ /dev/null @@ -1,11 +0,0 @@ ---- -apiVersion: tekton.dev/v1beta1 -kind: ClusterTask -metadata: - name: "clustertask-\\ .PipelineName //" -spec: - steps: - - name: echo - image: registry.access.redhat.com/ubi9/ubi-micro - script: | - echo "hello from clustertask" diff --git a/test/testdata/failures/not-existing-clustertask.yaml b/test/testdata/failures/not-existing-clustertask.yaml deleted file mode 100644 index e3f5d2addc..0000000000 --- a/test/testdata/failures/not-existing-clustertask.yaml +++ /dev/null @@ -1,16 +0,0 @@ ---- -apiVersion: tekton.dev/v1beta1 -kind: PipelineRun -metadata: - name: "\\.PipelineName//" - annotations: - pipelinesascode.tekton.dev/target-namespace: "\\ .TargetNamespace //" - pipelinesascode.tekton.dev/on-target-branch: "[\\ .TargetBranch //]" - pipelinesascode.tekton.dev/on-event: "[\\ .TargetEvent //]" -spec: - pipelineSpec: - tasks: - - name: task - taskRef: - kind: ClusterTask - name: foo-bar diff --git a/test/testdata/pipelinerunclustertasks.yaml b/test/testdata/pipelinerunclustertasks.yaml deleted file mode 100644 index 1835bc6242..0000000000 --- a/test/testdata/pipelinerunclustertasks.yaml +++ /dev/null @@ -1,16 +0,0 @@ ---- -apiVersion: tekton.dev/v1beta1 -kind: PipelineRun -metadata: - name: "\\ .PipelineName //" - annotations: - pipelinesascode.tekton.dev/target-namespace: "\\ .TargetNamespace //" - pipelinesascode.tekton.dev/on-target-branch: "[\\ .TargetBranch //]" - pipelinesascode.tekton.dev/on-event: "[\\ .TargetEvent //]" -spec: - pipelineSpec: - tasks: - - name: task - taskRef: - kind: ClusterTask - name: "clustertask-\\ .TargetNamespace //"