-
Notifications
You must be signed in to change notification settings - Fork 1
chore: add kafka config options #25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughChart version bumped to 0.10.0. Kafka configuration changed from a flat Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@charts/ctrlplane/values.yaml`:
- Around line 62-65: The chart introduced a breaking key rename so existing
overrides of global.kafkaBrokers will be ignored; update the chart to detect and
handle the old key by adding a validation or compatibility layer: add a
templated check (e.g., in templates/_helpers.tpl or a new
templates/validation.yaml) that fails install/upgrade with a clear error if
global.kafkaBrokers is set, or populate kafka.brokers from global.kafkaBrokers
when present to preserve backward compatibility, and document the change in the
README/CHANGELOG; reference the values keys global.kafkaBrokers and
kafka.brokers and the values.yaml defaults when implementing this.
| kafka: | ||
| brokers: localhost:9092 | ||
| groupId: workspace-engine | ||
| topic: workspace-events |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Breaking change: existing overrides of global.kafkaBrokers will silently stop working.
Users who have global.kafkaBrokers in their custom values.yaml or --set flags will not get an error — the old key will simply be ignored, and the new defaults will apply. Consider documenting this migration in the chart's changelog or README, and optionally adding a validation template that fails with a helpful message if the old key is still set.
🤖 Prompt for AI Agents
In `@charts/ctrlplane/values.yaml` around lines 62 - 65, The chart introduced a
breaking key rename so existing overrides of global.kafkaBrokers will be
ignored; update the chart to detect and handle the old key by adding a
validation or compatibility layer: add a templated check (e.g., in
templates/_helpers.tpl or a new templates/validation.yaml) that fails
install/upgrade with a clear error if global.kafkaBrokers is set, or populate
kafka.brokers from global.kafkaBrokers when present to preserve backward
compatibility, and document the change in the README/CHANGELOG; reference the
values keys global.kafkaBrokers and kafka.brokers and the values.yaml defaults
when implementing this.
Summary by CodeRabbit
Chores
Configuration
Documentation