diff --git a/.github/workflows/e2e.yaml b/.github/workflows/e2e.yaml index e9f28f7f84..5b323ac824 100644 --- a/.github/workflows/e2e.yaml +++ b/.github/workflows/e2e.yaml @@ -166,8 +166,9 @@ jobs: ./hack/gh-workflow-ci.sh collect_logs - name: Show controllers/watcher logs with Snazy + if: ${{ always() }} run: | - ./hack/gh-workflow-ci output-logs + ./hack/gh-workflow-ci.sh output_logs - name: Upload artifacts if: ${{ always() }} diff --git a/docs/content/docs/dev/_index.md b/docs/content/docs/dev/_index.md index 4042d62b06..755930bea5 100644 --- a/docs/content/docs/dev/_index.md +++ b/docs/content/docs/dev/_index.md @@ -189,12 +189,14 @@ For example, to test and lint the go files: make test lint-go ``` -If you add a CLI command with help, you will need to regenerate the golden files: +We use [golden](https://pkg.go.dev/gotest.tools/v3/golden) files in our tests, for instance, to compare the output of CLI commands or other detailed tests. Occasionally, you may need to regenerate the golden files if you modify the output of a command. For unit tests, you can use this Makefile target: ```shell make update-golden ``` +Head over to the [./test/README.md](./test/README.md) for more information on how to update the golden files on the E2E tests. + ## Configuring the Pre Push Git checks We are using several tools to verify that pipelines-as-code is up to a good diff --git a/docs/content/docs/guide/running.md b/docs/content/docs/guide/running.md index e02bf576d0..2a08b27aed 100644 --- a/docs/content/docs/guide/running.md +++ b/docs/content/docs/guide/running.md @@ -79,7 +79,7 @@ If the `OWNERS` file uses `filters` instead of a simple configuration, we only consider the `.*` filter and extract the `approvers` and `reviewers` lists from it. Any other filters targeting specific files or directories are ignored. -Additionally, `OWNERS_ALIASES` is supported and allows mapping alias names to +Additionally, `OWNERS_ALIASES` is supported and allows mapping alias names to a lists of usernames. Including contributors in the `approvers` or `reviewers` lists within your @@ -122,6 +122,29 @@ or on OpenShift using the OpenShift Console. Pipelines-as-Code will post a URL in the Checks tab for GitHub apps to let you click on it and follow the pipeline execution directly there. +## Errors When Parsing PipelineRun YAML + +When Pipelines-As-Code encounters an issue with the YAML formatting in the +repository, it will log the error in the user namespace events log and +the Pipelines-as-Code controller log. + +Despite the error, Pipelines-As-Code will continue to run other correctly parsed +and matched PipelineRuns. + +{{< support_matrix github_app="true" github_webhook="true" gitea="true" gitlab="true" bitbucket_cloud="false" bitbucket_server="false" >}} + +When an event is triggered from a Pull Request, a new comment will be created on +the Pull Request detailing the error. + +Subsequent iterations on the Pull Request will update the comment with any new +errors. + +If no new errors are detected, the comment will remain and will not be deleted. + +Here is an example of a YAML error being reported as a comment to a Pull Request: + +![report yaml error as comments](/images/report-error-comment-on-bad-yaml.png) + ## Cancelling ### Cancelling in-progress PipelineRuns diff --git a/docs/static/images/report-error-comment-on-bad-yaml.png b/docs/static/images/report-error-comment-on-bad-yaml.png new file mode 100644 index 0000000000..f8803ffea8 Binary files /dev/null and b/docs/static/images/report-error-comment-on-bad-yaml.png differ diff --git a/pkg/pipelineascode/match.go b/pkg/pipelineascode/match.go index e8c538e369..a4926b0546 100644 --- a/pkg/pipelineascode/match.go +++ b/pkg/pipelineascode/match.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "regexp" "strings" apipac "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys" @@ -20,6 +21,12 @@ import ( "go.uber.org/zap" ) +const validationErrorTemplate = `> [!CAUTION] +> There are some errors in your PipelineRun template. + +| PipelineRun | Error | +|------|-------|` + func (p *PacRun) matchRepoPR(ctx context.Context) ([]matcher.Match, *v1alpha1.Repository, error) { repo, err := p.verifyRepoAndUser(ctx) if err != nil { @@ -172,12 +179,18 @@ func (p *PacRun) getPipelineRunsFromRepo(ctx context.Context, repo *v1alpha1.Rep provenance = repo.Spec.Settings.PipelineRunProvenance } rawTemplates, err := p.vcx.GetTektonDir(ctx, p.event, tektonDir, provenance) - if err != nil && strings.Contains(err.Error(), "error unmarshalling yaml file") { + if err != nil && p.event.TriggerTarget == triggertype.PullRequest && strings.Contains(err.Error(), "error unmarshalling yaml file") { // make the error a bit more friendly for users who don't know what marshalling or intricacies of the yaml parser works - errmsg := err.Error() - errmsg = strings.ReplaceAll(errmsg, " error converting YAML to JSON: yaml:", "") - errmsg = strings.ReplaceAll(errmsg, "unmarshalling", "while parsing the") - return nil, fmt.Errorf("%s", errmsg) + // format is "error unmarshalling yaml file pr-bad-format.yaml: yaml: line 3: could not find expected ':'" + // get the filename with a regexp + reg := regexp.MustCompile(`error unmarshalling yaml file\s([^:]*):\s*(yaml:\s*)?(.*)`) + matches := reg.FindStringSubmatch(err.Error()) + if len(matches) == 4 { + p.reportValidationErrors(ctx, repo, map[string]string{matches[1]: matches[3]}) + return nil, nil + } + + return nil, err } if err != nil || rawTemplates == "" { msg := fmt.Sprintf("cannot locate templates in %s/ directory for this repository in %s", tektonDir, p.event.HeadBranch) @@ -227,15 +240,12 @@ func (p *PacRun) getPipelineRunsFromRepo(ctx context.Context, repo *v1alpha1.Rep return nil, err } - if types.ValidationErrors != nil { - for k, v := range types.ValidationErrors { - kv := fmt.Sprintf("prun: %s tekton validation error: %s", k, v) - p.eventEmitter.EmitMessage(repo, zap.ErrorLevel, "PipelineRunValidationErrors", kv) - } + if len(types.ValidationErrors) > 0 && p.event.TriggerTarget == triggertype.PullRequest { + p.reportValidationErrors(ctx, repo, types.ValidationErrors) } pipelineRuns := types.PipelineRuns if len(pipelineRuns) == 0 { - msg := fmt.Sprintf("cannot locate templates in %s/ directory for this repository in %s", tektonDir, p.event.HeadBranch) + msg := fmt.Sprintf("cannot locate valid templates in %s/ directory for this repository in %s", tektonDir, p.event.HeadBranch) p.eventEmitter.EmitMessage(nil, zap.InfoLevel, "RepositoryCannotLocatePipelineRun", msg) return nil, nil } @@ -437,3 +447,22 @@ func (p *PacRun) createNeutralStatus(ctx context.Context) error { return nil } + +// reportValidationErrors reports validation errors found in PipelineRuns by: +// 1. Creating error messages for each validation error +// 2. Emitting error messages to the event system +// 3. Creating a markdown formatted comment on the repository with all errors. +func (p *PacRun) reportValidationErrors(ctx context.Context, repo *v1alpha1.Repository, validationErrors map[string]string) { + errorRows := make([]string, 0, len(validationErrors)) + for name, err := range validationErrors { + errorRows = append(errorRows, fmt.Sprintf("| %s | `%s` |", name, err)) + p.eventEmitter.EmitMessage(repo, zap.ErrorLevel, "PipelineRunValidationErrors", + fmt.Sprintf("cannot read the PipelineRun: %s, error: %s", name, err)) + } + markdownErrMessage := fmt.Sprintf(`%s +%s`, validationErrorTemplate, strings.Join(errorRows, "\n")) + if err := p.vcx.CreateComment(ctx, p.event, markdownErrMessage, validationErrorTemplate); err != nil { + p.eventEmitter.EmitMessage(repo, zap.ErrorLevel, "PipelineRunCommentCreationError", + fmt.Sprintf("failed to create comment: %s", err.Error())) + } +} diff --git a/pkg/pipelineascode/match_test.go b/pkg/pipelineascode/match_test.go index 7f04c12c58..6b4b403b2e 100644 --- a/pkg/pipelineascode/match_test.go +++ b/pkg/pipelineascode/match_test.go @@ -179,7 +179,7 @@ func TestGetPipelineRunsFromRepo(t *testing.T) { }, tektondir: "testdata/invalid_tekton_yaml", event: pullRequestEvent, - logSnippet: `prun: bad-tekton-yaml tekton validation error: json: cannot unmarshal object into Go struct field PipelineSpec.spec.pipelineSpec.tasks of type []v1beta1.PipelineTask`, + logSnippet: `json: cannot unmarshal object into Go struct field PipelineSpec.spec.pipelineSpec.tasks of type []v1beta1.PipelineTask`, }, { name: "no-match pipelineruns in .tekton dir, only matched should be returned", diff --git a/pkg/pipelineascode/pipelineascode_test.go b/pkg/pipelineascode/pipelineascode_test.go index fce9f9fef0..80211eaff0 100644 --- a/pkg/pipelineascode/pipelineascode_test.go +++ b/pkg/pipelineascode/pipelineascode_test.go @@ -9,6 +9,7 @@ import ( "io" "net/http" "path/filepath" + "regexp" "strings" "sync" "testing" @@ -63,11 +64,6 @@ func testSetupCommonGhReplies(t *testing.T, mux *http.ServeMux, runevent info.Ev fmt.Sprintf("/repos/%s/%s/statuses/%s", runevent.Organization, runevent.Repository, runevent.SHA), "{}") - // using 666 as pull request number - replyString(mux, - fmt.Sprintf("/repos/%s/%s/issues/666/comments", runevent.Organization, runevent.Repository), - "{}") - jj := fmt.Sprintf(`{"sha": "%s", "html_url": "https://git.commit.url/%s", "message": "commit message"}`, runevent.SHA, runevent.SHA) replyString(mux, @@ -131,6 +127,7 @@ func TestRun(t *testing.T) { PayloadEncodedSecret string concurrencyLimit int expectedLogSnippet string + expectedPostedComment string // TODO: multiple posted comments when we need it }{ { name: "pull request/fail-to-start-apps", @@ -149,6 +146,23 @@ func TestRun(t *testing.T) { finalStatus: "failure", finalStatusText: "we need at least one pipelinerun to start with", }, + { + name: "pull request/bad-yaml", + runevent: info.Event{ + SHA: "principale", + Organization: "owner", + Repository: "lagaffe", + URL: "https://service/documentation", + HeadBranch: "press", + BaseBranch: "main", + Sender: "owner", + EventType: "pull_request", + TriggerTarget: "pull_request", + PullRequestNumber: 666, + }, + tektondir: "testdata/bad_yaml", + expectedPostedComment: ".*There are some errors in your PipelineRun template.*line 2: did not find expected key", + }, { name: "pull request/unknown-remotetask-but-fail-on-matching", runevent: info.Event{ @@ -548,6 +562,19 @@ func TestRun(t *testing.T) { ghtesthelper.SetupGitTree(t, mux, tt.tektondir, &tt.runevent, false) } + mux.HandleFunc(fmt.Sprintf("/repos/%s/%s/issues/%d/comments", tt.runevent.Organization, tt.runevent.Repository, tt.runevent.PullRequestNumber), + func(w http.ResponseWriter, req *http.Request) { + if req.Method == http.MethodPost { + _, _ = fmt.Fprintf(w, `{"id": %d}`, tt.runevent.PullRequestNumber) + // read body and compare it + body, _ := io.ReadAll(req.Body) + expectedRegexp := regexp.MustCompile(tt.expectedPostedComment) + assert.Assert(t, expectedRegexp.Match(body), "expected comment %s, got %s", tt.expectedPostedComment, string(body)) + return + } + _, _ = fmt.Fprint(w, `[]`) + }) + stdata, _ := testclient.SeedTestData(t, ctx, tdata) cs := ¶ms.Run{ Clients: clients.Clients{ diff --git a/pkg/provider/bitbucketcloud/bitbucket.go b/pkg/provider/bitbucketcloud/bitbucket.go index 7e3d135a22..75b7927ea3 100644 --- a/pkg/provider/bitbucketcloud/bitbucket.go +++ b/pkg/provider/bitbucketcloud/bitbucket.go @@ -45,6 +45,10 @@ func (v *Provider) Client() *bitbucket.Client { return v.bbClient } +func (v *Provider) CreateComment(_ context.Context, _ *info.Event, _, _ string) error { + return nil +} + // CheckPolicyAllowing TODO: Implement ME. func (v *Provider) CheckPolicyAllowing(_ context.Context, _ *info.Event, _ []string) (bool, string) { return false, "" diff --git a/pkg/provider/bitbucketdatacenter/bitbucketdatacenter.go b/pkg/provider/bitbucketdatacenter/bitbucketdatacenter.go index 0fb94bf955..b801a5676d 100644 --- a/pkg/provider/bitbucketdatacenter/bitbucketdatacenter.go +++ b/pkg/provider/bitbucketdatacenter/bitbucketdatacenter.go @@ -74,6 +74,10 @@ func (v *Provider) SetScmClient(client *scm.Client) { v.scmClient = client } +func (v *Provider) CreateComment(_ context.Context, _ *info.Event, _, _ string) error { + return nil +} + func (v *Provider) SetPacInfo(pacInfo *info.PacOpts) { v.pacInfo = pacInfo } diff --git a/pkg/provider/gitea/gitea.go b/pkg/provider/gitea/gitea.go index f66ed73244..db0e547aeb 100644 --- a/pkg/provider/gitea/gitea.go +++ b/pkg/provider/gitea/gitea.go @@ -7,6 +7,7 @@ import ( "fmt" "net/http" "path" + "regexp" "strconv" "strings" @@ -73,6 +74,40 @@ func (v *Provider) SetGiteaClient(client *gitea.Client) { v.giteaClient = client } +func (v *Provider) CreateComment(_ context.Context, event *info.Event, commit, updateMarker string) error { + if v.giteaClient == nil { + return fmt.Errorf("no gitea client has been initialized") + } + + if event.PullRequestNumber == 0 { + return fmt.Errorf("create comment only works on pull requests") + } + + // List comments of the PR + if updateMarker != "" { + comments, _, err := v.Client().ListIssueComments(event.Organization, event.Repository, int64(event.PullRequestNumber), gitea.ListIssueCommentOptions{}) + if err != nil { + return err + } + + re := regexp.MustCompile(updateMarker) + for _, comment := range comments { + if re.MatchString(comment.Body) { + _, _, err := v.Client().EditIssueComment(event.Organization, event.Repository, comment.ID, gitea.EditIssueCommentOption{ + Body: commit, + }) + return err + } + } + } + + _, _, err := v.Client().CreateIssueComment(event.Organization, event.Repository, int64(event.PullRequestNumber), gitea.CreateIssueCommentOption{ + Body: commit, + }) + + return err +} + func (v *Provider) SetPacInfo(pacInfo *info.PacOpts) { v.pacInfo = pacInfo } diff --git a/pkg/provider/gitea/gitea_test.go b/pkg/provider/gitea/gitea_test.go index d48f772ee4..281755781d 100644 --- a/pkg/provider/gitea/gitea_test.go +++ b/pkg/provider/gitea/gitea_test.go @@ -525,3 +525,101 @@ func TestGetTektonDir(t *testing.T) { }) } } + +func TestCreateComment(t *testing.T) { + tests := []struct { + name string + event *info.Event + commit string + updateMarker string + mockResponses map[string]func(rw http.ResponseWriter, _ *http.Request) + wantErr string + clientNil bool + }{ + { + name: "nil client error", + clientNil: true, + event: &info.Event{PullRequestNumber: 123}, + wantErr: "no gitea client has been initialized", + }, + { + name: "not a pull request error", + event: &info.Event{PullRequestNumber: 0}, + wantErr: "create comment only works on pull requests", + }, + { + name: "create new comment", + event: &info.Event{Organization: "org", Repository: "repo", PullRequestNumber: 123}, + commit: "New Comment", + updateMarker: "", + mockResponses: map[string]func(rw http.ResponseWriter, _ *http.Request){ + "/repos/org/repo/issues/123/comments": func(rw http.ResponseWriter, r *http.Request) { + assert.Equal(t, r.Method, http.MethodPost) + fmt.Fprint(rw, `{}`) + }, + }, + }, + { + name: "update existing comment", + event: &info.Event{Organization: "org", Repository: "repo", PullRequestNumber: 123}, + commit: "Updated Comment", + updateMarker: "MARKER", + mockResponses: map[string]func(rw http.ResponseWriter, _ *http.Request){ + "/repos/org/repo/issues/123/comments": func(rw http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodGet { + fmt.Fprint(rw, `[{"id": 555, "body": "MARKER"}]`) + return + } + }, + "/repos/org/repo/issues/comments/555": func(rw http.ResponseWriter, r *http.Request) { + assert.Equal(t, r.Method, "PATCH") + rw.WriteHeader(http.StatusOK) + fmt.Fprint(rw, `{}`) + }, + }, + }, + { + name: "no matching comment creates new", + event: &info.Event{Organization: "org", Repository: "repo", PullRequestNumber: 123}, + commit: "New Comment", + updateMarker: "MARKER", + mockResponses: map[string]func(rw http.ResponseWriter, _ *http.Request){ + "/repos/org/repo/issues/123/comments": func(rw http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodGet { + fmt.Fprint(rw, `[{"id": 555, "body": "NO_MATCH"}]`) + return + } + assert.Equal(t, r.Method, http.MethodPost) + rw.WriteHeader(http.StatusCreated) + fmt.Fprint(rw, `{}`) + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fakeclient, mux, teardown := tgitea.Setup(t) + defer teardown() + + if tt.clientNil { + p := &Provider{} + err := p.CreateComment(context.Background(), tt.event, tt.commit, tt.updateMarker) + assert.ErrorContains(t, err, tt.wantErr) + return + } + + for endpoint, handler := range tt.mockResponses { + mux.HandleFunc(endpoint, handler) + } + + p := &Provider{giteaClient: fakeclient} + err := p.CreateComment(context.Background(), tt.event, tt.commit, tt.updateMarker) + if tt.wantErr != "" { + assert.ErrorContains(t, err, tt.wantErr) + } else { + assert.NilError(t, err) + } + }) + } +} diff --git a/pkg/provider/github/github.go b/pkg/provider/github/github.go index 1b4b339fb0..d61fbf1aeb 100644 --- a/pkg/provider/github/github.go +++ b/pkg/provider/github/github.go @@ -6,6 +6,7 @@ import ( "fmt" "net/http" "net/url" + "regexp" "strings" "sync" "time" @@ -624,3 +625,42 @@ func (v *Provider) isHeadCommitOfBranch(ctx context.Context, runevent *info.Even func (v *Provider) GetTemplate(commentType provider.CommentType) string { return provider.GetHTMLTemplate(commentType) } + +// CreateComment creates a comment on a Pull Request. +func (v *Provider) CreateComment(ctx context.Context, event *info.Event, commit, updateMarker string) error { + if v.ghClient == nil { + return fmt.Errorf("no github client has been initialized") + } + + if event.PullRequestNumber == 0 { + return fmt.Errorf("create comment only works on pull requests") + } + + // List last page of the comments of the PR + if updateMarker != "" { + comments, _, err := v.Client().Issues.ListComments(ctx, event.Organization, event.Repository, event.PullRequestNumber, &github.IssueListCommentsOptions{ + ListOptions: github.ListOptions{ + Page: 1, + PerPage: 100, + }, + }) + if err != nil { + return err + } + + re := regexp.MustCompile(regexp.QuoteMeta(updateMarker)) + for _, comment := range comments { + if re.MatchString(comment.GetBody()) { + _, _, err := v.Client().Issues.EditComment(ctx, event.Organization, event.Repository, comment.GetID(), &github.IssueComment{ + Body: &commit, + }) + return err + } + } + } + + _, _, err := v.Client().Issues.CreateComment(ctx, event.Organization, event.Repository, event.PullRequestNumber, &github.IssueComment{ + Body: &commit, + }) + return err +} diff --git a/pkg/provider/github/github_test.go b/pkg/provider/github/github_test.go index 1fc3d0a431..31695323f1 100644 --- a/pkg/provider/github/github_test.go +++ b/pkg/provider/github/github_test.go @@ -1211,3 +1211,96 @@ func resetMetrics() { // have to reset sync.Once to allow recreation of Recorder. metrics.ResetRecorder() } + +func TestCreateComment(t *testing.T) { + tests := []struct { + name string + event *info.Event + updateMarker string + mockResponses map[string]func(rw http.ResponseWriter, _ *http.Request) + wantErr string + clientNil bool + }{ + { + name: "nil client error", + clientNil: true, + event: &info.Event{PullRequestNumber: 123}, + wantErr: "no github client has been initialized", + }, + { + name: "not a pull request error", + event: &info.Event{PullRequestNumber: 0}, + wantErr: "create comment only works on pull requests", + }, + { + name: "create new comment", + event: &info.Event{Organization: "org", Repository: "repo", PullRequestNumber: 123}, + updateMarker: "", + mockResponses: map[string]func(rw http.ResponseWriter, _ *http.Request){ + "/repos/org/repo/issues/123/comments": func(rw http.ResponseWriter, r *http.Request) { + assert.Equal(t, r.Method, http.MethodPost) + rw.WriteHeader(http.StatusCreated) + }, + }, + }, + { + name: "update existing comment", + event: &info.Event{Organization: "org", Repository: "repo", PullRequestNumber: 123}, + updateMarker: "MARKER", + mockResponses: map[string]func(rw http.ResponseWriter, _ *http.Request){ + "/repos/org/repo/issues/123/comments": func(rw http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodGet { + fmt.Fprint(rw, `[{"id": 555, "body": "MARKER"}]`) + return + } + }, + "/repos/org/repo/issues/comments/555": func(rw http.ResponseWriter, r *http.Request) { + assert.Equal(t, r.Method, http.MethodPatch) + rw.WriteHeader(http.StatusOK) + }, + }, + }, + { + name: "no matching comment creates new", + event: &info.Event{Organization: "org", Repository: "repo", PullRequestNumber: 123}, + updateMarker: "MARKER", + mockResponses: map[string]func(rw http.ResponseWriter, _ *http.Request){ + "/repos/org/repo/issues/123/comments": func(rw http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodGet { + fmt.Fprint(rw, `[{"id": 555, "body": "NO_MATCH"}]`) + return + } + assert.Equal(t, r.Method, http.MethodPost) + rw.WriteHeader(http.StatusCreated) + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx, _ := rtesting.SetupFakeContext(t) + + var provider *Provider + if !tt.clientNil { + fakeclient, mux, _, teardown := ghtesthelper.SetupGH() + defer teardown() + provider = &Provider{ghClient: fakeclient} + + for pattern, handler := range tt.mockResponses { + mux.HandleFunc(pattern, handler) + } + } else { + provider = &Provider{} // nil client + } + + err := provider.CreateComment(ctx, tt.event, "comment body", tt.updateMarker) + + if tt.wantErr != "" { + assert.ErrorContains(t, err, tt.wantErr) + return + } + assert.NilError(t, err) + }) + } +} diff --git a/pkg/provider/gitlab/gitlab.go b/pkg/provider/gitlab/gitlab.go index 3e2b30ffcc..822e4097f6 100644 --- a/pkg/provider/gitlab/gitlab.go +++ b/pkg/provider/gitlab/gitlab.go @@ -7,6 +7,7 @@ import ( "net/http" "net/url" "path/filepath" + "regexp" "strings" "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1" @@ -82,6 +83,45 @@ func (v *Provider) SetPacInfo(pacInfo *info.PacOpts) { v.pacInfo = pacInfo } +func (v *Provider) CreateComment(_ context.Context, event *info.Event, commit, updateMarker string) error { + if v.gitlabClient == nil { + return fmt.Errorf("no gitlab client has been initialized") + } + + if event.PullRequestNumber == 0 { + return fmt.Errorf("create comment only works on merge requests") + } + + // List comments of the merge request + if updateMarker != "" { + comments, _, err := v.Client().Notes.ListMergeRequestNotes(v.sourceProjectID, event.PullRequestNumber, &gitlab.ListMergeRequestNotesOptions{ + ListOptions: gitlab.ListOptions{ + Page: 1, + PerPage: 100, + }, + }) + if err != nil { + return err + } + + re := regexp.MustCompile(updateMarker) + for _, comment := range comments { + if re.MatchString(comment.Body) { + _, _, err := v.Client().Notes.UpdateMergeRequestNote(v.sourceProjectID, event.PullRequestNumber, comment.ID, &gitlab.UpdateMergeRequestNoteOptions{ + Body: &commit, + }) + return err + } + } + } + + _, _, err := v.Client().Notes.CreateMergeRequestNote(v.sourceProjectID, event.PullRequestNumber, &gitlab.CreateMergeRequestNoteOptions{ + Body: &commit, + }) + + return err +} + // CheckPolicyAllowing TODO: Implement ME. func (v *Provider) CheckPolicyAllowing(_ context.Context, _ *info.Event, _ []string) (bool, string) { return false, "" diff --git a/pkg/provider/gitlab/gitlab_test.go b/pkg/provider/gitlab/gitlab_test.go index 475dc74b64..8a3509ad80 100644 --- a/pkg/provider/gitlab/gitlab_test.go +++ b/pkg/provider/gitlab/gitlab_test.go @@ -916,3 +916,107 @@ func TestIsHeadCommitOfBranch(t *testing.T) { }) } } + +func TestGitLabCreateComment(t *testing.T) { + tests := []struct { + name string + event *info.Event + commit string + updateMarker string + mockResponses map[string]func(rw http.ResponseWriter, _ *http.Request) + wantErr string + clientNil bool + }{ + { + name: "nil client error", + clientNil: true, + event: &info.Event{PullRequestNumber: 123}, + wantErr: "no gitlab client has been initialized", + }, + { + name: "not a merge request error", + event: &info.Event{PullRequestNumber: 0}, + wantErr: "create comment only works on merge requests", + }, + { + name: "create new comment", + event: &info.Event{PullRequestNumber: 123}, + commit: "New Comment", + updateMarker: "", + mockResponses: map[string]func(rw http.ResponseWriter, _ *http.Request){ + "/projects/666/merge_requests/123/notes": func(rw http.ResponseWriter, r *http.Request) { + assert.Equal(t, r.Method, http.MethodPost) + rw.WriteHeader(http.StatusCreated) + fmt.Fprint(rw, `{}`) + }, + }, + }, + { + name: "update existing comment", + event: &info.Event{PullRequestNumber: 123}, + commit: "Updated Comment", + updateMarker: "MARKER", + mockResponses: map[string]func(rw http.ResponseWriter, _ *http.Request){ + "/projects/666/merge_requests/123/notes": func(rw http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodGet { + fmt.Fprint(rw, `[{"id": 555, "body": "MARKER"}]`) + return + } + }, + "/projects/666/merge_requests/123/notes/555": func(rw http.ResponseWriter, r *http.Request) { + assert.Equal(t, r.Method, "PUT") + rw.WriteHeader(http.StatusOK) + fmt.Fprint(rw, `{}`) + }, + }, + }, + { + name: "no matching comment creates new", + event: &info.Event{PullRequestNumber: 123}, + commit: "New Comment", + updateMarker: "MARKER", + mockResponses: map[string]func(rw http.ResponseWriter, _ *http.Request){ + "/projects/666/merge_requests/123/notes": func(rw http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodGet { + fmt.Fprint(rw, `[{"id": 555, "body": "NO_MATCH"}]`) + return + } + assert.Equal(t, r.Method, http.MethodPost) + rw.WriteHeader(http.StatusCreated) + fmt.Fprint(rw, `{}`) + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fakeclient, mux, teardown := thelp.Setup(t) + defer teardown() + + if tt.clientNil { + p := &Provider{ + sourceProjectID: 666, + } + err := p.CreateComment(context.Background(), tt.event, tt.commit, tt.updateMarker) + assert.ErrorContains(t, err, tt.wantErr) + return + } + + for endpoint, handler := range tt.mockResponses { + mux.HandleFunc(endpoint, handler) + } + + p := &Provider{ + sourceProjectID: 666, + gitlabClient: fakeclient, + } + err := p.CreateComment(context.Background(), tt.event, tt.commit, tt.updateMarker) + if tt.wantErr != "" { + assert.ErrorContains(t, err, tt.wantErr) + } else { + assert.NilError(t, err) + } + }) + } +} diff --git a/pkg/provider/interface.go b/pkg/provider/interface.go index 903ac29267..41e1410c47 100644 --- a/pkg/provider/interface.go +++ b/pkg/provider/interface.go @@ -46,6 +46,7 @@ type Interface interface { CreateToken(context.Context, []string, *info.Event) (string, error) CheckPolicyAllowing(context.Context, *info.Event, []string) (bool, string) GetTemplate(CommentType) string + CreateComment(ctx context.Context, event *info.Event, comment, updateMarker string) error } const DefaultProviderAPIUser = "git" diff --git a/pkg/test/provider/testwebvcs.go b/pkg/test/provider/testwebvcs.go index 634c138213..56e2428571 100644 --- a/pkg/test/provider/testwebvcs.go +++ b/pkg/test/provider/testwebvcs.go @@ -48,6 +48,10 @@ func (v *TestProviderImp) IsAllowedOwnersFile(_ context.Context, _ *info.Event) return v.AllowedInOwnersFile, nil } +func (v *TestProviderImp) CreateComment(_ context.Context, _ *info.Event, _, _ string) error { + return nil +} + func (v *TestProviderImp) SetLogger(_ *zap.SugaredLogger) { } diff --git a/test/README.md b/test/README.md index a4843dd8c4..a3e2c7ea4b 100644 --- a/test/README.md +++ b/test/README.md @@ -76,6 +76,8 @@ You can specify only a subsets of test to run with : same goes for `TestGitlab` or other methods. +If you need to update the golden files in the end-to-end test, add the `-update` flag to the [go test](https://pkg.go.dev/cmd/go#hdr-Test_packages) command to refresh those files. First, run it if you expect the test output to change (or for a new test), then run it again without the flag to ensure everything is correct. + ## Running nightly tests Some tests are set as nightly which mean not run on every PR, because exposing rate limitation often. diff --git a/test/gitea_test.go b/test/gitea_test.go index 842291fbab..44fe3c5db4 100644 --- a/test/gitea_test.go +++ b/test/gitea_test.go @@ -194,13 +194,57 @@ func TestGiteaStepActions(t *testing.T) { tgitea.WaitForSecretDeletion(t, topts, topts.TargetRefName) } +// TestGiteaBadYamlReportingOnPR makes sure that we can catch a bad yaml file +// and report on PR, we only do updates and not creating a new comment all the +// time. +func TestGiteaBadYamlReportingOnPR(t *testing.T) { + topts := &tgitea.TestOpts{ + TargetEvent: triggertype.PullRequest.String(), + YAMLFiles: map[string]string{".tekton/pr-bad-validation.yaml": "testdata/failures/pipeline-validation.yaml"}, + ExpectEvents: true, + } + + _, f := tgitea.TestPR(t, topts) + defer f() + topts.Regexp = regexp.MustCompile(`.*bad-valid | .json: cannot unmarshal array into Go struct field PipelineRunSpec.spec.pipelineSpec of type v1.PipelineSpec.*`) + tgitea.WaitForPullRequestCommentMatch(t, topts) + + comments, _, err := topts.GiteaCNX.Client().ListRepoIssueComments(topts.PullRequest.Base.Repository.Owner.UserName, topts.PullRequest.Base.Repository.Name, gitea.ListIssueCommentOptions{}) + assert.NilError(t, err) + assert.Equal(t, len(comments), 1, "should have only one comment") + + // sending a second time the comment should have been updated + scmOpts := &scm.Opts{ + GitURL: topts.GitCloneURL, + Log: topts.ParamsRun.Clients.Log, + WebURL: topts.GitHTMLURL, + TargetRefName: topts.TargetRefName, + BaseRefName: topts.DefaultBranch, + PushForce: true, + } + processed, err := payload.ApplyTemplate("testdata/failures/pipeline-validation.yaml", map[string]string{ + "TargetNamespace": topts.TargetNS, + "TargetBranch": topts.DefaultBranch, + "TargetEvent": topts.TargetEvent, + "PipelineName": "pr-a-second-time", + "Command": "sleep 10", + }) + assert.NilError(t, err) + entries := map[string]string{".tekton/pr-bad-validation.yaml": processed} + _ = scm.PushFilesToRefGit(t, scmOpts, entries) + + comments, _, err = topts.GiteaCNX.Client().ListRepoIssueComments(topts.PullRequest.Base.Repository.Owner.UserName, topts.PullRequest.Base.Repository.Name, gitea.ListIssueCommentOptions{}) + assert.NilError(t, err) + assert.Equal(t, len(comments), 1, "should have only one comment") +} + // TestGiteaBadYaml we can't check pr status but this shows up in the // controller, so let's dig ourself in there.... TargetNS is a random string, so // it can only success if it matches it. -func TestGiteaBadYaml(t *testing.T) { +func TestGiteaBadYamlValidation(t *testing.T) { topts := &tgitea.TestOpts{ TargetEvent: triggertype.PullRequest.String(), - YAMLFiles: map[string]string{".tekton/pr-bad-format.yaml": "testdata/failures/pipeline_bad_format.yaml"}, + YAMLFiles: map[string]string{".tekton/pr-bad-format.yaml": "testdata/failures/bad-yaml.yaml"}, ExpectEvents: true, } @@ -208,7 +252,8 @@ func TestGiteaBadYaml(t *testing.T) { defer f() maxLines := int64(20) assert.NilError(t, twait.RegexpMatchingInControllerLog(ctx, topts.ParamsRun, *regexp.MustCompile( - "pipelinerun.*has failed.*expected exactly one, got neither: spec.pipelineRef, spec.pipelineSpec"), 10, "controller", &maxLines)) + "cannot read the PipelineRun: pr-bad-format.yaml, error: line 3: could not find expected ':'"), + 10, "controller", &maxLines)) } // TestGiteaInvalidSpecValues tests invalid field values of a PipelinRun and ensures that these diff --git a/test/github_pullrequest_test.go b/test/github_pullrequest_test.go index b0ff73b337..acd28cdcec 100644 --- a/test/github_pullrequest_test.go +++ b/test/github_pullrequest_test.go @@ -159,10 +159,10 @@ func TestGithubPullRequestWebhook(t *testing.T) { defer g.TearDown(ctx, t) } -func TestGithubPullRequestSecondBadYaml(t *testing.T) { +func TestGithubSecondPullRequestBadYaml(t *testing.T) { ctx := context.Background() g := &tgithub.PRTest{ - Label: "Github Rerequest", + Label: "Github PullRequest Bad Yaml", YamlFiles: []string{"testdata/failures/bad-yaml.yaml"}, SecondController: true, NoStatusCheck: true, @@ -170,33 +170,24 @@ func TestGithubPullRequestSecondBadYaml(t *testing.T) { g.RunPullRequest(ctx, t) defer g.TearDown(ctx, t) - opt := github.ListOptions{} - res := &github.ListCheckRunsResults{} - resp := &github.Response{} - var err error - counter := 0 - for { - res, resp, err = g.Provider.Client().Checks.ListCheckRunsForRef(ctx, g.Options.Organization, g.Options.Repo, g.SHA, &github.ListCheckRunsOptions{ - AppID: g.Provider.ApplicationID, - ListOptions: opt, - }) + maxLoop := 10 + for i := 0; i < maxLoop; i++ { + comments, _, err := g.Provider.Client().Issues.ListComments( + ctx, g.Options.Organization, g.Options.Repo, g.PRNumber, + &github.IssueListCommentsOptions{}) assert.NilError(t, err) - assert.Equal(t, resp.StatusCode, 200) - if len(res.CheckRuns) > 0 { - break - } - g.Cnx.Clients.Log.Infof("Waiting for the check run to be created") - if counter > 10 { - t.Errorf("Check run not created after 10 tries") - break + + if len(comments) > 0 { + assert.Assert(t, len(comments) == 1, "Should have only one comment created we got way too many: %+v", comments) + golden.Assert(t, comments[0].GetBody(), strings.ReplaceAll(fmt.Sprintf("%s.golden", t.Name()), "/", "-")) + return } - time.Sleep(5 * time.Second) + + g.Cnx.Clients.Log.Infof("Looping %d/%d waiting for a comment to appear", i, maxLoop) + time.Sleep(6 * time.Second) } - assert.Equal(t, len(res.CheckRuns), 1) - assert.Equal(t, res.CheckRuns[0].GetOutput().GetTitle(), "pipelinerun start failure") - // may be fragile if we change the application name, but life goes on if it fails and we fix the name if that happen - assert.Equal(t, res.CheckRuns[0].GetOutput().GetSummary(), "Pipelines as Code GHE has failed.") - golden.Assert(t, res.CheckRuns[0].GetOutput().GetText(), strings.ReplaceAll(fmt.Sprintf("%s.golden", t.Name()), "/", "-")) + + t.Fatal("No comments with the pipelinerun error found on the pull request") } // TestGithubPullRequestInvalidSpecValues tests invalid field values of a PipelinRun and diff --git a/test/testdata/TestGithubSecondPullRequestBadYaml.golden b/test/testdata/TestGithubSecondPullRequestBadYaml.golden new file mode 100644 index 0000000000..946349c16b --- /dev/null +++ b/test/testdata/TestGithubSecondPullRequestBadYaml.golden @@ -0,0 +1,6 @@ +> [!CAUTION] +> There are some errors in your PipelineRun template. + +| PipelineRun | Error | +|------|-------| +| bad-yaml.yaml | `line 3: could not find expected ':'` | \ No newline at end of file diff --git a/test/testdata/failures/pipeline-validation.yaml b/test/testdata/failures/pipeline-validation.yaml new file mode 100644 index 0000000000..07365b145c --- /dev/null +++ b/test/testdata/failures/pipeline-validation.yaml @@ -0,0 +1,28 @@ +apiVersion: tekton.dev/v1 +kind: PipelineRun +metadata: + name: bad-valid + annotations: + pipelinesascode.tekton.dev/on-event: "[pull_request]" + pipelinesascode.tekton.dev/on-target-branch: "[main]" + pipelinesascode.tekton.dev/max-keep-runs: "5" +spec: + pipelineSpec: + # Customize this task if you like, or just do a taskRef + # to one of the hub task. + - name: noop-task + displayName: Task with no effect + taskSpec: + steps: + - name: noop-task + image: registry.access.redhat.com/ubi9/ubi-micro + script: | + exit 0 + - name: noop-task-2 + displayName: Task with no effect + taskSpec: + steps: + - name: noop-task + image: registry.access.redhat.com/ubi9/ubi-micro + script: | + exit 1