notify: log a warning when a template fails to render#5388
notify: log a warning when a template fails to render#5388mihir-dixit2k27 wants to merge 4 commits into
Conversation
Signed-off-by: Mihir Dixit <dixitmihir1@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughNotification template rendering now uses logger-aware text and HTML helpers across notifier integrations. The helpers log execution failures and propagate errors through existing error handling. Tests cover logged failures and initialize PagerDuty rendering dependencies. ChangesLogger-aware notification template rendering
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@notify/slack/slack.go`:
- Line 68: In Notify, check err immediately after the final TmplTextWithLogger
call and before JSON encoding or Slack delivery; return false and err when
rendering fails, while preserving the existing request flow when err is nil.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d248b26a-6ea7-4a19-9873-1da633f0e7f7
📒 Files selected for processing (19)
notify/discord/discord.gonotify/email/email.gonotify/jira/jira.gonotify/mattermost/mattermost.gonotify/msteams/msteams.gonotify/msteamsv2/msteamsv2.gonotify/opsgenie/opsgenie.gonotify/pagerduty/pagerduty.gonotify/pagerduty/pagerduty_test.gonotify/pushover/pushover.gonotify/rocketchat/rocketchat.gonotify/slack/slack.gonotify/sns/sns.gonotify/telegram/telegram.gonotify/util.gonotify/victorops/victorops.gonotify/webex/webex.gonotify/webhook/webhook.gonotify/wechat/wechat.go
| var ( | ||
| data = notify.GetTemplateData(ctx, n.tmpl, as, logger) | ||
| tmplText = notify.TmplText(n.tmpl, data, &err) | ||
| tmplText = notify.TmplTextWithLogger(n.tmpl, data, &err, logger) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Return template errors before sending the Slack request.
TmplTextWithLogger records failures in err, but Notify never checks err after rendering the request fields. Add an if err != nil { return false, err } check after the final template call and before JSON encoding/network delivery; otherwise a failure short-circuits subsequent fields to empty strings while the malformed request is still sent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@notify/slack/slack.go` at line 68, In Notify, check err immediately after the
final TmplTextWithLogger call and before JSON encoding or Slack delivery; return
false and err when rendering fails, while preserving the existing request flow
when err is nil.
There was a problem hiding this comment.
Looks like a pre-existing issue, but would be nice to adress @mihir-dixit2k27
TheMeier
left a comment
TheMeier
left a comment
There was a problem hiding this comment.
Maybe a test for TmplTextWithLogger that checks that a log is actually emitted could be added.
Signed-off-by: Mihir Dixit <dixitmihir1@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@notify/util_test.go`:
- Line 294: Update the warning assertions in the affected tests around the
existing require.Contains checks to validate the complete payload in both text
and HTML log outputs. Assert that each output includes the template name and the
rendering error details in addition to the generic “template execution failed”
message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e2d7c38c-c2d8-4be9-b07c-a86251b2b9ca
📒 Files selected for processing (1)
notify/util_test.go
Signed-off-by: Mihir Dixit <dixitmihir1@gmail.com>
2 participants
Title:
notify: log a warning when a template fails to renderWhich user-facing changes does this PR introduce?
When a template fails, the error string gets embedded into the rendered output and sent to the handler. Depending on the handler, this can result in a rejected API call with no log entry pointing to a template failure as the root cause (see #5322 for a real Slack example).
This adds
TmplTextWithLogger/TmplHTMLWithLoggerinnotify/util.goand switches all notifiers to use them. On any template error aWarnlog line is emitted with the template name and error before the notifier continues. Delivery behaviour is unchanged, per #3490.Pull Request Checklist