Conversation
dkorittki
requested review from
Noroth,
SkArchon,
StarpTech,
devsergiy,
endigma and
jensneuse
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughReplaces nested NATS consumers configuration with a single top-level boolean Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
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 |
Router image scan passed✅ No security vulnerabilities found in image: |
Router image scan passed✅ No security vulnerabilities found in image: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2628 +/- ##
===========================================
+ Coverage 38.38% 62.79% +24.41%
===========================================
Files 969 244 -725
Lines 125253 25818 -99435
Branches 5419 0 -5419
===========================================
- Hits 48077 16213 -31864
+ Misses 75490 8223 -67267
+ Partials 1686 1382 -304
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
router/pkg/config/config.schema.json (1)
2584-2596:⚠️ Potential issue | 🟠 MajorKeep
consumersas a deprecated alias instead of hard-renaming the key.Line 2584 changes the public config key, and
LoadConfigvalidates user YAML against this schema before unmarshal (router/pkg/config/config.goLine 1278 and Line 1311). That means any existing config still usingconsumers:now fails startup even though the behavior behind the option is unchanged. If the goal is only to flag the setting as experimental, keepconsumersworking for at least one release and deprecate it while introducingexperiment_consumers—and make sure the Go YAML mapping accepts both names during the transition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/config/config.schema.json` around lines 2584 - 2596, The schema change renamed the public config key to "experiment_consumers" which breaks existing configs using "consumers"; restore a deprecated alias and accept both names during config load: add a "consumers" property in the JSON schema that mirrors "experiment_consumers" (same object schema and a description noting it's deprecated) so validation accepts old configs, and update the Go config loading/unmarshal logic in LoadConfig (and the Config struct field handling ExperimentConsumers/Consumers) to accept both keys—if only "consumers" is present, copy its value into the ExperimentConsumers field and emit a deprecation warning; ensure this mapping happens before the rest of validation so old configs continue to work for one release.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@router/pkg/config/config.schema.json`:
- Around line 2584-2596: The schema change renamed the public config key to
"experiment_consumers" which breaks existing configs using "consumers"; restore
a deprecated alias and accept both names during config load: add a "consumers"
property in the JSON schema that mirrors "experiment_consumers" (same object
schema and a description noting it's deprecated) so validation accepts old
configs, and update the Go config loading/unmarshal logic in LoadConfig (and the
Config struct field handling ExperimentConsumers/Consumers) to accept both
keys—if only "consumers" is present, copy its value into the ExperimentConsumers
field and emit a deprecation warning; ensure this mapping happens before the
rest of validation so old configs continue to work for one release.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 66727b2e-27d0-44ea-afc8-930429ee87e0
📒 Files selected for processing (2)
router/pkg/config/config.gorouter/pkg/config/config.schema.json
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@router/pkg/config/config.go`:
- Around line 620-623: Remove the env tag from the
DeleteDurableConsumersOnShutdown field in the struct that defines NATS provider
entries (the field named DeleteDurableConsumersOnShutdown of type bool, adjacent
to ID, URL and Authentication/NatsAuthentication) so it no longer has
env:"EXPERIMENT_DELETE_DURABLE_CONSUMERS_ON_SHUTDOWN" envDefault:"false"; keep
the YAML tag intact and ensure only the yaml tag remains
(yaml:"experiment_delete_durable_consumers_on_shutdown") to match other array
element structs and avoid a global env variable applying to individual slice
elements.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e1331a6-1637-464e-a26a-6aec471d8269
📒 Files selected for processing (2)
router/pkg/config/config.gorouter/pkg/config/config.schema.json
endigma
left a comment
endigma
left a comment
There was a problem hiding this comment.
approved once the env tag is removed
…ion-experimental' into dominik/eng-9159-make-config-option-experimental
4 participants
Summary by CodeRabbit
Configuration
Tests
Documentation
Checklist
We shortly added a configuration option meant to be experimental but hasn't been flagged so. This pr changes it be making the whole
consumersoption category experimental. Given that this option has only been added today I think we are safe when renaming it.