Skip to content

Commit 46067b2

Browse files
chmouelpipelines-as-code[bot]
authored andcommitted
fix: avoid reporting errors for non-Tekton res
The handling of YAML validation errors was refactored to be more intelligent and less noisy. It now only reports errors for resources that are identified as Tekton resources or are fundamentally invalid YAML. - Refactored validation errors from a simple map to a structured type that includes the resource name, schema, and error object. - Updated YAML parsing to extract the resource's `apiVersion` (schema) even when the document fails to decode fully as a Tekton object. - Implemented filtering to report validation errors only for resources with a `tekton.dev` API group or for generic YAML syntax errors. - This prevents creating error comments on pull requests for other valid, non-Tekton YAML files located in the `.tekton` directory. Jira: https://issues.redhat.com/browse/SRVKP-7906 Signed-off-by: Chmouel Boudjnah <[email protected]>
1 parent 002a60d commit 46067b2

File tree

8 files changed

+270
-53
lines changed

8 files changed

+270
-53
lines changed

docs/content/docs/guide/running.md

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -124,9 +124,8 @@ click on it and follow the pipeline execution directly there.
124124

125125
## Errors When Parsing PipelineRun YAML
126126

127-
When Pipelines-As-Code encounters an issue with the YAML formatting in the
128-
repository, it will create comment on pull request describing the error and will log the error in the user namespace events log and
129-
the Pipelines-as-Code controller log.
127+
If Pipelines-As-Code encounters an issue with the YAML formatting of Tekton resources in the repository, it will create a comment on
128+
the pull request describing the error. The error will also be logged in the user namespace events log and in the Pipelines-as-Code controller log.
130129

131130
Despite validation errors, Pipelines-as-Code continues to run other correctly parsed and matched PipelineRuns.
132131
However, if a PipelineRun has YAML syntax error, it halts the execution of all PipelineRuns, even those that are syntactically correct.

pkg/errors/errors.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
package errors
2+
3+
type PacYamlValidations struct {
4+
Name string
5+
Err error
6+
Schema string
7+
}
8+
9+
const GenericBadYAMLValidation = "Generic bad YAML Validation"

pkg/pipelineascode/match.go

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99

