feat(config): support global receiver_templates.pagerduty.details overrides#5104
feat(config): support global receiver_templates.pagerduty.details overrides#5104mihir-dixit2k27 wants to merge 5 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a global per-receiver-type template override facility that merges ChangesCore implementation
Notifier types & validation
Tests
Documentation
Sequence DiagramsequenceDiagram
participant User
participant ConfigParser as Config Parser
participant GlobalTemplates as Global Receiver Templates
participant MergeLogic as Merge Helper Functions
participant ReceiverConfigs as Receiver Configurations
User->>ConfigParser: Load YAML config (may include global.receiver_templates)
ConfigParser->>GlobalTemplates: Parse & default GlobalConfig.ReceiverTemplates
ConfigParser->>ReceiverConfigs: Parse receiver definitions
loop for each receiver and channel
MergeLogic->>GlobalTemplates: Get global template for channel/field
MergeLogic->>ReceiverConfigs: Inspect receiver field value
alt Receiver field explicitly set (including "")
MergeLogic->>ReceiverConfigs: Preserve receiver value
else Receiver field unset
alt Global template present
MergeLogic->>ReceiverConfigs: Apply global template value
else
MergeLogic->>ReceiverConfigs: Use built-in default
end
end
alt PagerDuty details merging
MergeLogic->>GlobalTemplates: Iterate global.details keys
MergeLogic->>ReceiverConfigs: Apply per-key rules (skip global empty-string, preserve explicit receiver "", override built-in defaults only for standard keys, merge custom keys)
end
end
MergeLogic->>ConfigParser: Return merged receivers
ConfigParser->>User: Return final validated config
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
mihir-dixit2k27
force-pushed
the
feat/pagerduty-global-details
branch
from
5165941 to
00594fd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/notifiers.go (1)
466-467:⚠️ Potential issue | 🟠 MajorDon't make PagerDuty receiver
detailsstring-only.
PagerdutyConfig.Detailsis still amap[string]any, and existing receiver configs support nested Event API v2 payloads. This new unmarshal check will now reject previously valid values like nested maps or numeric constants, even though the global override feature only needs string comparisons for the built-in keys it merges. Keep that type handling local to the merge path instead of tightening the whole receiver schema.Also applies to: 514-517
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/notifiers.go` around lines 466 - 467, The Pagerduty receiver schema should not be narrowed to string-only; restore PagerdutyConfig.Details as map[string]any and remove any new strict unmarshal/type-check that rejects nested maps or numeric values (the change that affects Details at PagerdutyConfig.Details and the similar check around lines 514-517). Instead, perform any string-only comparisons or conversions locally in the merge/override logic where the global override merges built-in keys (handle casting/formatting there), leaving the receiver struct flexible to accept nested Event API v2 payloads.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/config.go`:
- Around line 211-223: The applyEmailTemplates function currently uses
applyGlobalTemplateOverride for ec.Text (and similarly for opsgenie.note
elsewhere), which relies on a string-equality helper that cannot distinguish an
omitted field from an explicit empty string; change this to use presence
tracking instead of plain string comparison: update the EmailConfig/Text (and
OpsGenie note) handling so the unmarshalling records whether the field was
provided (e.g., change the struct to use *string or add a boolean flag like
TextSet/NoteSet) and then modify applyEmailTemplates and the other call sites
(the applyGlobalTemplateOverride invocations for HTML/Text and the opsgenie.note
override locations) to check that presence flag before applying the global
template, leaving explicit "" values untouched while still applying defaults
when absent.
- Around line 309-313: applyWeChatTemplates currently never applies
GlobalWeChatTemplates.agent_id and fails to treat an unset message_type
correctly because it compares against DefaultWechatConfig.MessageType instead of
the normalized default used by WechatConfig.UnmarshalYAML; modify
applyWeChatTemplates to (1) merge gt.AgentID into wc.AgentID (or call the same
helper used for other fields) so agent_id is not ignored, and (2) detect an
unset/normalized default message type (match the effective default used by
WechatConfig.UnmarshalYAML, e.g., treat wc.MessageType == "" or the normalized
"text" as unset) before applying gt.MessageType so global message_type actually
applies to receivers that left it unset.
- Around line 127-191: The loops that call
applySlackTemplates/applyEmailTemplates/applyPagerDutyTemplates/applyOpsGenieTemplates/applyVictorOpsTemplates/applyPushoverTemplates/applyWeChatTemplates/applyTelegramTemplates
currently assume each receiver config element is non-nil and can panic; update
each inner loop to skip nil entries (e.g., check r.SlackConfigs[j] == nil and
continue) before calling the corresponding apply* function so nil slice entries
are guarded against while still preserving existing gt handling.
---
Outside diff comments:
In `@config/notifiers.go`:
- Around line 466-467: The Pagerduty receiver schema should not be narrowed to
string-only; restore PagerdutyConfig.Details as map[string]any and remove any
new strict unmarshal/type-check that rejects nested maps or numeric values (the
change that affects Details at PagerdutyConfig.Details and the similar check
around lines 514-517). Instead, perform any string-only comparisons or
conversions locally in the merge/override logic where the global override merges
built-in keys (handle casting/formatting there), leaving the receiver struct
flexible to accept nested Event API v2 payloads.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b9d8f61a-29e8-4209-8878-acc8fe8377e1
📒 Files selected for processing (5)
config/config.goconfig/config_test.goconfig/notifiers.goconfig/testdata/global_receiver_templates.ymldocs/configuration.md
mihir-dixit2k27
force-pushed
the
feat/pagerduty-global-details
branch
from
00594fd to
64cb5f0
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
config/config.go (2)
211-223:⚠️ Potential issue | 🟠 Major
applyGlobalTemplateOverridebreaks explicit""on empty-default fields.The helper explicitly assumes
defaultVal != "", but Lines 222 and 289 use it foremail.textandopsgenie.note, whose built-in defaults are empty. That makes “omitted” and""indistinguishable, so receivertext: ""/note: ""will be overwritten by the global template instead of taking precedence. This needs presence tracking in the config structs (*stringor...Setflags), plus regression coverage for both cases.Also applies to: 284-290, 322-331
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/config.go` around lines 211 - 223, The helper applyGlobalTemplateOverride currently treats empty default values as absent, causing explicit empty fields (e.g., EmailConfig.Text or OpsGenie note) to be overwritten by global templates; change the config structs (e.g., EmailConfig.HTML/Text and the opsgenie note field) to track presence (use *string or add a boolean ...Set flag), update applyGlobalTemplateOverride to check presence rather than string emptiness before applying gt values, adjust callers such as applyEmailTemplates to pass/inspect the presence-aware fields, and add regression tests covering both omitted (nil/unset) and explicitly empty ("") cases so explicit empty values take precedence over global overrides.
127-191:⚠️ Potential issue | 🔴 CriticalGuard nil receiver entries before this pre-merge pass.
*_configs: -still yields nil slice elements. Because this runs before the later receiver-validation loops, a matchingglobal.receiver_templatesblock can now dereference a nil config and panic during load instead of returning the existingmissing ... configerror path. Please skip nil entries before calling eachapply*Templateshelper, and add a regression case for that shape.Suggested guard pattern
for j := range r.SlackConfigs { + if r.SlackConfigs[j] == nil { + continue + } var slackGt *GlobalSlackTemplates if gt != nil { slackGt = gt.Slack } applySlackTemplates(r.SlackConfigs[j], slackGt) }Apply the same guard to the email, PagerDuty, OpsGenie, VictorOps, Pushover, WeChat, and Telegram loops.
Also applies to: 570-571
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/config.go` around lines 127 - 191, The receiver template pre-merge loop must guard against nil slice elements before calling each apply*Templates helper: for each receivers loop (iterate receivers -> r := &receivers[i]) check if the specific config entry (e.g., r.SlackConfigs[j], r.EmailConfigs[j], r.PagerdutyConfigs[j], r.OpsGenieConfigs[j], r.VictorOpsConfigs[j], r.PushoverConfigs[j], r.WechatConfigs[j], r.TelegramConfigs[j]) is nil and skip it instead of passing it to applySlackTemplates, applyEmailTemplates, applyPagerDutyTemplates, applyOpsGenieTemplates, applyVictorOpsTemplates, applyPushoverTemplates, applyWeChatTemplates, applyTelegramTemplates; update those loops to continue on nil entries and add a regression unit test that constructs receivers with nil slice elements (e.g., *_configs: -) to verify the loader returns the existing validation error path rather than panicking.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@config/config.go`:
- Around line 211-223: The helper applyGlobalTemplateOverride currently treats
empty default values as absent, causing explicit empty fields (e.g.,
EmailConfig.Text or OpsGenie note) to be overwritten by global templates; change
the config structs (e.g., EmailConfig.HTML/Text and the opsgenie note field) to
track presence (use *string or add a boolean ...Set flag), update
applyGlobalTemplateOverride to check presence rather than string emptiness
before applying gt values, adjust callers such as applyEmailTemplates to
pass/inspect the presence-aware fields, and add regression tests covering both
omitted (nil/unset) and explicitly empty ("") cases so explicit empty values
take precedence over global overrides.
- Around line 127-191: The receiver template pre-merge loop must guard against
nil slice elements before calling each apply*Templates helper: for each
receivers loop (iterate receivers -> r := &receivers[i]) check if the specific
config entry (e.g., r.SlackConfigs[j], r.EmailConfigs[j], r.PagerdutyConfigs[j],
r.OpsGenieConfigs[j], r.VictorOpsConfigs[j], r.PushoverConfigs[j],
r.WechatConfigs[j], r.TelegramConfigs[j]) is nil and skip it instead of passing
it to applySlackTemplates, applyEmailTemplates, applyPagerDutyTemplates,
applyOpsGenieTemplates, applyVictorOpsTemplates, applyPushoverTemplates,
applyWeChatTemplates, applyTelegramTemplates; update those loops to continue on
nil entries and add a regression unit test that constructs receivers with nil
slice elements (e.g., *_configs: -) to verify the loader returns the existing
validation error path rather than panicking.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7dced856-9088-4baa-8c9d-a117d7ce104f
📒 Files selected for processing (5)
config/config.goconfig/config_test.goconfig/notifiers.goconfig/testdata/global_receiver_templates.ymldocs/configuration.md
✅ Files skipped from review due to trivial changes (1)
- config/testdata/global_receiver_templates.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- config/notifiers.go
mihir-dixit2k27
force-pushed
the
feat/pagerduty-global-details
branch
from
64cb5f0 to
e3ba561
Compare
Signed-off-by: Mihir Dixit <dixitmihir1@gmail.com>
mihir-dixit2k27
force-pushed
the
feat/pagerduty-global-details
branch
from
e3ba561 to
ec60a2d
Compare
Spaceman1701
left a comment
Spaceman1701
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
I really like this idea! However, I'm a little concerned about the need to keep a separate set of structs for each integration that kinda duplicate the set of template fields in each integration struct.
I wonder if we could just include a "default" version of every integration in the global config... We already have a lot of global versions of each integrations fields, so maybe it'd save some duplication in the end.
I'm interested to see what the other maintainers think as well - I might be alone in being worried about the extra duplication 😄
|
Hi! We discussed this today in the Alertmanager bug scrub, and the general consensus is that we like the idea, but the amount of struct duplication is a little problematic. @TheMeier is going to try playing with some changes which might let us include entire default versions of the integrations in the alertmanager/config/notifiers.go Lines 55 to 60 in 0779ba3 Unfortunately, because of the custom |
|
@Spaceman1701 Thanks for the update! Makes sense, happy to wait and see what @TheMeier comes up with and adjust from there. |
|
So I came up empty. I thought something like this TheMeier@5138f15 One thought I had before trying this out: In many cases yaml anchors/refrences can be leveraged to avoid duplications. Eg this works: But it doesn't allow to modify existing keys from the refrence ATM because |
|
Thanks for trying it out! |
Well, just a thought. Both solutions rely on conventions (that $someone defines the defaults either in the global section or the yaml-anchor is defined). However in case the convention is not fulfilled with the global config the configuration will/might validate but it will be wrong. In case of referencing a non-existent yaml-anchor the config does not even validate. |
…hods Refactor the package-level applySlackTemplates, applyEmailTemplates, applyPagerDutyTemplates, applyOpsGenieTemplates, applyVictorOpsTemplates, applyPushoverTemplates, applyWeChatTemplates and applyTelegramTemplates functions into methods on their respective config types, as suggested by TheMeier in the PR review. Each method is placed directly after its type's UnmarshalYAML to follow the existing codebase convention. The logic is unchanged. Signed-off-by: Mihir Dixit <dixitmihir1@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
config/notifiers.go (1)
452-452: 💤 Low valueConsider using consistent receiver names.
The static analyzer flags that new
applyTemplatesmethods use descriptive receiver names (ec,pc,sc, etc.) while existing methods on the same types usec. For consistency within the file, consider usingcfor all receiver names.🤖 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 `@config/notifiers.go` at line 452, Receiver names are inconsistent: the new method func (ec *EmailConfig) applyTemplates uses "ec" while other methods on the same types in this file use "c"; rename the receiver to "c" in applyTemplates (and any other new methods using "ec", "pc", "sc") so the method signature becomes func (c *EmailConfig) applyTemplates and align all same-type methods to use "c" for consistency with existing methods in the file (update references inside the method to use the new receiver name).
🤖 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.
Nitpick comments:
In `@config/notifiers.go`:
- Line 452: Receiver names are inconsistent: the new method func (ec
*EmailConfig) applyTemplates uses "ec" while other methods on the same types in
this file use "c"; rename the receiver to "c" in applyTemplates (and any other
new methods using "ec", "pc", "sc") so the method signature becomes func (c
*EmailConfig) applyTemplates and align all same-type methods to use "c" for
consistency with existing methods in the file (update references inside the
method to use the new receiver name).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 00a71e34-a081-4dad-b7e0-ff883db0d805
📒 Files selected for processing (2)
config/config.goconfig/notifiers.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/notifiers.go (1)
454-461:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep PagerDuty
sourcealigned with a globalclientoverride.
UnmarshalYAMLderivessourcefrom the pre-mergeclient. If a receiver omitssourceand this helper later appliesgt.Client,Sourcestays on the old built-in template instead of following the new client value, so the config no longer preserves the existing “source defaults to client” behavior.Suggested fix
func (pc *PagerdutyConfig) applyTemplates(gt *GlobalPagerDutyTemplates) { if gt == nil { return } + oldClient := pc.Client applyGlobalTemplateOverride(&pc.Description, gt.Description, DefaultPagerdutyConfig.Description) applyGlobalTemplateOverride(&pc.Client, gt.Client, DefaultPagerdutyConfig.Client) applyGlobalTemplateOverride(&pc.ClientURL, gt.ClientURL, DefaultPagerdutyConfig.ClientURL) + if pc.Source == oldClient { + pc.Source = pc.Client + } // Merge global template overrides into the receiver `details` map.🤖 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 `@config/notifiers.go` around lines 454 - 461, The applyTemplates method for PagerdutyConfig must keep Source aligned with a global client override: after applying gt.Client to pc.Client (via applyGlobalTemplateOverride), check pc.Source and if it is unset or still equal to the original default-derived value (DefaultPagerdutyConfig.Source or the value that UnmarshalYAML would have produced), update pc.Source to match the new client (use gt.Client or DefaultPagerdutyConfig.Client as appropriate). Concretely, in PagerdutyConfig.applyTemplates, either call applyGlobalTemplateOverride on pc.Source using gt.Client/DefaultPagerdutyConfig.Client or add a short conditional after applyGlobalTemplateOverride(&pc.Client, ...) that sets pc.Source = pc.Client when pc.Source is empty or still the default, ensuring UnmarshalYAML’s “source defaults to client” behavior is preserved when gt.Client overrides occur.
🤖 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.
Outside diff comments:
In `@config/notifiers.go`:
- Around line 454-461: The applyTemplates method for PagerdutyConfig must keep
Source aligned with a global client override: after applying gt.Client to
pc.Client (via applyGlobalTemplateOverride), check pc.Source and if it is unset
or still equal to the original default-derived value
(DefaultPagerdutyConfig.Source or the value that UnmarshalYAML would have
produced), update pc.Source to match the new client (use gt.Client or
DefaultPagerdutyConfig.Client as appropriate). Concretely, in
PagerdutyConfig.applyTemplates, either call applyGlobalTemplateOverride on
pc.Source using gt.Client/DefaultPagerdutyConfig.Client or add a short
conditional after applyGlobalTemplateOverride(&pc.Client, ...) that sets
pc.Source = pc.Client when pc.Source is empty or still the default, ensuring
UnmarshalYAML’s “source defaults to client” behavior is preserved when gt.Client
overrides occur.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c1e051df-f616-4ebb-aa67-2c1e91f03259
📒 Files selected for processing (3)
config/config.goconfig/notifiers.godocs/configuration.md
✅ Files skipped from review due to trivial changes (1)
- docs/configuration.md
mihir-dixit2k27
force-pushed
the
feat/pagerduty-global-details
branch
from
30b14dd to
f575ca0
Compare
Signed-off-by: mihir-dixit2k27 <143348248+mihir-dixit2k27@users.noreply.github.com>
…hods Refactor the package-level applySlackTemplates, applyEmailTemplates, applyPagerDutyTemplates, applyOpsGenieTemplates, applyVictorOpsTemplates, applyPushoverTemplates, applyWeChatTemplates and applyTelegramTemplates functions into methods on their respective config types, as suggested by TheMeier in the PR review. Each method is placed directly after its type's UnmarshalYAML to follow the existing codebase convention. The logic is unchanged. Also restore missing DefaultIncidentioConfig, DefaultWebhookConfig and DefaultMSTeamsConfig vars that were dropped during the upstream merge but are still referenced by the UnmarshalYAML methods in this file. Signed-off-by: Mihir Dixit <dixitmihir1@gmail.com>
mihir-dixit2k27
force-pushed
the
feat/pagerduty-global-details
branch
from
e1af5a6 to
85adfe9
Compare
I'm not sure if it's a good idea, but we could decouple type IntegrationConfig[SELF any] interface {
InheritDefaults(a SELF)
Validate(c *Config) error
}
type ExampleIntegration struct {
...
}
func (e *ExampleIntegration) InheritDefaults(a *ExampleIntegration) {
// apply defaults to fields that are unset
}
func (e *ExampleIntegration) Validate(c *Config) error {
// check for invalid data given the context of the entire config
}and this would be loaded somewhat like: var config Config
yaml.Unmarshal(&config)
config.Global.DefaultExampleIntegration.InheritDefaults(hardCodedDefault)
for _, exampleIntegration := range allExampleIntegrations(config) {
exampleIntegration.InheritDefaults(config.Global.DefaultExampleIntegration)
reportError(exampleIntegration.Validate(config))
}I'm know that's very sketchy pseudocode, but hopefully it makes a little sense. And again, not sure if this is a good idea / worth it, but I think it would work. |
3 participants
Closes #5002
The built-in PagerDuty
detailsdefaults usetoJson, which JSON-ifies alert payloads and loses annotation URLs and generator info readability. There was no global template to override this — users had to repeatdetailsoverrides in every PagerDuty receiver individually.This extends
global.receiver_templates.pagerdutyto support adetailsmap, using the same 3-tier precedence as all other receiver template fields:Merge semantics:
firing,resolved,num_firing,num_resolved): global override applies only when the receiver value still equals the built-in default. Explicitfiring: ''is preserved.environment,priority, etc.): global value is injected only when the receiver omits the key entirely. Existing per-receiver values are never clobbered.Changes:
config/notifiers.go: addDetails map[string]stringtoGlobalPagerDutyTemplatesconfig/config.go: extendapplyPagerDutyTemplates()with details merge logic and nil guardsconfig/testdata/global_receiver_templates.yml: add PD fixtures covering absent/override/empty-string/custom-key scenariosconfig/config_test.go: 5 subtests covering all merge paths and backward compatibilitydocs/configuration.md: documentdetailsmap, precedence rules, and unrendered Go template string semanticsgo test ./...acceptance failures are pre-existing — require builtalertmanager/amtoolbinaries unrelated to this change.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests