feat(base-cluster): migrate loki to grafana-community helm chart#2118
feat(base-cluster): migrate loki to grafana-community helm chart#2118
Conversation
📝 WalkthroughWalkthroughThe Loki Helm chart dependency is migrated from the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Code Review
This pull request migrates the Loki helm chart from the Grafana repository to the Grafana Community repository, updating the version to 13.2.3 and adjusting configuration values like deploymentMode and imageRegistry. Feedback indicates that the deploymentMode value should be lowercase 'monolithic' to ensure compatibility with the new chart's requirements.
There was a problem hiding this comment.
Pull request overview
Migrates the Loki HelmRelease in base-cluster from the grafana Helm repository to the grafana-community Helm repository, aligning chart sourcing and repository creation conditions with when Loki is enabled.
Changes:
- Move Loki chart version pinning from
global.helmRepositories.grafanatoglobal.helmRepositories.grafana-community. - Update the Loki HelmRelease to reference the
grafana-communityrepository and adjust chart values for the new chart. - Broaden the
grafana-communityHelmRepository creation condition to cover Loki-enabled clusters (not only tracing-enabled ones).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| charts/base-cluster/values.yaml | Moves Loki chart pin to grafana-community and updates repo creation condition to include Loki. |
| charts/base-cluster/templates/monitoring/logs/loki.yaml | Points Loki HelmRelease at grafana-community/loki and updates chart values for the migrated chart. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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 `@charts/base-cluster/values.yaml`:
- Around line 128-133: Update the PR description / chart CHANGELOG to call out
the large cross-major-version bump for the grafana-community:loki chart (from
6.55.0 → 13.2.3 as referenced under the grafana-community.charts.loki entry),
explicitly note the Kubernetes 1.25+ and Helm 3.x requirements, mention the
default deploymentMode change (now Monolithic) and confirm we explicitly set
deploymentMode in our loki values, and advise operators to back up existing Loki
PVCs before upgrading; include these points succinctly in the PR/changelog so
downstream consumers of the loki chart and maintainers reviewing the
grafana-community block and its condition are aware.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3150d67-3a85-49df-b823-1f6beffbf101
📒 Files selected for processing (2)
charts/base-cluster/templates/monitoring/logs/loki.yamlcharts/base-cluster/values.yaml
|
Is tested that the volume remains? Just asking because it's not marked breaking |
Summary by CodeRabbit