1010
apipac "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys"
1111
"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
12+
pacerrors "github.com/openshift-pipelines/pipelines-as-code/pkg/errors"
1213
"github.com/openshift-pipelines/pipelines-as-code/pkg/matcher"
1314
"github.com/openshift-pipelines/pipelines-as-code/pkg/opscomments"
1415
"github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype"
@@ -186,7 +187,15 @@ func (p *PacRun) getPipelineRunsFromRepo(ctx context.Context, repo *v1alpha1.Rep
186187
reg := regexp.MustCompile(`error unmarshalling yaml file\s([^:]*):\s*(yaml:\s*)?(.*)`)
187188
matches := reg.FindStringSubmatch(err.Error())
188189
if len(matches) == 4 {
189-
p.reportValidationErrors(ctx, repo, map[string]string{matches[1]: matches[3]})
190+
p.reportValidationErrors(ctx, repo,
191+
[]*pacerrors.PacYamlValidations{
192+
{
193+
Name: matches[1],
194+
Err: fmt.Errorf("yaml validation error: %s", matches[3]),
195+
Schema: pacerrors.GenericBadYAMLValidation,
196+
},
197+
},
198+
)
190199
return nil, nil
191200
}
192201

@@ -463,12 +472,19 @@ func (p *PacRun) createNeutralStatus(ctx context.Context) error {
463472
// 1. Creating error messages for each validation error
464473
// 2. Emitting error messages to the event system
465474
// 3. Creating a markdown formatted comment on the repository with all errors.
466-
func (p *PacRun) reportValidationErrors(ctx context.Context, repo *v1alpha1.Repository, validationErrors map[string]string) {
475+
func (p *PacRun) reportValidationErrors(ctx context.Context, repo *v1alpha1.Repository, validationErrors []*pacerrors.PacYamlValidations) {
467476
errorRows := make([]string, 0, len(validationErrors))
468-
for name, err := range validationErrors {
469-
errorRows = append(errorRows, fmt.Sprintf("| %s | `%s` |", name, err))
477+
for _, err := range validationErrors {
478+
// if the error is a TektonConversionError, we don't want to report it since it may be a file that is not a tekton resource
479+
// and we don't want to report it as a validation error.
480+
if strings.HasPrefix(err.Schema, tektonv1.SchemeGroupVersion.Group) || err.Schema == pacerrors.GenericBadYAMLValidation {
481+
errorRows = append(errorRows, fmt.Sprintf("| %s | `%s` |", err.Name, err.Err.Error()))
482+
}
470483
p.eventEmitter.EmitMessage(repo, zap.ErrorLevel, "PipelineRunValidationErrors",
471-
fmt.Sprintf("cannot read the PipelineRun: %s, error: %s", name, err))
484+
fmt.Sprintf("cannot read the PipelineRun: %s, error: %s", err.Name, err.Err.Error()))
485+
}
486+
if len(errorRows) == 0 {
487+
return
472488
}
473489
markdownErrMessage := fmt.Sprintf(`%s
474490
%s`, validationErrorTemplate, strings.Join(errorRows, "\n"))

pkg/resolve/resolve.go

Lines changed: 26 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,11 @@ import (
44
"context"
55
"fmt"
66
"regexp"
7+
"slices"
78
"strings"
89

910
apipac "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys"
11+
pacerrors "github.com/openshift-pipelines/pipelines-as-code/pkg/errors"
1012
"github.com/openshift-pipelines/pipelines-as-code/pkg/formatting"
1113
"github.com/openshift-pipelines/pipelines-as-code/pkg/matcher"
1214
"github.com/openshift-pipelines/pipelines-as-code/pkg/params"
@@ -15,7 +17,6 @@ import (
1517
tektonv1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
1618
tektonv1beta1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
1719
"go.uber.org/zap"
18-
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1920
k8scheme "k8s.io/client-go/kubernetes/scheme"
2021
yaml "sigs.k8s.io/yaml/goyaml.v2"
2122
)
@@ -26,7 +27,7 @@ type TektonTypes struct {
2627
Pipelines []*tektonv1.Pipeline
2728
TaskRuns []*tektonv1.TaskRun
2829
Tasks []*tektonv1.Task
29-
ValidationErrors map[string]string
30+
ValidationErrors []*pacerrors.PacYamlValidations
3031
}
3132

3233
// Contains Fetched Resources for Event, with key equals to annotation value.
@@ -43,31 +44,34 @@ type FetchedResourcesForRun struct {
4344

4445
func NewTektonTypes() TektonTypes {
4546
return TektonTypes{
46-
ValidationErrors: map[string]string{},
47+
ValidationErrors: []*pacerrors.PacYamlValidations{},
4748
}
4849
}
4950

5051
var yamlDocSeparatorRe = regexp.MustCompile(`(?m)^---\s*$`)
5152

52-
// detectAtleastNameOrGenerateNameFromPipelineRun detects the name or
53+
// detectAtleastNameOrGenerateNameAndSchemaFromPipelineRun detects the name or
5354
// generateName of a yaml files even if there is an error decoding it as tekton types.
54-
func detectAtleastNameOrGenerateNameFromPipelineRun(data string) string {
55-
var metadataName struct {
56-
Metadata metav1.ObjectMeta
55+
func detectAtleastNameOrGenerateNameAndSchemaFromPipelineRun(data string) (string, string) {
56+
var genericKubeObj struct {
57+
APIVersion string `yaml:"apiVersion"`
58+
Metadata struct {
59+
Name string `yaml:"name,omitempty"`
60+
GenerateName string `yaml:"generateName,omitempty"`
61+
} `yaml:"metadata"`
5762
}
58-
err := yaml.Unmarshal([]byte(data), &metadataName)
63+
err := yaml.Unmarshal([]byte(data), &genericKubeObj)
5964
if err != nil {
60-
return ""
65+
return "nokube", ""
6166
}
62-
if metadataName.Metadata.Name != "" {
63-
return metadataName.Metadata.Name
67+
if genericKubeObj.Metadata.Name != "" {
68+
return genericKubeObj.Metadata.Name, genericKubeObj.APIVersion
6469
}
6570

66-
// TODO: yaml Unmarshal don't want to parse generatename and i have no idea why
67-
if metadataName.Metadata.GenerateName != "" {
68-
return metadataName.Metadata.GenerateName
71+
if genericKubeObj.Metadata.GenerateName != "" {
72+
return genericKubeObj.Metadata.GenerateName, genericKubeObj.APIVersion
6973
}
70-
return "unknown"
74+
return "unknown", genericKubeObj.APIVersion
7175
}
7276

7377
// getPipelineByName returns the Pipeline with the given name the first one found
@@ -106,15 +110,6 @@ func pipelineRunsWithSameName(prs []*tektonv1.PipelineRun) error {
106110
return nil
107111
}
108112

109-
func skippingTask(taskName string, skippedTasks []string) bool {
110-
for _, value := range skippedTasks {
111-
if value == taskName {
112-
return true
113-
}
114-
}
115-
return false
116-
}
117-
118113
func isTektonAPIVersion(apiVersion string) bool {
119114
return strings.HasPrefix(apiVersion, "tekton.dev/") || apiVersion == ""
120115
}
@@ -126,7 +121,7 @@ func inlineTasks(tasks []tektonv1.PipelineTask, ropt *Opts, remoteResource Fetch
126121
task.TaskRef.Resolver == "" &&
127122
isTektonAPIVersion(task.TaskRef.APIVersion) &&
128123
string(task.TaskRef.Kind) != "ClusterTask" &&
129-
!skippingTask(task.TaskRef.Name, ropt.SkipInlining) {
124+
!slices.Contains(ropt.SkipInlining, task.TaskRef.Name) {
130125
taskResolved, ok := remoteResource.Tasks[task.TaskRef.Name]
131126
if !ok {
132127
return nil, fmt.Errorf("cannot find referenced task %s. if it's a remote task make sure to add it in the annotations", task.TaskRef.Name)
@@ -164,7 +159,12 @@ func ReadTektonTypes(ctx context.Context, log *zap.SugaredLogger, data string) (
164159

165160
obj, _, err := decoder.Decode([]byte(doc), nil, nil)
166161
if err != nil {
167-
types.ValidationErrors[detectAtleastNameOrGenerateNameFromPipelineRun(doc)] = err.Error()
162+
dt, dv := detectAtleastNameOrGenerateNameAndSchemaFromPipelineRun(doc)
163+
types.ValidationErrors = append(types.ValidationErrors, &pacerrors.PacYamlValidations{
164+
Name: dt,
165+
Err: fmt.Errorf("error decoding yaml document: %w", err),
166+
Schema: dv,
167+
})
168168
continue
169169
}
170170
switch o := obj.(type) {

0 commit comments

Comments
 (0)