Skip to content

Conversation

@chmouel
Copy link
Member

@chmouel chmouel commented Feb 26, 2025

Changes

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.

Jira: https://issues.redhat.com/browse/SRVKP-7075

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:

Git Provider Supported
GitHub App ❌️
GitHub Webhook ✅️
Gitea ✅️
GitLab ✅️
Bitbucket Cloud ❌️
Bitbucket Server ❌️

@chmouel chmouel changed the title feat: Report validation errors on pull requests feat: Report validation errors on pull requests as comment Feb 26, 2025
@chmouel chmouel force-pushed the report-yaml-error-properly-as-comment branch 2 times, most recently from 1a30d06 to b63beda Compare February 27, 2025 16:08
@chmouel chmouel requested a review from sm43 February 27, 2025 16:08
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👼🏼

@chmouel chmouel requested a review from Copilot March 1, 2025 15:21
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Overview

This PR adds functionality to report PipelineRun validation errors as comments on pull requests and introduces a new provider interface function CreateComment. Key changes include:

  • Reporting PipelineRun YAML validation errors as a markdown-formatted comment on pull requests.
  • Adding a CreateComment function implementation across multiple provider packages (GitHub, GitLab, Gitea, Bitbucket Cloud, Bitbucket Server).
  • Updating tests and documentation to reflect the new error reporting behavior.

Reviewed Changes

File Description
test/testdata/failures/pipeline-validation.yaml New invalid PipelineRun template for testing validation errors
test/gitea_test.go Added tests for reporting bad YAML errors on pull requests
docs/content/docs/guide/running.md Added documentation for YAML parsing error reporting
pkg/provider/github/github.go Added CreateComment implementation for GitHub
pkg/pipelineascode/match.go Enhanced error reporting by creating a markdown comment with YAML errors
pkg/provider/gitlab/gitlab.go, gitea.go, etc. Added CreateComment implementations for other providers
pkg/provider/interface.go Updated provider interface to include CreateComment

Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.

@chmouel chmouel force-pushed the report-yaml-error-properly-as-comment branch 2 times, most recently from 06c2a02 to 4c16336 Compare March 10, 2025 12:38
@aThorp96
Copy link
Member

Once this is merged, I think it will also lay a good foundation for alerting users to warnings such as Warning: Both 'on-event' and 'on-cel-expression' annotations were found on <pipeline> but are mutually exclusive. Only 'on-cel-expression' will be checked

@chmouel chmouel force-pushed the report-yaml-error-properly-as-comment branch from 4c16336 to fc81197 Compare March 10, 2025 12:45
@chmouel
Copy link
Member Author

chmouel commented Mar 10, 2025

/test doc-generation

@chmouel
Copy link
Member Author

chmouel commented Mar 10, 2025

Once this is merged, I think it will also lay a good foundation for alerting users to warnings such as Warning: Both 'on-event' and 'on-cel-expression' annotations were found on but are mutually exclusive. Only 'on-cel-expression' will be checked

I'd like to get a full blown linter available in tkn pac for this, maybe we can add the output in the comment... or inline as annotations on the code

@chmouel
Copy link
Member Author

chmouel commented Mar 10, 2025

but let's discuss this on the jira/issue

@zakisk
Copy link
Contributor

zakisk commented Mar 10, 2025

/retest

1 similar comment
@chmouel
Copy link
Member Author

chmouel commented Mar 10, 2025

/retest

@chmouel chmouel force-pushed the report-yaml-error-properly-as-comment branch 2 times, most recently from f529b6d to 1c9584e Compare March 12, 2025 09:14
@vdemeester
Copy link
Member

Testing

@chmouel
Copy link
Member Author

chmouel commented Mar 17, 2025

/cancel go-testing

@chmouel
Copy link
Member Author

chmouel commented Mar 17, 2025

/retest

@zakisk
Copy link
Contributor

zakisk commented Mar 18, 2025

@chmouel I was testing this on my PR, so PR was having only comment /test before I pushed bad PLR yaml, after pushing a bad PLR yaml error was reported but /test comment was edited (you can see validation comment as it is edited) why is regexp matching /test comment?

