Skip to content

fix handle multiline trigger comment var properly #1934

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

chmouel
Copy link
Member

@chmouel chmouel commented Feb 14, 2025

Submitter Checklist

  • fix handle multiline trigger_comment var properly
    When using {{ trigger_comment }} in a PipelineRun YAML, GitHub
    comments containing newlines (e.g., /help) can break YAML parsing due
    to improper expansion.

    This change replaces \r\n with \n in trigger_comment, similar to
    how we did for only \n previously are handled. This ensures valid YAML
    formatting and avoids parser errors.

    Fixes trigger_comment may screw pipelinerun yaml with the the newline #1929

  • 📝 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 ✅️

Go 1.22 introduced a restriction where type assertions in multiple
assignments must be standalone.

This fix ensures compliance by:
- Separating type assertions before passing values to functions.
- Adding error handling for failed assertions to prevent runtime panics.
- Consolidating duplicate cases for cleaner logic.

This resolves golangci-lint 'forcetypeassert' errors.

Fixes openshift-pipelines#1932

Signed-off-by: Chmouel Boudjnah <[email protected]>
@zakisk
Copy link
Contributor

zakisk commented Feb 14, 2025

I guess its because of PR changes?

{"level":"info","ts":1739527147.9725325,"caller":"wait/wait.go:64","msg":"Still waiting for repository status to be updated: 3/3"}
    logs.go:91: assertion failed: 
        --- expected
        +++ actual
        @@ -2,5 +2,5 @@
         /hello-world\n\n
         The event is on-comment
        -The revision is 23354021b71abb8c2cc20eec6a84f408d339d4db
        +The revision is 37428d2e1e59fe52c01551a8c4381a9ec11cc82c
         The custom1 value is foo
         The custom2 value is bar
        
        
        You can run 'go test . -update' to automatically update testdata/TestGiteaOnCommentAnnotation-pipelinerun-on-comment-annotation.golden to the new expected value.'

When using `{{ trigger_comment }}` in a PipelineRun YAML, GitHub
comments containing newlines (e.g., `/help`) can break YAML parsing due
to improper expansion.

This change replaces `\r\n` with `\n` in `trigger_comment`, similar to
how `pull_requests_labels` are handled. This ensures valid YAML
formatting and avoids parser errors.

Fixes openshift-pipelines#1929

Signed-off-by: Chmouel Boudjnah <[email protected]>
@chmouel chmouel force-pushed the fix-handle-multiline-trigger-comment-var-properly branch from 90d5cdb to 66e5059 Compare February 14, 2025 10:25
@chmouel
Copy link
Member Author

chmouel commented Feb 14, 2025

@zakisk fixed and green, PTAL 🙏🏻

@zakisk
Copy link
Contributor

zakisk commented Feb 14, 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.

✅ Pull Request Approved

Approval Status:

  • Required Approvals: 1
  • Current Approvals: 1

👥 Approved By:

Reviewer Permission Status
@zakisk admin

📝 Next Steps

  • All required checks must pass
  • Branch protection rules apply
  • Get a maintainer to use the /merge command to merge the PR

Thank you for your contributions! 🎉

@zakisk
Copy link
Contributor

zakisk commented Feb 14, 2025

/merge

@pipelines-as-code pipelines-as-code bot merged commit 0390c98 into openshift-pipelines:main Feb 14, 2025
7 checks passed
Copy link

✅ PR Successfully Merged

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

Approvals Summary:

Reviewer Permission Status
@zakisk admin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

trigger_comment may screw pipelinerun yaml with the the newline
2 participants