Print friendly error message on bad detected yaml

We now print a more friendly error message when we detect a bad yaml by
showing the errors on the git provider interface and the namespace
events stream.

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

.yamllint

@@ -2,6 +2,8 @@ ignore: |
   /vendor
   .git
   test/testdata/failures/pipeline_bad_format.yaml
+  test/testdata/failures/bad-yaml.yaml
+  *badyaml.yaml
   .github/workflows/*
 
 rules:

docs/content/docs/guide/resolver.md

@@ -4,13 +4,20 @@ weight: 2
 ---
 # Pipelines-as-Code resolver
 
-Pipelines-as-Code resolver ensures the PipelineRun you are running does not
-conflicts with others.
+The Pipelines-as-Code resolver ensures that the PipelineRun you are running
+doesn't conflict with others.
 
-If `Pipelines-as-Code` sees a PipelineRun with a reference to a `Task` or to a
-`Pipeline` in any YAML file located in the `.tekton/` directory it will
-automatically try to *resolves* it (see below) as a single PipelineRun with an
-embedded `PipelineSpec` to a `PipelineRun`.
+When Pipelines-as-Code detects a PipelineRun referencing a Task or Pipeline in
+any YAML file within the .tekton/ directory.
+
+In case of errors in any YAML files in the `.tekton` directory, parsing will
+halt, and errors will be reported on both the Git provider interface and the
+event's Namespace stream.
+
+It will automatically attempt to detect the tekton yaml files and resolved them
+as a single PipelineRun with an embedded PipelineSpec. This embedding ensures
+that all dependencies required for execution are contained within a single
+PipelineRun at the time of execution on the cluster.
 
 The resolver will then transform the Pipeline `Name` field to a `GenerateName`
 based on the Pipeline name as well.

pkg/pipelineascode/match.go

@@ -150,6 +150,13 @@ 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") {
+		// 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(errmsg)
+	}
 	if err != nil || rawTemplates == "" {
 		msg := fmt.Sprintf("cannot locate templates in %s/ directory for this repository in %s", tektonDir, p.event.HeadBranch)
 		if err != nil {

pkg/pipelineascode/pipelineascode.go

@@ -54,9 +54,9 @@ func (p *PacRun) Run(ctx context.Context) error {
 			Text:       fmt.Sprintf("There was an issue validating the commit: %q", err),
 			DetailsURL: p.run.Clients.ConsoleUI.URL(),
 		})
-		p.eventEmitter.EmitMessage(repo, zap.ErrorLevel, "RepositoryCreateStatus", fmt.Sprintf("There was an error while processing the payload: %s", err))
+		p.eventEmitter.EmitMessage(repo, zap.ErrorLevel, "RepositoryCreateStatus", fmt.Sprintf("an error occurred: %s", err))
 		if createStatusErr != nil {
-			p.eventEmitter.EmitMessage(repo, zap.ErrorLevel, "RepositoryCreateStatus", fmt.Sprintf("Cannot create status: %s: %s", err, createStatusErr))
+			p.eventEmitter.EmitMessage(repo, zap.ErrorLevel, "RepositoryCreateStatus", fmt.Sprintf("cannot create status: %s: %s", err, createStatusErr))
 		}
 	}
 	if len(matchedPRs) == 0 {

pkg/pipelineascode/testdata/bad_yaml/.tekton/badyaml.yaml

@@ -0,0 +1,3 @@
+foo:
+    bar:
+   xlxlxl

pkg/provider/bitbucketcloud/bitbucket.go

@@ -17,6 +17,7 @@ import (
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/provider"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/provider/bitbucketcloud/types"
 	"go.uber.org/zap"
+	"gopkg.in/yaml.v2"
 )
 
 var _ provider.Interface = (*Provider)(nil)
@@ -256,6 +257,10 @@ func (v *Provider) concatAllYamlFiles(objects []bitbucket.RepositoryFile, event
 			if err != nil {
 				return "", err
 			}
+			var i any
+			if err := yaml.Unmarshal([]byte(data), &i); err != nil {
+				return "", fmt.Errorf("error unmarshalling yaml file %s: %w", value.Path, err)
+			}
 
 			if allTemplates != "" && !strings.HasPrefix(data, "---") {
 				allTemplates += "---"

pkg/provider/bitbucketcloud/bitbucket_test.go

@@ -28,7 +28,7 @@ func TestGetTektonDir(t *testing.T) {
 		event                *info.Event
 		testDirPath          string
 		contentContains      string
-		wantErr              bool
+		wantErr              string
 		removeSuffix         bool
 		provenance           string
 		filterMessageSnippet string
@@ -54,6 +54,12 @@ func TestGetTektonDir(t *testing.T) {
 			testDirPath:     "../../pipelineascode/testdata/subdir/.tekton",
 			contentContains: "kind: PipelineRun",
 		},
+		{
+			name:        "Bad yaml files in there",
+			event:       bbcloudtest.MakeEvent(nil),
+			testDirPath: "../../pipelineascode/testdata/bad_yaml/.tekton",
+			wantErr:     "error unmarshalling yaml file .tekton/badyaml.yaml: yaml: line 2: did not find expected key",
+		},
 		{
 			name:            "No yaml files in there",
 			event:           bbcloudtest.MakeEvent(nil),
@@ -71,8 +77,9 @@ func TestGetTektonDir(t *testing.T) {
 			v := &Provider{Logger: fakelogger, Client: bbclient}
 			bbcloudtest.MuxDirContent(t, mux, tt.event, tt.testDirPath, tt.provenance)
 			content, err := v.GetTektonDir(ctx, tt.event, ".tekton", tt.provenance)
-			if tt.wantErr {
-				assert.Assert(t, err != nil, "GetTektonDir() error = %v, wantErr %v", err, tt.wantErr)
+			if tt.wantErr != "" {
+				assert.Assert(t, err != nil, "expected error %s, got %v", tt.wantErr, err)
+				assert.Equal(t, err.Error(), tt.wantErr)
 				return
 			}
 			if tt.contentContains == "" {

pkg/provider/gitea/gitea.go

@@ -20,6 +20,7 @@ import (
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/provider"
 	"go.uber.org/zap"
+	"gopkg.in/yaml.v2"
 )
 
 const (
@@ -251,6 +252,11 @@ func (v *Provider) concatAllYamlFiles(objects []gitea.GitEntry, event *info.Even
 			if err != nil {
 				return "", err
 			}
+			// validate yaml
+			var i any
+			if err := yaml.Unmarshal(data, &i); err != nil {
+				return "", fmt.Errorf("error unmarshalling yaml file %s: %w", value.Path, err)
+			}
 			if allTemplates != "" && !strings.HasPrefix(string(data), "---") {
 				allTemplates += "---"
 			}

pkg/provider/github/github.go

@@ -22,6 +22,7 @@ import (
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/provider"
 	"go.uber.org/zap"
 	"golang.org/x/oauth2"
+	"k8s.io/apimachinery/pkg/util/yaml"
 	"k8s.io/client-go/kubernetes"
 )
 
@@ -403,6 +404,11 @@ func (v *Provider) concatAllYamlFiles(ctx context.Context, objects []*github.Tre
 			if err != nil {
 				return "", err
 			}
+			// validate yaml
+			var i any
+			if err := yaml.Unmarshal(data, &i); err != nil {
+				return "", fmt.Errorf("error unmarshalling yaml file %s: %w", value.GetPath(), err)
+			}
 			if allTemplates != "" && !strings.HasPrefix(string(data), "---") {
 				allTemplates += "---"
 			}

pkg/provider/github/github_test.go

@@ -231,6 +231,7 @@ func TestGetTektonDir(t *testing.T) {
 		expectedString       string
 		provenance           string
 		filterMessageSnippet string
+		wantErr              string
 	}{
 		{
 			name: "test no subtree",
@@ -265,6 +266,17 @@ func TestGetTektonDir(t *testing.T) {
 			expectedString: "FROMSUBTREE",
 			treepath:       "testdata/tree/subdir",
 		},
+		{
+			name: "test with badly formatted yaml",
+			event: &info.Event{
+				Organization: "tekton",
+				Repository:   "cat",
+				SHA:          "123",
+			},
+			expectedString: "FROMSUBTREE",
+			treepath:       "testdata/tree/badyaml",
+			wantErr:        "error unmarshalling yaml file badyaml.yaml: error converting YAML to JSON: yaml: line 2: did not find expected key",
+		},
 	}
 	for _, tt := range testGetTektonDir {
 		t.Run(tt.name, func(t *testing.T) {
@@ -286,6 +298,11 @@ func TestGetTektonDir(t *testing.T) {
 			ghtesthelper.SetupGitTree(t, mux, tt.treepath, tt.event, false)
 
 			got, err := gvcs.GetTektonDir(ctx, tt.event, ".tekton", tt.provenance)
+			if tt.wantErr != "" {
+				assert.Assert(t, err != nil, "we should have get an error here")
+				assert.Equal(t, tt.wantErr, err.Error())
+				return
+			}
 			assert.NilError(t, err)
 			assert.Assert(t, strings.Contains(got, tt.expectedString), "expected %s, got %s", tt.expectedString, got)
 			if tt.filterMessageSnippet != "" {

pkg/provider/github/testdata/tree/badyaml/.tekton/badyaml.yaml

@@ -0,0 +1,3 @@
+foo:
+    bar:
+   xlxlxl

pkg/provider/gitlab/gitlab.go

@@ -18,6 +18,7 @@ import (
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/provider"
 	"github.com/xanzy/go-gitlab"
 	"go.uber.org/zap"
+	"gopkg.in/yaml.v2"
 )
 
 const (
@@ -36,6 +37,7 @@ const (
 </td></tr>
 {{- end }}
 </table>`
+	noClientErrStr = `no gitlab client has been initialized, exiting... (hint: did you forget setting a secret on your repo?)`
 )
 
 var _ provider.Interface = (*Provider)(nil)
@@ -247,14 +249,18 @@ func (v *Provider) GetTektonDir(_ context.Context, event *info.Event, path, prov
 // 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) {
 	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)
 			if err != nil {
 				return "", err
 			}
+			// validate yaml
+			var i any
+			if err := yaml.Unmarshal(data, &i); err != nil {
+				return "", fmt.Errorf("error unmarshalling yaml file %s: %w", value.Path, err)
+			}
 			if allTemplates != "" && !strings.HasPrefix(string(data), "---") {
 				allTemplates += "---"
 			}
@@ -289,8 +295,7 @@ func (v *Provider) GetFileInsideRepo(_ context.Context, runevent *info.Event, pa
 
 func (v *Provider) GetCommitInfo(_ context.Context, runevent *info.Event) error {
 	if v.Client == nil {
-		return fmt.Errorf("no gitlab client has been initialized, " +
-			"exiting... (hint: did you forget setting a secret on your repo?)")
+		return fmt.Errorf(noClientErrStr)
 	}
 
 	// if we don't have a SHA (ie: incoming-webhook) then get it from the branch

pkg/provider/gitlab/gitlab_test.go

@@ -257,14 +257,14 @@ func TestGetTektonDir(t *testing.T) {
 		fields               fields
 		args                 args
 		wantStr              string
-		wantErr              bool
+		wantErr              string
 		wantClient           bool
 		prcontent            string
 		filterMessageSnippet string
 	}{
 		{
 			name:    "no client set",
-			wantErr: true,
+			wantErr: noClientErrStr,
 		},
 		{
 			name:       "not found, no err",
@@ -274,11 +274,15 @@ func TestGetTektonDir(t *testing.T) {
 		{
 			name:       "bad yaml",
 			wantClient: true,
-			args:       args{event: &info.Event{SHA: "abcd", HeadBranch: "main"}},
+			args: args{
+				event: &info.Event{SHA: "abcd", HeadBranch: "main"},
+				path:  ".tekton",
+			},
 			fields: fields{
 				sourceProjectID: 10,
 			},
-			prcontent: "bad yaml",
+			prcontent: "bad:\n- yaml\nfoo",
+			wantErr:   "error unmarshalling yaml file .tekton/subtree/pr.yaml: yaml: line 4: could not find expected ':'",
 		},
 		{
 			name:      "list tekton dir",
@@ -354,8 +358,9 @@ func TestGetTektonDir(t *testing.T) {
 			}
 
 			got, err := v.GetTektonDir(ctx, tt.args.event, tt.args.path, tt.args.provenance)
-			if (err != nil) != tt.wantErr {
-				t.Errorf("GetTektonDir() error = %v, wantErr %v", err, tt.wantErr)
+			if tt.wantErr != "" {
+				assert.Assert(t, err != nil, "expected error %s, got %v", tt.wantErr, err)
+				assert.Equal(t, err.Error(), tt.wantErr)
 				return
 			}
 			if tt.wantStr != "" {

test/github_pullrequest_test.go

@@ -5,10 +5,16 @@ package test
 
 import (
 	"context"
+	"fmt"
 	"os"
+	"strings"
 	"testing"
+	"time"
 
+	"github.com/google/go-github/v56/github"
 	tgithub "github.com/openshift-pipelines/pipelines-as-code/test/pkg/github"
+	"gotest.tools/v3/assert"
+	"gotest.tools/v3/golden"
 )
 
 func TestGithubPullRequest(t *testing.T) {
@@ -87,6 +93,46 @@ func TestGithubPullRequestWebhook(t *testing.T) {
 	defer g.TearDown(ctx, t)
 }
 
+func TestGithubPullRequestSecondBadYaml(t *testing.T) {
+	ctx := context.Background()
+	g := &tgithub.PRTest{
+		Label:            "Github Rerequest",
+		YamlFiles:        []string{"testdata/failures/bad-yaml.yaml"},
+		SecondController: true,
+		NoStatusCheck:    true,
+	}
+	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,
+		})
+		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
+		}
+		time.Sleep(5 * time.Second)
+	}
+	assert.Equal(t, len(res.CheckRuns), 1)
+	assert.Equal(t, res.CheckRuns[0].GetOutput().GetTitle(), "Failed")
+	// 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 <b>failed</b>.")
+	golden.Assert(t, res.CheckRuns[0].GetOutput().GetText(), strings.ReplaceAll(fmt.Sprintf("%s.golden", t.Name()), "/", "-"))
+}
+
 // Local Variables:
 // compile-command: "go test -tags=e2e -v -info TestGithubPullRequest$ ."
 // End:

test/testdata/TestGithubPullRequestSecondBadYaml.golden

@@ -0,0 +1 @@
+There was an issue validating the commit: "error while parsing the yaml file bad-yaml.yaml: line 3: could not find expected ':'"

test/testdata/failures/bad-yaml.yaml

@@ -0,0 +1,3 @@
+foo: aaa 1
+1
+bar: - aaaa