@zakisk
Copy link
Contributor

zakisk commented Mar 18, 2025

and also if I send /test command on PR validation error comment doesn't get updated.

@chmouel
Copy link
Member Author

chmouel commented Mar 18, 2025

ah this is weird

@zakisk
Copy link
Contributor

zakisk commented Apr 4, 2025

@chmouel can you please update this?

@chmouel chmouel force-pushed the report-yaml-error-properly-as-comment branch from 9148f7e to 97a1902 Compare April 9, 2025 10:07
@osp-pac osp-pac added the e2e label Apr 9, 2025
@chmouel
Copy link
Member Author

chmouel commented Apr 9, 2025

I don't think i can quite reproduce your issue chmouel/scratchmyback#352 it seems to work for me, can you give me a clear path to reproduce this ? (by recording a video for example)

@zakisk
Copy link
Contributor

zakisk commented Apr 10, 2025

I don't think i can quite reproduce your issue chmouel/scratchmyback#352 it seems to work for me, can you give me a clear path to reproduce this ? (by recording a video for example)

@chmouel in your PR if you see /test toremove comment is replaced by validation comment rather it should have created a new comment, see screenshot:

image

@chmouel chmouel force-pushed the report-yaml-error-properly-as-comment branch from c5da652 to f9cdcd4 Compare May 5, 2025 08:02
@chmouel
Copy link
Member Author

chmouel commented May 5, 2025

/retest

@chmouel chmouel force-pushed the report-yaml-error-properly-as-comment branch from f9cdcd4 to aea95cd Compare May 5, 2025 14:51
@chmouel
Copy link
Member Author

chmouel commented May 5, 2025

This should be fixed (it was just a regexp quote issue)! see chmouel/scratchmyback#358 cc @zakisk

@chmouel chmouel added the feature New feature or request label May 6, 2025
@chmouel
Copy link
Member Author

chmouel commented May 6, 2025

AI:
After analyzing the provided code changes, here's a detailed summary:

Spelling Mistakes & Corrections

  1. Already Corrected:
    • In running.md, the typo in "a lists of usernames" was fixed to "lists of usernames".
    • In match_test.go, tests now reference the correct error messages post-regex parsing adjustments.

Bugs & Potential Issues

  1. Provider-Specific Comment Support:

    • For Bitbucket Cloud/Server and GitLab, the CreateComment method just returns nil without logging or warning. If a user expects comments on PRs for these providers, this could result in silent failures. Suggestion: Add a log warning indicating that comment functionality is not supported for these providers.
  2. Regex Specificity in YAML Parsing:

    • The regex error unmarshalling yaml file\s([^:]*):\s*(yaml:\s*)?(.*) is used to extract error details. While it works for the expected error format, it might fail if the error message varies. For example, if the YAML parser throws unexpected errors not matching this pattern, the original error would be returned but may not be as user-friendly. Consider alternative parsing or a fallback message.

Code Improvements & Idiomatic Go

  1. Error Handling in CreateComment:

    • The provider-specific implementations correctly handle nil clients and non-PR scenarios with descriptive errors (e.g., "no github client has been initialized"). This follows idiomatic Go practices for error handling.
  2. Test Coverage & Design:

    • New tests like TestGithubSecondPullRequestBadYaml and TestGiteaBadYamlReportingOnPR thoroughly validate that comments are created or updated correctly on PRs. The use of golden files ensures consistent output checks.
    • The tests simulate various scenarios (e.g., no existing comment, multiple iterations) to validate update vs. create logic. The regexp.QuoteMeta usage in provider code safeguards against regex injection from user input.
  3. Markdown Formatting:

    • The error message includes a markdown table for clarity. The separator |------|-------| adheres to markdown syntax rules.

Documentations & Notes

  1. Updated Docs:
    • The match.go reference to test/README.md for E2E tests is correct. Ensure the markdown link is functional.
    • The "Errors When Parsing PipelineRun YAML" section in running.md is well-explained with examples.

