fix: avoid reporting errors for non-Tekton res

[chmouel]

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

Changes

Submitter Checklist

• 📝 Ensure your commit message is clear and informative. Refer to the How to write a git commit message guide. Include the commit message in the PR body rather than linking to an external site (e.g., Jira ticket).

• ♽ Run make test lint before submitting a PR to avoid unnecessary CI processing. Consider installing pre-commit and running pre-commit install in the repository root for an efficient workflow.

• ✨ We use linters to maintain clean and consistent code. Run make lint before submitting a PR. Some linters offer a --fix mode, executable with make fix-linters (ensure markdownlint and golangci-lint are installed).

• 📖 Document any user-facing features or changes in behavior.

• 🧪 While 100% coverage isn't required, we encourage unit tests for code changes where possible.

• 🎁 If feasible, add an end-to-end test. See README for details.

• 🔎 Address any CI test flakiness before merging, or provide a valid reason to bypass it (e.g., token rate limitations).

• If adding a provider feature, fill in the following details:

  • GitHub App
  • GitHub Webhook
  • Gitea/Forgejo
  • GitLab
  • Bitbucket Cloud
  • Bitbucket Data Center

  (update the provider documentation accordingly)

[chmouel]

AI Review from gemini-pro

Of course! I've carefully analyzed the provided git changes. Here's a breakdown of my findings, covering improvements, potential issues, and overall code quality.

This change is a well-executed refactoring of the YAML validation error handling. It significantly improves the system's robustness and the clarity of error reporting for users.

Overall Analysis

The main goal of this change is to replace a simple map[string]string for validation errors with a more structured type, pacerrors.PacYamlValidations. This new struct holds the resource name, the error object itself, and the resource's schema (apiVersion). This is a major improvement that enables more sophisticated error handling.

The changes are consistently applied across the resolve package (where errors are generated) and the pipelineascode package (where errors are reported), and the tests are updated thoroughly.

---

File-by-File Breakdown

1. pkg/errors/errors.go (New File)

• Improvement: The introduction of the PacYamlValidations struct is excellent. It provides a strongly-typed, structured way to handle errors, which is much better than passing around maps of strings. Storing the error type directly preserves the original error context.
• Improvement: The GenericBadYAMLValidation constant provides a clear, reusable way to classify YAML syntax errors that occur before a schema can even be identified.

This new file establishes a solid foundation for the refactoring.

2. pkg/resolve/resolve.go

• Improvement (Code Idiom): The use of slices.Contains to replace the skippingTask helper function is a great example of adopting modern, idiomatic Go. It's cleaner and more readable.
• Improvement (Robustness): The new function detectAtleastNameOrGenerateNameAndSchemaFromPipelineRun is more robust than its predecessor. It attempts to parse apiVersion in addition to the name, providing valuable context even when the document is malformed.
• Improvement (Error Handling): In ReadTektonTypes, when a YAML document fails to decode, the error is now wrapped using fmt.Errorf("...: %w", err). This preserves the original error while adding context, which is best practice. The error is then stored in the new PacYamlValidations struct.

This file is the core of the change, and the improvements here are solid.

3. pkg/pipelineascode/match.go

• Improvement (User Experience): The most significant improvement is in reportValidationErrors. The new logic filters errors based on their schema:

  if strings.HasPrefix(err.Schema, tektonv1.SchemeGroupVersion.Group) || err.Schema == pacerrors.GenericBadYAMLValidation

  This ensures that comments are only created for actual Tekton resource validation errors or malformed YAML files. It smartly ignores other valid Kubernetes resources or configuration files that a user might have in their .tekton/ directory, which drastically reduces noise and false-positive error reports.
• Improvement (Bug Fix): The addition of if len(errorRows) == 0 { return } prevents the creation of an empty validation report on the pull request, which would happen if all detected errors were filtered out. This is a subtle but important fix.
• Minor Suggestion (Clarity): The comment above the new filtering logic is slightly imprecise:

  // if the error is a TektonConversionError, we don't want to report it...

  The code actually checks the resource's apiVersion group (tekton.dev), not a specific error type. A more accurate comment would be:

  // Only report validation errors for Tekton resources or generic bad YAML.
  // This prevents creating comments for other valid resources (e.g., ConfigMaps)
  // that might be present in the .tekton directory.

4. pkg/resolve/resolve_test.go

