fix: silently skip edited comment events instead of logging errors#2657
fix: silently skip edited comment events instead of logging errors#2657majiayu000 wants to merge 3 commits intotektoncd:mainfrom
Conversation
Edited or deleted issue comments on GitHub trigger a webhook that reaches processEvent(), which returned an error for any non-"created" action. That error propagated up to sinker and was logged as Errorf, producing scary log entries for a normal, harmless event. Replace the error return with a debug log and nil,nil return so these events are silently skipped, matching the existing skip pattern used elsewhere in the file. Signed-off-by: majiayu000 <1835304752@qq.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies the GitHub provider to skip issue comment events that are not 'created' instead of returning an error. The logic in processEvent now logs a debug message and returns nil for non-created actions, and the test suite has been updated to reflect this change. I have no feedback to provide.
| }, | ||
| { | ||
| name: "bad/issue_comment_not_from_created", | ||
| wantErrString: "only newly created comment is supported, received: deleted", |
There was a problem hiding this comment.
we need to test that the debug log has issued, not just skip it!
|
/label ok-to-test |
|
/ok-to-test |
|
✅ Added labels: ok-to-test. |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2657 +/- ##
=======================================
Coverage 58.80% 58.80%
=======================================
Files 206 206
Lines 20304 20305 +1
=======================================
+ Hits 11940 11941 +1
Misses 7591 7591
Partials 773 773 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: majiayu000 <1835304752@qq.com>
|
@chmouel Done. Switched the test logger to |
|
@majiayu000: PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
4 participants
Description of the Change
Fixes #2652
When GitHub sends a webhook for an edited/deleted issue comment,
processEvent()was returning an error ("only newly created comment is supported, received: edited"), which propagated up tosinker.processEventPayload()and got logged ass.logger.Errorf("failed to parse event: %v", err). This produced scary-looking error logs for a perfectly normal event.Changed the error return to a
v.Logger.Debugf()log +return nil, nil, which is the established skip pattern already used at lines 190, 309, 419, and 834 of the same file. Updated the corresponding test case to expect a silent skip instead of an error.Linked GitHub Issue
Fixes #2652
Testing Strategy
Updated existing test case
bad/issue_comment_not_from_created→good/issue_comment_not_from_created_skipto expect nil return instead of error.make testandmake lintpass locally.AI Assistance
Submitter Checklist
fix:,feat:) matches the "Type of Change" I selected above.make testandmake lintlocally to check for and fix any issues. For an efficient workflow, I have considered installing pre-commit and runningpre-commit installto automate these checks.