Edge Cases & Considerations

  • Multiple Error Reports: If multiple errors occur in a PR, the current approach may generate multiple comments. If the intent is to consolidate into a single comment, the code could be adjusted to update an existing comment with all errors instead of appending new ones.
  • Regex Matching: For providers using CreateComment, the regex for searching updateMarker finds the first matching comment. If multiple comments exist, only the first is updated, which might be acceptable if the marker is unique.

Suggestions for Future Work

  1. Add Warnings for Unsupported Features:

    // BitbucketCloud/bitbucket.go
    func (v *Provider) CreateComment(...) error {
        log.Warn("Comment creation is not supported for Bitbucket Cloud")
        return nil
    }
  2. Improve Regex Robustness:
    Consider expanding the regex to handle diverse error messages from the YAML parser. For example:

    reg := regexp.MustCompile(`unmarshalling[^:]*:\s*(.*\.yaml):\s*(.*)`)
  3. Centralize Error Templates:
    The validationErrorTemplate is duplicated in the markdown comment. Consider centralizing this template if used in multiple places across the codebase.


Summary

The code changes are well-structured, address the intended functionality (YAML error reporting via PR comments), and include comprehensive tests. The documentation updates (mostly correcting typos) are appropriate. Potential enhancements focus on improving clarity and robustness for edge cases, particularly around unsupported features in some providers and regex parsing.

model: openrouter:qwen/qwen3-32b:free

@zakisk
Copy link
Contributor

zakisk commented May 6, 2025

@chmouel tested it working fine now

/lgtm

@zakisk
Copy link
Contributor

zakisk commented May 6, 2025

/lgtm

Copy link

@pipelines-as-code pipelines-as-code bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Congrats @chmouel your PR Has been approved 🎉

✅ Pull Request Approved

Approval Status:

  • Required Approvals: 1
  • Current Approvals: 2

👥 Reviewers Who Approved:

Reviewer Permission Level Approval Status
@vdemeester admin
@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
Copy link
Contributor

zakisk commented May 6, 2025

@chmouel tested it working fine now

/lgtm

LGTM Not working with this 😕

@chmouel
Copy link
Member Author

chmouel commented May 6, 2025

🤖 AI Review provided by openrouter:x-ai/grok-3-beta:

After carefully analyzing the changes in the provided git diff, I'll summarize my findings regarding potential bugs, spelling mistakes, and opportunities for code improvement. The changes primarily focus on enhancing error reporting for invalid YAML in PipelineRun templates, adding comments on pull requests when errors are detected, and updating documentation.

Observations and Analysis

1. Documentation Changes

  • File: docs/content/docs/dev/_index.md

    • Improvement: The section on golden files has been updated to provide clearer instructions on regenerating golden files for unit tests and refers readers to ./test/README.md for E2E test updates. This is a good enhancement for clarity.
    • Spelling/Grammar Check: No spelling mistakes found. The text is clear and well-written.
    • Suggestion: Consider adding a direct link to the golden package documentation in addition to the provided URL for easier reference.
  • File: docs/content/docs/guide/running.md

    • Improvement: A new section on "Errors When Parsing PipelineRun YAML" has been added, explaining how errors are logged and reported as comments on pull requests. This enhances user experience by documenting behavior for error handling.
    • Spelling/Grammar Check: Fixed a minor grammar issue in line 85: Changed "allows mapping alias names to lists" to "allows mapping alias names to a list" for better readability and correctness.
    • Suggestion: The image reference report-error-comment-on-bad-yaml.png is added. Ensure the image file is correctly rendered in the documentation.
  • File: test/README.md

    • Improvement: Added a note about using the -update flag to refresh golden files in E2E tests. This is helpful for developers updating test outputs.
    • Spelling/Grammar Check: No issues found.
    • Suggestion: Provide an example command for updating golden files to make it more actionable for developers.