• Improvement (Test Quality): The tests have been updated comprehensively. This is a sign of a high-quality change.
  • TestReportBadTektonYaml is correctly refactored to work with the new slice of structs.
  • The new tests (TestDetectNameOrGenerateNameAndSchema, TestValidationErrorStructure, TestValidationErrorFiltering, TestErrorMessageFormat) are excellent. They specifically target all aspects of the new functionality, from schema detection to the crucial filtering logic.
  • Testing the filtering logic from the consuming package (match.go) here is a good decision, as it ensures the data produced by resolve is fit for its intended purpose.

Bugs or Regressions

I found no bugs or regressions in this change. It appears to be a pure improvement and bug-fix release, addressing the shortcomings of the previous implementation.

Conclusion

This is an excellent change. It's a thoughtful refactoring that improves code quality, robustness, and the end-user experience. The code is clean, idiomatic, and very well-tested. I would approve this change without hesitation.

[Copilot]

Pull Request Overview

This PR refactors YAML validation error handling to only report issues for Tekton resources or fundamentally invalid YAML by introducing a structured error type and filtering logic.

• Introduced PacYamlValidations struct and GenericBadYAMLValidation constant.
• Updated ReadTektonTypes and reportValidationErrors to collect and filter structured errors by Tekton API group or generic YAML syntax.
• Added end-to-end tests and a Gitea integration test to verify non-Tekton resources are ignored.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
test/testdata/randomcrd.yaml	Added CRD YAML for non-Tekton resource test
test/gitea_test.go	New test to ensure non-Tekton YAML does not generate comments on PR
pkg/resolve/resolve.go	Refactored YAML decoding to record structured errors and filter by schema
pkg/resolve/resolve_test.go	Updated tests to validate structured errors, schema detection, and filtering logic
pkg/pipelineascode/match.go	Adapted getPipelineRunsFromRepo to use structured errors type
pkg/errors/errors.go	Introduced PacYamlValidations type and GenericBadYAMLValidation constant
docs/content/docs/guide/running.md	Updated documentation to clarify error reporting scope for Tekton resources

Comments suppressed due to low confidence (6)

pkg/resolve/resolve.go:55

• [nitpick] The function name has a typo: Atleast should be AtLeast for correct casing and readability. Consider renaming to detectAtLeastNameOrGenerateNameAndSchemaFromPipelineRun.

func detectAtleastNameOrGenerateNameAndSchemaFromPipelineRun(data string) (string, string) {

test/gitea_test.go:241

• [nitpick] The test name is a bit long and uses a double negative. Consider renaming to something clearer like TestGiteaDoesNotReportNonTektonResources.

func TestGiteaYamlReportingNotReportingNotTektonResources(t *testing.T) {

pkg/pipelineascode/match.go:194

• The code uses fmt.Errorf but fmt is not imported in this file. Please add import "fmt" to resolve the compilation error.

						Err:    fmt.Errorf("yaml validation error: %s", matches[3]),

pkg/resolve/resolve_test.go:333

• The assert.Equal arguments are reversed: the literal should be the expected value and the constant should be the actual. Swap them for clearer intent (assert.Equal(t, "Generic bad YAML Validation", pacerrors.GenericBadYAMLValidation)).

	assert.Equal(t, pacerrors.GenericBadYAMLValidation, "Generic bad YAML Validation")

[zakisk]

if there is any non-tekton resource with invalid YAML, PaC will still fail to process the event, right?

[chmouel]

@zakisk that's correct, if it ends up with .yaml and it's invalid it would fail, but then it's up to the user to not put *** in their .tekton

[zakisk]

but if comment is not created for non-tekton resources (and it shouldn't) it will create confusion that why event is not processed? I think non-tekton resources should be ignored whether they're good or bad, wdyt?

[chmouel]

we can't read the yaml file because of the error so we don't really know if it's a tekton resource or not, we try to grep inot the yaml if there is a kind: PipelineRun but that would be not enough

[zakisk]

/lgtm

[zakisk]

/lgtm

[pipelines-as-code]

Congrats @chmouel your PR Has been approved 🎉

✅ Pull Request Approved

Approval Status:

• Required Approvals: 1
• Current Approvals: 1

👥 Reviewers Who Approved:

Reviewer	Permission Level	Approval Status
@zakisk	admin	✅

📝 Next Steps

• Ensure all required checks pass
• Comply with branch protection rules
• Request a maintainer to merge using the /merge command (or merge it
  directly if you have repository permission).

Automated by the PAC Boussole 🧭

[zakisk]

/merge

[pipelines-as-code]

✅ PR Successfully Merged

• Merge method: rebase
• Merged by: @zakisk
• Total approvals: 1/1

Approvals Summary:

Reviewer	Permission	Status
@zakisk	admin	✅

Thank you @chmouel for your valuable contribution! 🎉

Automated by the PAC Boussole 🧭