From d5d649a875dda5ee7f9b0a945e1646088dcd3123 Mon Sep 17 00:00:00 2001 From: Zaki Shaikh Date: Tue, 18 Feb 2025 22:20:20 +0530 Subject: [PATCH 1/4] Fix: PLR Definition Issue When Provenance is Set in GitLab Issue: In GitLab, when provenance is set to default_branch, the API call to get .tekton directory tree is correct and fetching the tree as per provenance but when each is file is fetched as raw its using `source` as provenance no matter what value has been set in Repository CR. Solution: This pull request use revision correctly in concatAllYamlFiles func to make it obey provenance settings. Signed-off-by: Zaki Shaikh --- pkg/provider/gitlab/acl_test.go | 2 +- pkg/provider/gitlab/gitlab.go | 6 ++--- pkg/provider/gitlab/gitlab_test.go | 38 ++++++++++++++++++++++++++++-- pkg/provider/gitlab/test/test.go | 16 +++++++++---- 4 files changed, 52 insertions(+), 10 deletions(-) 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) }) } From 27a3f81261997524cd95384b8f1791e94d835fdc Mon Sep 17 00:00:00 2001 From: Zaki Shaikh Date: Mon, 10 Feb 2025 12:46:26 +0530 Subject: [PATCH 2/4] Invalidate ok-to-test command on pushed commits invalidated ok-to-test command on pushed commits as it is not needed there. Signed-off-by: Zaki Shaikh --- docs/content/docs/guide/gitops_commands.md | 3 +++ pkg/provider/github/detect.go | 7 ++++--- pkg/provider/github/detect_test.go | 13 +++++++++++++ 3 files changed, 20 insertions(+), 3 deletions(-) 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/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 { From 1d4282641c5f9fb695010f42c466ad90134d2fbf Mon Sep 17 00:00:00 2001 From: Zaki Shaikh Date: Mon, 28 Apr 2025 18:47:34 +0530 Subject: [PATCH 3/4] fix: PAC controller crash on empty changes in BDC fixed crash on push when there are no changes in in event payload in Bitbucket Data Center. https://issues.redhat.com/browse/SRVKP-7522 Signed-off-by: Zaki Shaikh --- pkg/provider/bitbucketserver/parse_payload.go | 5 +++++ pkg/provider/bitbucketserver/parse_payload_test.go | 9 ++++++++- pkg/provider/bitbucketserver/test/test.go | 9 ++------- 3 files changed, 15 insertions(+), 8 deletions(-) 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, } } From 967f155438ce14ce699cfdda6b7940b32807c1ec Mon Sep 17 00:00:00 2001 From: Zaki Shaikh Date: Wed, 30 Apr 2025 21:37:15 +0530 Subject: [PATCH 4/4] fix: Delete Gitea Cluster Task from E2E deleted gitea cluster task e2e tests as clustertask is deprecated in OSP and its CRDs will be removed in upcoming release. Signed-off-by: Zaki Shaikh --- test/gitea_test.go | 78 ------------------- test/testdata/clustertask.yaml | 11 --- .../failures/not-existing-clustertask.yaml | 16 ---- test/testdata/pipelinerunclustertasks.yaml | 16 ---- 4 files changed, 121 deletions(-) delete mode 100644 test/testdata/clustertask.yaml delete mode 100644 test/testdata/failures/not-existing-clustertask.yaml delete mode 100644 test/testdata/pipelinerunclustertasks.yaml 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 //"