2. Code Changes for Error Reporting

  • File: pkg/pipelineascode/match.go

    • Improvement: Introduced a new function reportValidationErrors to create formatted Markdown comments on pull requests when validation errors occur in PipelineRun YAML files. This enhances user feedback by directly notifying users of errors on the pull request.
    • Improvement: Modified error handling to extract filenames and specific errors using regex when YAML unmarshaling fails, and report them as comments. This is a significant UX improvement over generic error messages.
    • Potential Issue: If the regex pattern error unmarshalling yaml file\s([^:]*):\s*(yaml:\s*)?(.*) fails to match due to changes in error message format, it could silently fallback to returning the original error without reporting. A fallback mechanism to log a generic message might be prudent.
    • Suggestion: Consider logging a warning or error if the regex match fails to ensure visibility into parsing issues.
    • Idiomatic Improvement: The code for creating Markdown tables could be abstracted into a utility function for reusability if this format is used elsewhere.
  • File: pkg/pipelineascode/pipelineascode_test.go and match_test.go

    • Improvement: Added test cases for bad YAML reporting on pull requests, ensuring that comments are posted with expected content. This strengthens test coverage for error handling.
    • Spelling/Grammar Check: Removed outdated log snippet reference to "unmarshalling"; updated to reflect new error messaging.
    • Potential Issue: The regex in expectedPostedComment might be fragile if the error message format changes. Consider making it more flexible or adding multiple acceptable patterns.
    • Suggestion: Use a golden file approach for expected comments in tests to handle variations in error messages more gracefully.

3. Provider Interface and Implementations

  • File: pkg/provider/interface.go

    • Improvement: Added CreateComment method to the Interface to support posting comments on pull requests for various providers (GitHub, Gitea, GitLab, etc.).
    • Spelling/Grammar Check: No issues found.
    • Suggestion: Ensure all provider implementations handle comment updates consistently with the updateMarker logic.
  • Files: pkg/provider/github/github.go, pkg/provider/gitea/gitea.go, pkg/provider/gitlab/gitlab.go, pkg/provider/bitbucketcloud/bitbucket.go, pkg/provider/bitbucketdatacenter/bitbucketdatacenter.go

    • Improvement: Implemented CreateComment for GitHub, Gitea, and GitLab, including logic to update existing comments if an updateMarker is provided. Bitbucket implementations return a nil error (no-op), which is documented as not supporting this feature.
    • Potential Issue: For Bitbucket providers, lack of comment support might confuse users expecting error feedback. A log message or documented limitation could clarify this.
    • Spelling/Grammar Check: No issues found.
    • Suggestion: Consider using a common helper for regex matching across providers to reduce code duplication for comment updates.
    • Idiomatic Improvement: Error messages like "no github client has been initialized" could use a consistent format across providers for better maintainability (e.g., using a template string).
  • Test Files: pkg/provider/github/github_test.go, pkg/provider/gitea/gitea_test.go, pkg/provider/gitlab/gitlab_test.go

    • Improvement: Added comprehensive tests for CreateComment, covering cases like nil clients, non-pull request events, new comment creation, and updating existing comments.
    • Potential Issue: Tests assume HTTP status codes and mock responses. If the API behavior changes, tests might fail. Consider adding more robust error handling in tests.
    • Suggestion: Use a table-driven test approach consistently across providers for easier maintenance.

4. End-to-End Tests

  • File: test/gitea_test.go and test/github_pullrequest_test.go
    • Improvement: Added tests for bad YAML reporting on pull requests, ensuring comments are created/updated correctly and only once.
    • Potential Issue: In TestGithubSecondPullRequestBadYaml, if no comment appears within the loop limit, the test fails without retrying or additional diagnostics. Adding more logging or a retry mechanism could help.
    • Spelling/Grammar Check: Updated test names and comments for clarity (e.g., "Github PullRequest Bad Yaml").
    • Suggestion: Use golden files consistently for expected output in E2E tests to simplify validation.

5. General Observations

  • Consistency: The feature to report YAML errors as comments on pull requests is well-implemented across supported providers (GitHub, Gitea, GitLab), with appropriate no-op for unsupported ones (Bitbucket).
  • Testing: Test coverage is thorough for the new feature, including unit and E2E tests.
  • Documentation: Documentation updates reflect the new feature, improving user awareness of error reporting.

