feat: Report validation errors on pull requests

This commit introduces the capability to report PipelineRun
validation errors as comments on pull requests. This functionality
is triggered when validation errors are detected and the event
type is a pull request. The errors are formatted into a
markdown table and posted as a comment on the pull request,
allowing users to easily identify and address issues in their
PipelineRun templates.

The commit also includes updates to the provider interfaces to
include a CreateComment function, and implementations for
GitHub, GitLab, Bitbucket Cloud, Bitbucket Server and Gitea
providers.

Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>

Author: chmouel

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

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

docs/static/images/report-error-comment-on-bad-yaml.png

@@ -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()))
+	}
+}

pkg/pipelineascode/match.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",

pkg/pipelineascode/match_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 := &params.Run{
 				Clients: clients.Clients{

pkg/pipelineascode/pipelineascode_test.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, ""

pkg/provider/bitbucketcloud/bitbucket.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
 }

pkg/provider/bitbucketdatacenter/bitbucketdatacenter.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
 }

pkg/provider/gitea/gitea.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)
+			}
+		})
+	}
+}