feat: Install telemetry CRDs via crd-installer - #312
Open
jthakkar04 wants to merge 3 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThe change vendors VictoriaMetrics and Grafana CRDs, embeds them in the operator, and installs them through the upgrade-safe CRD installer. Helm CRD installation is disabled for these operators. Tests cover rendering, parsing, composition, and annotations. ChangesTelemetry CRD installation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Makefile
participant VendoredCRDs
participant EmbeddedCRDs
participant CRDInstaller
participant HelmChart
Makefile->>VendoredCRDs: Extract and synchronize CRD manifests
VendoredCRDs->>EmbeddedCRDs: Copy Grafana and VictoriaMetrics CRDs
HelmChart->>CRDInstaller: Render telemetry CRD groups
EmbeddedCRDs->>CRDInstaller: Provide embedded CRD filesystems
CRDInstaller->>CRDInstaller: Compose telemetry CRDs without operator annotations
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/vendored/grafana-operator/crds/README.md (1)
30-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the upstream license file next to the vendored manifests. The README states the manifests retain the upstream Apache 2.0 license, but the directory contains no license copy. Add the upstream
LICENSEfile to this directory so the attribution is self-contained.🤖 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 `@pkg/vendored/grafana-operator/crds/README.md` around lines 30 - 33, Add the upstream Apache 2.0 LICENSE file alongside the vendored manifests in the directory described by the README, preserving its original license text and attribution so the stated licensing is self-contained.internal/controller/reconciler/telemetry_chart_test.go (1)
148-148: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider asserting each group independently.
The assertion matches the full flag string, so it also depends on redis and clickhouse being enabled by default and on the exact group order. A future default change to an unrelated subchart then breaks this test for a reason outside its intent. Assert that the rendered
--groups=flag containsvictoriametricsandgrafana, so the test stays focused on telemetry.🤖 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 `@internal/controller/reconciler/telemetry_chart_test.go` at line 148, Update the assertion around the rendered --groups= flag in the telemetry chart test to verify victoriametrics and grafana independently, rather than matching the complete group list and its order. Keep the test focused on telemetry groups without asserting unrelated default groups such as redis or clickhouse.
🤖 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.
Inline comments:
In `@pkg/vendored/grafana-operator/crds/README.md`:
- Around line 8-11: Update the grafana-operator source documentation in the
README and the pinned chart reference in deploy/operator/Chart.yaml to use the
official OCI registry source, or explicitly document why this tree intentionally
uses the HTTP Helm repository and why it differs from upstream’s OCI
distribution.
---
Nitpick comments:
In `@internal/controller/reconciler/telemetry_chart_test.go`:
- Line 148: Update the assertion around the rendered --groups= flag in the
telemetry chart test to verify victoriametrics and grafana independently, rather
than matching the complete group list and its order. Keep the test focused on
telemetry groups without asserting unrelated default groups such as redis or
clickhouse.
In `@pkg/vendored/grafana-operator/crds/README.md`:
- Around line 30-33: Add the upstream Apache 2.0 LICENSE file alongside the
vendored manifests in the directory described by the README, preserving its
original license text and attribution so the stated licensing is self-contained.
🪄 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 Plus
Run ID: 759270e0-e19f-452a-9cad-6286d52cb700
📒 Files selected for processing (35)
MakefileTiltfiledeploy/operator/templates/_helpers.tpldeploy/operator/values.yamlinternal/controller/reconciler/telemetry_chart_test.gointernal/crdinstaller/compose_test.gointernal/crdinstaller/crds/grafana/grafana.integreatly.org_grafanaalertrulegroups.yamlinternal/crdinstaller/crds/grafana/grafana.integreatly.org_grafanacontactpoints.yamlinternal/crdinstaller/crds/grafana/grafana.integreatly.org_grafanadashboards.yamlinternal/crdinstaller/crds/grafana/grafana.integreatly.org_grafanadatasources.yamlinternal/crdinstaller/crds/grafana/grafana.integreatly.org_grafanafolders.yamlinternal/crdinstaller/crds/grafana/grafana.integreatly.org_grafanalibrarypanels.yamlinternal/crdinstaller/crds/grafana/grafana.integreatly.org_grafanamutetimings.yamlinternal/crdinstaller/crds/grafana/grafana.integreatly.org_grafananotificationpolicies.yamlinternal/crdinstaller/crds/grafana/grafana.integreatly.org_grafananotificationpolicyroutes.yamlinternal/crdinstaller/crds/grafana/grafana.integreatly.org_grafananotificationtemplates.yamlinternal/crdinstaller/crds/grafana/grafana.integreatly.org_grafanas.yamlinternal/crdinstaller/crds/grafana/grafana.integreatly.org_grafanaserviceaccounts.yamlinternal/crdinstaller/crds/victoriametrics/vm_crds.yamlinternal/crdinstaller/embed.gopkg/vendored/grafana-operator/crds/README.mdpkg/vendored/grafana-operator/crds/grafana.integreatly.org_grafanaalertrulegroups.yamlpkg/vendored/grafana-operator/crds/grafana.integreatly.org_grafanacontactpoints.yamlpkg/vendored/grafana-operator/crds/grafana.integreatly.org_grafanadashboards.yamlpkg/vendored/grafana-operator/crds/grafana.integreatly.org_grafanadatasources.yamlpkg/vendored/grafana-operator/crds/grafana.integreatly.org_grafanafolders.yamlpkg/vendored/grafana-operator/crds/grafana.integreatly.org_grafanalibrarypanels.yamlpkg/vendored/grafana-operator/crds/grafana.integreatly.org_grafanamutetimings.yamlpkg/vendored/grafana-operator/crds/grafana.integreatly.org_grafananotificationpolicies.yamlpkg/vendored/grafana-operator/crds/grafana.integreatly.org_grafananotificationpolicyroutes.yamlpkg/vendored/grafana-operator/crds/grafana.integreatly.org_grafananotificationtemplates.yamlpkg/vendored/grafana-operator/crds/grafana.integreatly.org_grafanas.yamlpkg/vendored/grafana-operator/crds/grafana.integreatly.org_grafanaserviceaccounts.yamlpkg/vendored/victoria-metrics-operator/crds/README.mdpkg/vendored/victoria-metrics-operator/crds/vm_crds.yaml
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jthakkar04
force-pushed
the
jthakkar/install-telemetry-crds
branch
from
August 3, 2026 18:48
28228f1 to
997bfcc
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
Makefile (1)
53-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit the oversized target recipes.
checkmakereportsgenerate-vendoredandsync-crd-embedabove the configured body-length limit. Move extraction and synchronization into helper targets or a script. Keep these targets as small orchestration targets.Also applies to: 74-82
🤖 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 `@Makefile` around lines 53 - 71, Split the oversized recipes for generate-vendored and sync-crd-embed into helper targets or a script that performs the extraction and synchronization commands. Keep both named targets as short orchestration targets invoking those helpers, while preserving their existing outputs and behavior.Source: Linters/SAST tools
🤖 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.
Inline comments:
In `@internal/controller/reconciler/telemetry_chart_test.go`:
- Line 158: Update the comment above crdInstallerGroups to explain why it must
scan the rendered Helm manifest, focusing on the non-obvious validation purpose;
otherwise remove the comment rather than restating the helper’s behavior.
- Around line 149-151: Update the group validation loop in the telemetry chart
test to split the comma-separated groups value, trim each token, and verify
exact membership for “victoriametrics” and “grafana” instead of using
strings.Contains on the raw string.
In `@Makefile`:
- Around line 65-70: The Makefile CRD generation recipes must replace existing
artifacts only after every extraction or copy succeeds. At Makefile lines 65-70,
stage VictoriaMetrics and Grafana outputs in a temporary location, validate both
results, then atomically replace the vendored CRD directories; at lines 75-81,
stage all source copies first and replace internal/crdinstaller/crds only after
every copy completes successfully.
---
Nitpick comments:
In `@Makefile`:
- Around line 53-71: Split the oversized recipes for generate-vendored and
sync-crd-embed into helper targets or a script that performs the extraction and
synchronization commands. Keep both named targets as short orchestration targets
invoking those helpers, while preserving their existing outputs and behavior.
🪄 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 Plus
Run ID: 361b6ec2-3c09-464f-80e4-c13dd67dd53b
📒 Files selected for processing (35)
MakefileTiltfiledeploy/operator/templates/_helpers.tpldeploy/operator/values.yamlinternal/controller/reconciler/telemetry_chart_test.gointernal/crdinstaller/compose_test.gointernal/crdinstaller/crds/grafana/grafana.integreatly.org_grafanaalertrulegroups.yamlinternal/crdinstaller/crds/grafana/grafana.integreatly.org_grafanacontactpoints.yamlinternal/crdinstaller/crds/grafana/grafana.integreatly.org_grafanadashboards.yamlinternal/crdinstaller/crds/grafana/grafana.integreatly.org_grafanadatasources.yamlinternal/crdinstaller/crds/grafana/grafana.integreatly.org_grafanafolders.yamlinternal/crdinstaller/crds/grafana/grafana.integreatly.org_grafanalibrarypanels.yamlinternal/crdinstaller/crds/grafana/grafana.integreatly.org_grafanamutetimings.yamlinternal/crdinstaller/crds/grafana/grafana.integreatly.org_grafananotificationpolicies.yamlinternal/crdinstaller/crds/grafana/grafana.integreatly.org_grafananotificationpolicyroutes.yamlinternal/crdinstaller/crds/grafana/grafana.integreatly.org_grafananotificationtemplates.yamlinternal/crdinstaller/crds/grafana/grafana.integreatly.org_grafanas.yamlinternal/crdinstaller/crds/grafana/grafana.integreatly.org_grafanaserviceaccounts.yamlinternal/crdinstaller/crds/victoriametrics/vm_crds.yamlinternal/crdinstaller/embed.gopkg/vendored/grafana-operator/crds/README.mdpkg/vendored/grafana-operator/crds/grafana.integreatly.org_grafanaalertrulegroups.yamlpkg/vendored/grafana-operator/crds/grafana.integreatly.org_grafanacontactpoints.yamlpkg/vendored/grafana-operator/crds/grafana.integreatly.org_grafanadashboards.yamlpkg/vendored/grafana-operator/crds/grafana.integreatly.org_grafanadatasources.yamlpkg/vendored/grafana-operator/crds/grafana.integreatly.org_grafanafolders.yamlpkg/vendored/grafana-operator/crds/grafana.integreatly.org_grafanalibrarypanels.yamlpkg/vendored/grafana-operator/crds/grafana.integreatly.org_grafanamutetimings.yamlpkg/vendored/grafana-operator/crds/grafana.integreatly.org_grafananotificationpolicies.yamlpkg/vendored/grafana-operator/crds/grafana.integreatly.org_grafananotificationpolicyroutes.yamlpkg/vendored/grafana-operator/crds/grafana.integreatly.org_grafananotificationtemplates.yamlpkg/vendored/grafana-operator/crds/grafana.integreatly.org_grafanas.yamlpkg/vendored/grafana-operator/crds/grafana.integreatly.org_grafanaserviceaccounts.yamlpkg/vendored/victoria-metrics-operator/crds/README.mdpkg/vendored/victoria-metrics-operator/crds/vm_crds.yaml
🚧 Files skipped from review as they are similar to previous changes (30)
- deploy/operator/values.yaml
- internal/crdinstaller/crds/grafana/grafana.integreatly.org_grafanaalertrulegroups.yaml
- internal/crdinstaller/compose_test.go
- internal/crdinstaller/crds/grafana/grafana.integreatly.org_grafanadatasources.yaml
- Tiltfile
- internal/crdinstaller/crds/grafana/grafana.integreatly.org_grafananotificationtemplates.yaml
- deploy/operator/templates/_helpers.tpl
- internal/crdinstaller/crds/grafana/grafana.integreatly.org_grafananotificationpolicyroutes.yaml
- pkg/vendored/grafana-operator/crds/grafana.integreatly.org_grafananotificationtemplates.yaml
- pkg/vendored/grafana-operator/crds/grafana.integreatly.org_grafananotificationpolicyroutes.yaml
- pkg/vendored/grafana-operator/crds/README.md
- pkg/vendored/grafana-operator/crds/grafana.integreatly.org_grafanafolders.yaml
- internal/crdinstaller/crds/grafana/grafana.integreatly.org_grafanadashboards.yaml
- internal/crdinstaller/crds/grafana/grafana.integreatly.org_grafanamutetimings.yaml
- pkg/vendored/victoria-metrics-operator/crds/README.md
- internal/crdinstaller/embed.go
- internal/crdinstaller/crds/grafana/grafana.integreatly.org_grafanacontactpoints.yaml
- internal/crdinstaller/crds/grafana/grafana.integreatly.org_grafananotificationpolicies.yaml
- pkg/vendored/grafana-operator/crds/grafana.integreatly.org_grafanacontactpoints.yaml
- pkg/vendored/grafana-operator/crds/grafana.integreatly.org_grafananotificationpolicies.yaml
- internal/crdinstaller/crds/grafana/grafana.integreatly.org_grafanalibrarypanels.yaml
- pkg/vendored/grafana-operator/crds/grafana.integreatly.org_grafanalibrarypanels.yaml
- pkg/vendored/grafana-operator/crds/grafana.integreatly.org_grafanadatasources.yaml
- pkg/vendored/grafana-operator/crds/grafana.integreatly.org_grafanamutetimings.yaml
- internal/crdinstaller/crds/grafana/grafana.integreatly.org_grafanas.yaml
- pkg/vendored/grafana-operator/crds/grafana.integreatly.org_grafanadashboards.yaml
- internal/crdinstaller/crds/victoriametrics/vm_crds.yaml
- pkg/vendored/grafana-operator/crds/grafana.integreatly.org_grafanaalertrulegroups.yaml
- internal/crdinstaller/crds/grafana/grafana.integreatly.org_grafanafolders.yaml
- pkg/vendored/grafana-operator/crds/grafana.integreatly.org_grafanas.yaml
Comment on lines
+149
to
+151
| for _, g := range []string{"victoriametrics", "grafana"} { | ||
| if !strings.Contains(groups, g) { | ||
| t.Errorf("crd-installer --groups=%q missing %q", groups, g) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Match complete group names instead of substrings.
strings.Contains(groups, g) accepts malformed values such as victoriametrics-extra or grafana-extra. The test can pass without an exact --groups token. Split and trim the comma-separated value, then check exact membership.
Proposed assertion fix
- groups := crdInstallerGroups(t, output)
+ groupValue := crdInstallerGroups(t, output)
+ groups := make(map[string]struct{})
+ for _, group := range strings.Split(groupValue, ",") {
+ groups[strings.TrimSpace(group)] = struct{}{}
+ }
for _, g := range []string{"victoriametrics", "grafana"} {
- if !strings.Contains(groups, g) {
- t.Errorf("crd-installer --groups=%q missing %q", groups, g)
+ if _, ok := groups[g]; !ok {
+ t.Errorf("crd-installer --groups=%q missing exact group %q", groupValue, g)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for _, g := range []string{"victoriametrics", "grafana"} { | |
| if !strings.Contains(groups, g) { | |
| t.Errorf("crd-installer --groups=%q missing %q", groups, g) | |
| groupValue := crdInstallerGroups(t, output) | |
| groups := make(map[string]struct{}) | |
| for _, group := range strings.Split(groupValue, ",") { | |
| groups[strings.TrimSpace(group)] = struct{}{} | |
| } | |
| for _, g := range []string{"victoriametrics", "grafana"} { | |
| if _, ok := groups[g]; !ok { | |
| t.Errorf("crd-installer --groups=%q missing exact group %q", groupValue, g) | |
| } | |
| } |
🤖 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 `@internal/controller/reconciler/telemetry_chart_test.go` around lines 149 -
151, Update the group validation loop in the telemetry chart test to split the
comma-separated groups value, trim each token, and verify exact membership for
“victoriametrics” and “grafana” instead of using strings.Contains on the raw
string.
| } | ||
| } | ||
|
|
||
| // crdInstallerGroups returns the value of the crd-installer Job's --groups flag. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Explain why the helper scans rendered output.
The comment only restates crdInstallerGroups behavior. Rewrite it to explain the non-obvious reason for scanning the rendered Helm manifest, or remove it.
Proposed comment update
-// crdInstallerGroups returns the value of the crd-installer Job's --groups flag.
+// Scan rendered YAML so the test verifies the Helm hook's command arguments.As per coding guidelines, **/*.{go,py,sh,bash}: Do not add inline comments that merely restate what code does; comments should concisely explain why, especially business logic or non-obvious behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // crdInstallerGroups returns the value of the crd-installer Job's --groups flag. | |
| // Scan rendered YAML so the test verifies the Helm hook's command arguments. |
🤖 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 `@internal/controller/reconciler/telemetry_chart_test.go` at line 158, Update
the comment above crdInstallerGroups to explain why it must scan the rendered
Helm manifest, focusing on the non-obvious validation purpose; otherwise remove
the comment rather than restating the helper’s behavior.
Source: Coding guidelines
Comment on lines
+65
to
+70
| @mkdir -p pkg/vendored/victoria-metrics-operator/crds pkg/vendored/grafana-operator/crds | ||
| @tar -xzOf deploy/operator/charts/victoria-metrics-operator-*.tgz victoria-metrics-operator/crd.yaml \ | ||
| > pkg/vendored/victoria-metrics-operator/crds/vm_crds.yaml | ||
| @rm -f pkg/vendored/grafana-operator/crds/*.yaml | ||
| @tar -xzf deploy/operator/charts/grafana-operator-*.tgz -C pkg/vendored/grafana-operator/crds \ | ||
| --strip-components=3 grafana-operator/files/crds |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make CRD replacement all-or-nothing.
Both recipes remove or overwrite existing CRD artifacts before all reads and copies succeed. A failed tar or cp can leave generated inputs incomplete.
- Makefile#L65-L70: stage VictoriaMetrics and Grafana extraction, validate the results, then replace vendored files.
- Makefile#L75-L81: stage all source copies, then replace
internal/crdinstaller/crdsafter every copy succeeds.
📍 Affects 1 file
Makefile#L65-L70(this comment)Makefile#L75-L81
🤖 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 `@Makefile` around lines 65 - 70, The Makefile CRD generation recipes must
replace existing artifacts only after every extraction or copy succeeds. At
Makefile lines 65-70, stage VictoriaMetrics and Grafana outputs in a temporary
location, validate both results, then atomically replace the vendored CRD
directories; at lines 75-81, stage all source copies first and replace
internal/crdinstaller/crds only after every copy completes successfully.
| {{- if (dig "altinity-clickhouse-operator" "enabled" false .Values.AsMap) -}} | ||
| {{- $groups = append $groups "clickhouse" -}} | ||
| {{- end -}} | ||
| {{- /* Telemetry CRDs install in every mode so an off→full upgrade never has to add CRDs. */ -}} |
Contributor
There was a problem hiding this comment.
we should only install this when they are needed, but the difference is that the crd-installer will install them during a helm upgrade if they are enabled, which the helm charts were not doing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In this PR we enabled installing the telemetry CRDs (VictoriaMetrics + Grafana) across
helm upgrades by routing them through the existingcrd-installerJob — which server-side-applies embedded CRDs on both install and upgrade — instead of shipping them inside the mode-gated operator subcharts. They now install in every telemetry mode, so switchingoff → forward/fullbecomes a clean controller/CR toggle.We chose this because Helm never installs CRDs on
upgrade(only on a freshinstall), so a cluster first installed with telemetryoffcould never be upgraded tofull. Reusing the crd-installer — the same mechanism already used for redis/clickhouse — keeps CRD lifecycle consistent and decoupled from the feature toggle.Testing
Tilt up with
off:tilt trigger wandb-operator🤖 Generated with Claude Code
Summary by CodeRabbit
Summary by CodeRabbit