Potential Bugs

  1. Regex Fragility in match.go: If the error message format for YAML parsing changes, the regex might fail to extract filenames and errors, silently falling back to returning the original error without a comment.
  2. No Fallback for Comment Creation Failure: If CreateComment fails, there’s no fallback mechanism to notify users via another channel (e.g., logs or status Checks).
  3. Bitbucket Limitation: Lack of comment support for Bitbucket providers might lead to silent failures from a user perspective since errors won’t be reported on pull requests.

Spelling Mistakes

  • None found. The minor grammar fix in running.md has been noted and corrected in the patch.

Code Improvements for Idiomatic Go

  1. Error Handling: Use structured errors (e.g., errors.Wrap from pkg/errors or fmt.Errorf) for better stack traces and context instead of plain string replacements or direct fmt.Errorf.
  2. Duplication: Abstract common logic for comment updating (regex matching for updateMarker) into a shared utility across providers.
  3. Testing: Use golden files more consistently for expected outputs in both unit and E2E tests to handle variations in output formats.
  4. Logging: Enhance logging for regex failures or comment creation errors to ensure visibility into issues.

Conclusion

The changes introduce a valuable feature for user feedback on YAML errors in PipelineRun templates via pull request comments. The implementation is robust across supported providers with good test coverage. However, there are minor areas of concern regarding regex fragility and unsupported providers (Bitbucket) that could be mitigated with additional logging or fallback mechanisms. Documentation updates are clear and helpful, and code improvements suggested focus on maintainability and idiomatic Go practices.

If you'd like me to elaborate on any specific part or provide a code snippet for suggested improvements (e.g., utility function for Markdown tables or structured error handling), please let me know!

@chmouel
Copy link
Member Author

chmouel commented May 6, 2025

🤖 AI Review provided by gemini:gemini-2.5-pro-exp-03-25:

Okay, let's break down these changes carefully.

Overall Goal of the Change:

The primary goal of this change appears to be enhancing the user experience when Pipelines-as-Code encounters errors while parsing or validating PipelineRun definitions found in the .tekton/ directory of a repository. Instead of just logging errors or failing silently/opaquely, the system will now:

  1. Detect YAML syntax errors during parsing.
  2. Detect Tekton validation errors after successful parsing.
  3. For Pull/Merge Requests on supported platforms (GitHub, Gitea, GitLab), post a comment directly on the PR/MR detailing these errors.
  4. Update the existing error comment on subsequent commits to the PR/MR, rather than posting multiple comments.
  5. Allow other valid PipelineRuns in the same directory/PR to potentially still run, even if some are invalid.

Analysis by File:

  1. docs/content/docs/dev/_index.md:

    • Improvement: The explanation for make update-golden is expanded and clarified. It now correctly states it's for any change affecting golden file output (like CLI output), not just adding commands. Links to gotest.tools/v3/golden.
    • Improvement: Adds a link to test/README.md for E2E golden file updates. Good cross-referencing.
    • Verdict: Good documentation improvement. No issues found.
  2. docs/content/docs/guide/running.md:

    • New Feature Documentation: Adds a clear section "Errors When Parsing PipelineRun YAML".
    • Clarity: Explains that errors are logged, but valid PipelineRuns can still execute.
    • Clarity: Documents the new behavior of posting comments on PRs/MRs for specific providers (using the existing support matrix shortcode).
    • Clarity: Explains the comment update mechanism (updates existing, doesn't delete).
    • New Image: Adds a visual example of the error comment.
    • Minor Wording: "Additionally, OWNERS_ALIASES is supported and allows mapping alias names to a lists of usernames." -> Should be "Additionally, OWNERS_ALIASES is supported and allows mapping alias names to lists of usernames." (Remove the "a").
    • Verdict: Good documentation for the new feature, apart from one minor grammatical error ("a lists").
  3. docs/static/images/report-error-comment-on-bad-yaml.png:

    • New binary file. Assumed correct based on context.
    • Verdict: Necessary asset for documentation.
  4. pkg/pipelineascode/match.go:

    • New Constant: validationErrorTemplate defines the Markdown structure for the error comment. Good practice.
    • Error Handling Change: In getPipelineRunsFromRepo, specific YAML unmarshalling errors (error unmarshalling yaml file...) are now caught only for Pull Request triggers.
    • Error Parsing: A regular expression (regexp.MustCompile) is used to extract the filename and error message from the unmarshalling error string. This is slightly fragile if the error format from the underlying YAML library changes, but pragmatic.
    • New Behavior: If an unmarshalling error matches the regex on a PR, it calls reportValidationErrors and returns nil, nil, effectively swallowing the error for the main flow but reporting it via comment. This allows processing of other potentially valid PipelineRuns in the same commit.
    • Validation Error Reporting: Existing logic for reporting types.ValidationErrors (from tektonyaml.ReadTektonTypes) is moved into the new reportValidationErrors function, also only for PR triggers.
    • New Function reportValidationErrors:
      • Consolidates error reporting (logs + comments).
      • Formats errors into a Markdown table.
      • Calls the new provider interface method p.vcx.CreateComment, crucially passing validationErrorTemplate as the updateMarker. This marker is used by providers to find and update existing comments.
      • Logs an error if comment creation fails but doesn't seem to halt execution.
    • Improvement: Log message clarifies "cannot locate valid templates".
    • Potential Improvement (Minor): The function reportValidationErrors is now used for both YAML syntax errors (unmarshalling) and Tekton validation errors. Renaming it to something like reportPipelineRunDefinitionErrors might be slightly more accurate, but the current name is understandable.
    • Verdict: Core logic for the new feature. Seems correct and implements the desired behavior. The error swallowing for specific parse errors is a key change.
  5. pkg/pipelineascode/match_test.go:

    • Test adjusted (TestGetPipelineRunsFromRepo) to reflect that certain validation errors on PRs might not produce the same log message as before because they are now reported via comments (handled elsewhere/in different tests).
    • Verdict: Test correctly updated to match code changes.
  6. pkg/pipelineascode/pipelineascode_test.go:

    • Test Setup: Removes generic comment mocking, replaced by more specificHandleFunc.
    • New Test Case: Adds pull request/bad-yaml to test the new error reporting path.
    • Test Coverage: Uses expectedPostedComment with a regex assertion to verify the content of the comment created via the mock HTTP handler.
    • Verdict: Good integration test coverage for the error commenting feature.
  7. pkg/provider/*/bitbucket*.go & pkg/test/provider/testwebvcs.go:

    • Interface Compliance: Implements the new CreateComment method as a no-op (return nil).
    • Verdict: Correctly satisfies the interface while indicating the feature is not implemented for these providers.
  8. pkg/provider/gitea/gitea.go & pkg/provider/github/github.go & pkg/provider/gitlab/gitlab.go:

    • New Function: Implements CreateComment.
    • Logic: Correctly checks for initialized client and non-zero PR/MR number.
    • Update Logic: If updateMarker is present, lists recent/relevant comments, searches for the marker using regex, and edits the comment if found.
    • Create Logic: If no marker provided or no match found, creates a new comment.
    • Provider Specifics: Uses the correct API client methods for each platform (listing, editing, creating comments/notes).
    • Potential Improvement (Minor): GitHub provider compiles the regex regexp.MustCompile(regexp.QuoteMeta(updateMarker)) inside the loop. Could be moved outside for minuscule performance gain.
    • Potential Scalability (Gitea): Listing all comments (ListIssueCommentOptions{}) could be slow on very active PRs. This might be a Gitea API limitation.
    • Verdict: Solid implementation of the comment creation/update logic for supported providers.
  9. pkg/provider/*/gitea_test.go & github_test.go & gitlab_test.go (Unit Tests):

    • New Tests: Add comprehensive unit tests for the CreateComment function in each provider.
    • Coverage: Tests cover nil client, non-PR events, creating new comments, updating existing ones, and creating when the marker doesn't match. Uses mock HTTP servers effectively.
    • Verdict: Excellent unit test coverage for the new provider functionality.
  10. pkg/provider/interface.go:

    • Interface Change: Adds the CreateComment method signature.
    • Verdict: Correctly defines the new required provider capability.
  11. test/README.md:

    • Improvement: Adds clear instructions on using go test -update for refreshing E2E golden files.
    • Verdict: Good documentation update for contributors.
  12. test/gitea_test.go (E2E):

    • New Test: TestGiteaBadYamlReportingOnPR specifically tests the comment reporting and update logic using a validation error file (pipeline-validation.yaml). It pushes a second change to verify the comment is updated, not duplicated.
    • Modified Test: TestGiteaBadYaml is renamed TestGiteaBadYamlValidation and now uses a syntax error file (bad-yaml.yaml) and checks the controller log. This focuses on the backend logging aspect for syntax errors.
    • Verdict: Good E2E coverage. The two tests effectively cover reporting validation errors via comments (and updates) and syntax errors via logs (as seen by the controller).
  13. test/github_pullrequest_test.go (E2E):

    • Modified Test: TestGithubSecondPullRequestBadYaml is repurposed. Instead of checking CheckRuns for failure, it now uses a syntax error file (bad-yaml.yaml) and waits for a comment to appear on the PR, asserting its content against a golden file.
    • Verdict: Effectively tests the comment reporting feature for syntax errors on GitHub PRs using golden file comparison.
  14. test/testdata/TestGithubSecondPullRequestBadYaml.golden:

    • New golden file containing the expected Markdown comment for the GitHub syntax error test.
    • Verdict: Necessary test artifact.
  15. test/testdata/failures/pipeline-validation.yaml:

    • New test file containing a PipelineRun with a Tekton validation error (incorrect structure), not a YAML syntax error.
    • Verdict: Necessary test artifact for TestGiteaBadYamlReportingOnPR.

Summary & Verdict:

  • Bug Free: The code appears logically sound and implements the intended feature. No obvious bugs were detected. The error handling change (swallowing specific parse errors on PRs to report via comment) is intentional.
  • Spelling/Docs:
    • Minor grammatical fix needed in docs/content/docs/guide/running.md ("a lists" -> "lists").
    • Documentation is clear and well-updated for the new feature and related testing procedures.
  • Improvements/Idiomatic Code:
    • The code follows idiomatic Go practices.
    • Minor potential improvements noted (function naming in match.go, regex compilation in github.go, potential Gitea scalability) but none are critical issues.
  • Testing: Test coverage (unit, integration, E2E) is good and verifies the new functionality, including the crucial comment update logic.

This is a well-implemented feature enhancement with good documentation and test coverage. The change significantly improves feedback for users encountering malformed or invalid PipelineRun definitions in their repositories.

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 <[email protected]>
@chmouel chmouel force-pushed the report-yaml-error-properly-as-comment branch from eca4596 to 6ef4357 Compare May 6, 2025 08:54
@chmouel
Copy link
Member Author

chmouel commented May 6, 2025

the e2e error is a failure which i fixed in 6ef4357 we need to merge it tho to get the e2e fixed so merging this now...

i mean this commit actually 🤦🏻 ac7ef5d

- Corrected the script path from `./hack/gh-workflow-ci` to
`./hack/gh-workflow-ci.sh` for the log output step.
- Ensured the log output step always runs using `if: always()` to aid
  debugging failures.

Signed-off-by: Chmouel Boudjnah <[email protected]>
@chmouel chmouel force-pushed the report-yaml-error-properly-as-comment branch from 6ef4357 to ac7ef5d Compare May 6, 2025 08:56
@chmouel chmouel merged commit 03788d6 into openshift-pipelines:main May 6, 2025
3 of 7 checks passed
@chmouel chmouel deleted the report-yaml-error-properly-as-comment branch May 6, 2025 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

e2e feature New feature or request

Development

Successfully merging this pull request may close these issues.

5 participants