incident.io notifier: add metadata field with templating support - #5022
incident.io notifier: add metadata field with templating support#5022leonore wants to merge 3 commits into
Conversation
202f665 to
642f435
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds optional incident.io metadata configuration. Values are rendered with alert data and included in outgoing messages, with omission when unset and non-retryable errors for invalid templates. ChangesIncident.io metadata field
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Notifier
participant TemplateEngine
participant IncidentioAPI
Caller->>Notifier: Notify(alert data)
Notifier->>TemplateEngine: Render metadata templates
TemplateEngine-->>Notifier: Rendered metadata or error
Notifier->>IncidentioAPI: Send message with metadata
Notifier-->>Caller: Return notification result
Estimated code review effort: 2 (Simple) | ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
e3ef305 to
c957a79
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
notify/incidentio/incidentio.go (1)
168-182: Optional: early-exit on template error.Because
notify.TmplTextshort-circuits after the first error, all subsequenttmpl(v)calls return empty strings that still get written into themetadatamap. Since the function returns an error anyway the map is discarded, so this is purely cosmetic — but you can make intent clearer and skip wasted work by checkingtmplErrinside the loop or returning immediately after the error check. Current behavior is safe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notify/incidentio/incidentio.go` around lines 168 - 182, The metadata-templating code uses notify.TmplText which sets tmplErr on first error but still calls tmpl(v) for every entry; change the logic in the metadata block (variables tmplErr, tmpl, metadata, n.conf.Metadata, notify.TmplText) to check tmplErr immediately before iterating (or check inside the loop and return on error) so you stop calling tmpl(v) after an error — i.e., if tmplErr != nil return the error (or skip/map only after confirming tmplErr==nil) to avoid populating metadata with empty values after a template failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@notify/incidentio/incidentio_test.go`:
- Line 498: The tests are failing to compile because they construct URL values
with the nonexistent type config.URL; replace each occurrence (e.g., the
instances constructing URL for IncidentioConfig.URL) with the defined type
&amcommoncfg.URL{URL: u} so IncidentioConfig.URL receives a *amcommoncfg.URL;
update the four places that use &config.URL{URL: u} to &amcommoncfg.URL{URL: u}
and ensure the amcommoncfg import alias is present where the test references
IncidentioConfig and its URL field.
---
Nitpick comments:
In `@notify/incidentio/incidentio.go`:
- Around line 168-182: The metadata-templating code uses notify.TmplText which
sets tmplErr on first error but still calls tmpl(v) for every entry; change the
logic in the metadata block (variables tmplErr, tmpl, metadata, n.conf.Metadata,
notify.TmplText) to check tmplErr immediately before iterating (or check inside
the loop and return on error) so you stop calling tmpl(v) after an error — i.e.,
if tmplErr != nil return the error (or skip/map only after confirming
tmplErr==nil) to avoid populating metadata with empty values after a template
failure.
🪄 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: Pro Plus
Run ID: b2f4a9aa-7cc4-43d8-b445-3e284781603d
📒 Files selected for processing (4)
config/notifiers.godocs/configuration.mdnotify/incidentio/incidentio.gonotify/incidentio/incidentio_test.go
Allow users to send custom key-value metadata with alerts, with full
Go template rendering support (e.g. {{ .CommonLabels.severity }}).
Signed-off-by: leo <leo@incident.io>
Signed-off-by: Edd Sowden <edd.sowden@incident.io>
d117913 to
99fdc28
Compare
|
👋 Hey, we'd love to get this improvement merged, and a bunch of our customers would appreciate it too. @siavashs / @Spaceman1701 any chance either of you have time to take a look at this? 🙏 |
Check the template error inside the render loop so we return as soon as a metadata value fails to render, rather than continuing to call the template function for the remaining entries after it has already errored. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Isaac Seymour <i.seymour@oxon.org>
|
Hi @isaacseymour , LGTM overall. Needs a rebase could you say a word on what your use case is? How do your customers benefit? Phrase it from the user perspective, don't let the AI dictate (it's very bad knowing these kinds of things). Kind regards |
Allow users to send custom key-value metadata with alerts, with full Go template rendering support (e.g. {{ .CommonLabels.severity }}). This mirrors the approach to the Opsgenie
Detailsattribute.Pull Request Checklist
Please check all the applicable boxes.
benchstatto compare benchmarksWhich user-facing changes does this PR introduce?