fix(config): fix regression in config merge - #5457
Conversation
|
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)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthrough
ChangesNotifier configuration merge
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This PR corrects configuration merging for several notifier integrations and includes regression tests; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
SoloJacobs
left a comment
There was a problem hiding this comment.
Can we restrict this change to the lines, which are actually problematic? :-)
| } | ||
| ogc.HTTPConfig = cmp.Or(ogc.HTTPConfig, c.Global.HTTPConfig) | ||
| ogc.APIURL = cmp.Or(ogc.APIURL, c.Global.OpsGenieAPIURL) | ||
| if ogc.HTTPConfig == nil { |
There was a problem hiding this comment.
This seems like a noop? I like cmp.Or.
There was a problem hiding this comment.
yes, the revert is really not necessary for c.Global.HTTPConfig. Changed
3cb39d5 to
a9c0eac
Compare
Fixes: prometheus#5456 Signed-off-by: Christoph Maser <christoph.maser+github@gmail.com>
a9c0eac to
7b5c1b6
Compare
Fixes: #5456
Pull Request Checklist
Please check all the applicable boxes.
Which user-facing changes does this PR introduce?