🐛 OCPBUGS-95281: add HelmChartSupport to Boxcutter backend#2797
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
Adds Helm chart bundle detection/rendering to the Boxcutter revision generation path so HelmChartSupport actually affects behavior when BoxcutterRuntime=true, enabling ClusterExtension installs from OCI Helm charts (including CRDs and hook/template filtering).
Changes:
- Gate Helm chart detection in
SimpleRevisionGenerator.GenerateRevision()and render templates via Helm’s engine when a chart bundle is detected. - Extend e2e catalog-building/test plumbing to support packaging a chart directory into a
.tgz“bundle” and add an e2e scenario + minimal chart testdata. - Add unit coverage for chart-path rendering, filtering, and feature-gate behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
internal/operator-controller/applier/boxcutter.go |
Detect chart bundles under HelmChartSupport, render templates/CRDs, filter hooks/non-manifests, and build a ClusterObjectSet from rendered objects. |
internal/operator-controller/applier/boxcutter_test.go |
Add unit tests covering Helm chart rendering path, feature-gate behavior, filtering, and values handling. |
test/internal/catalog/bundle.go |
Add support for packaging a Helm chart directory into a .tgz bundle artifact for e2e catalog usage. |
test/internal/catalog/catalog.go |
Avoid applying registryv1 bundle OCI labels for Helm-chart bundles in test catalog image creation. |
test/e2e/steps/steps.go |
Allow specifying a chart column in catalog tables to package a chart into a bundle for e2e runs. |
test/e2e/features/install.feature |
Add an e2e scenario that installs a Helm chart from a catalog under Boxcutter runtime. |
test/e2e/steps/testdata/charts/helm-test/Chart.yaml |
Minimal Helm chart metadata for the new e2e scenario. |
test/e2e/steps/testdata/charts/helm-test/templates/configmap.yaml |
Minimal templated manifest validating .Release / .Chart values during rendering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| headerCols := make(map[string]int) | ||
| for i, cell := range table.Rows[0].Cells { | ||
| headerCols[strings.ToLower(cell.Value)] = i | ||
| } | ||
| cellVal := func(rowIdx int, name string) string { | ||
| if idx, ok := headerCols[name]; ok && idx < len(table.Rows[rowIdx].Cells) { | ||
| return table.Rows[rowIdx].Cells[idx].Value | ||
| } | ||
| return "" | ||
| } |
There was a problem hiding this comment.
Acknowledged — the header-based parsing replaces the previous positional parsing which also had no validation. Adding required-header validation is a good idea but out of scope for this bug fix.
pedjak
force-pushed
the
ocpbugs-95281-helmchart-boxcutter
branch
from
654ca51 to
ac66840
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
pedjak
requested review from
perdasilva and
tmshort
and removed request for
oceanc80
| if features.OperatorControllerFeatureGate.Enabled(features.HelmChartSupport) { | ||
| meta := new(chart.Metadata) | ||
| if ok, _ := imageutil.IsBundleSourceChart(bundleFS, meta); ok { | ||
| return r.generateRevisionFromChart(ctx, bundleFS, ext, meta, objectLabels, revisionAnnotations) | ||
| } | ||
| } |
There was a problem hiding this comment.
The ok, _ := pattern matches the Helm backend (helm.go:253) — it's a consistent design choice, not a new regression. IsBundleSourceChart is lightweight (one ReadDir + one ReadFile), so the cost on non-chart bundles is minimal.
When BoxcutterRuntime=true (default in experimental install), installing helm charts via ClusterExtension fails because SimpleRevisionGenerator.GenerateRevision() always delegates to RegistryV1ManifestProvider, which expects metadata/annotations.yaml. Add helm chart detection and rendering to GenerateRevision(), gated behind the HelmChartSupport feature gate. When a helm chart bundle is detected via IsBundleSourceChart(), render templates using Helm's engine.Render() (pure template rendering, no cluster access), include CRDs from the crds/ directory, filter out non-manifest files (.tpl, NOTES.txt) by extension and helm hooks by annotation, and detect install vs upgrade from the extension status. Co-Authored-By: Claude <noreply@anthropic.com>
pedjak
force-pushed
the
ocpbugs-95281-helmchart-boxcutter
branch
from
ac66840 to
ba6b647
Compare
|
Seems to me like a heavy maintenance burden to take on this code in the boxcutter path, especially when the path forward on the next bundle format is not clear and may not even be helm. I propose we solve this bug by crashing the program when both feature gates are enabled and documenting their mutual exclusivity. I'd also be amenable to fully removing the code related to helm support since I think we'd want to revisit it again in earnest with a broader design (catalog implications, pipeline implications, lifecycling implications with boxcutter, etc.) |
Description
When
BoxcutterRuntime=true(default in experimental install), theHelmChartSupportfeature gate is accepted but has no effect. Installing a helm chart from an OCI registry via a ClusterExtension fails withopen metadata/annotations.yaml: no such file or directorybecause the Boxcutter backend always assumes registryv1 bundle format.This PR adds helm chart detection and rendering to
SimpleRevisionGenerator.GenerateRevision(), gated behind the existingHelmChartSupportfeature gate:IsBundleSourceChart()before falling back to the registryv1 pathengine.Render()(pure template rendering, no cluster access needed)crds/directory.Release.IsInstall/.Release.IsUpgradetemplate valuesvalues.yamldefaults for template renderingReviewer Checklist