⚠️ OPRUN-4075: Move to a helm-based configuration #2145
Conversation
|
[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.
Needs 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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2145 +/- ##
==========================================
+ Coverage 72.75% 72.76% +0.01%
==========================================
Files 79 79
Lines 7340 7340
==========================================
+ Hits 5340 5341 +1
Misses 1653 1653
+ Partials 347 346 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -53,6 +53,12 @@ $(GORELEASER): $(BINGO_DIR)/goreleaser.mod | |||
| @echo "(re)installing $(GOBIN)/goreleaser-v1.26.2" | |||
There was a problem hiding this comment.
Helm charts usually follow a clear folder structure under /templates to group related files.
Putting everything in one place makes it harder to read, review, and maintain.
So, wdyt? Let’s split these into logical subfolders so it’s easier to find and manage resources.
Following a proposed structure would like
olmv1/
Chart.yaml
values.yaml
README.md
templates/
_helpers.tpl
namespace.yaml
# CRDs (templated so Helm upgrades them); pick standard vs experimental via values
crds/
clustercatalogs.crd.yaml
clusterextensions.crd.yaml
rbac/
clusterroles.yaml
clusterrolebindings.yaml
roles.yaml
rolebindings.yaml
serviceaccounts/
catalogd.yaml
operator-controller.yaml
# Per-component (stable)
catalogd/
deployments/controller-manager.yaml
services/controller-manager.yaml
networkpolicies/{default-deny.yaml,controller-manager.yaml}
webhooks/mutating.yaml
certificates/{issuer.yaml,serving-cert.yaml}
monitoring/servicemonitor.yaml
configmaps/trusted-ca.yaml
operator-controller/
deployments/controller-manager.yaml
services/controller-manager.yaml
networkpolicies/{default-deny.yaml,controller-manager.yaml}
webhooks/mutating.yaml
certificates/{issuer.yaml,serving-cert.yaml}
monitoring/servicemonitor.yaml
configmaps/registries-conf.yaml
# E2E (optional)
e2e/{pvc.yaml,copy-pod.yaml}
# Feature-gated area (per-feature, per-component)
features/
experimental/
crds/ # only render when features.experimental.enabled
catalogd/experimental/{deployments,services,rbac,...}
operatorController/experimental/{deployments,services,rbac,...}
e2e/experimental/ # only if both e2e + experimental are enabledThen, the values could be something like
options:
openshift:
enabled: false
certManager:
enabled: true
e2e:
enabled: false
features:
experimental:
enabled: false # global (e.g., experimental CRDs)
catalogd:
experimental:
enabled: false
featureGates: [] # e.g., ["APIV1MetaHandler"]
operatorController:
experimental:
enabled: false
featureGates: [] # e.g., ["WebhookProviderManager","HelmChartSupport"]
e2e:
experimental: false # requires options.e2e.enabled
In the templates we would need add helpers like
# {{- if .Values.features.experimental.enabled }} # for templates/features/experimental/**
# {{- if .Values.features.catalogd.experimental.enabled }} # per component
Maybe the calls could be something like:
HELM ?= helm
CHART_NAME ?= olmv1
CHART_DIR ?= helm/olmv1
OUT_DIR ?= manifests
CERT_VALUES ?= helm/cert-manager.yaml
E2E_VALUES ?= helm/e2e.yaml
EXP_VALUES ?= helm/experimental.yaml
OPENSHIFT_VALS ?= helm/openshift.yaml
STANDARD_MANIFEST ?= $(OUT_DIR)/standard.yaml
STANDARD_E2E_MANIFEST ?= $(OUT_DIR)/standard-e2e.yaml
EXPERIMENTAL_MANIFEST ?= $(OUT_DIR)/experimental.yaml
EXPERIMENTAL_E2E_MANIFEST ?= $(OUT_DIR)/experimental-e2e.yaml
OPENSHIFT_OC_MANIFEST ?= $(OUT_DIR)/openshift-operator-controller.yaml
.PHONY: manifests dirs standard standard-e2e experimental experimental-e2e fg-oc openshift-oc
dirs:
mkdir -p $(OUT_DIR)
standard-e2e: dirs
$(HELM) template $(CHART_NAME) $(CHART_DIR) \
--values $(CERT_VALUES) \
--values $(E2E_VALUES) \
> $(STANDARD_E2E_MANIFEST)
experimental-e2e: dirs
$(HELM) template $(CHART_NAME) $(CHART_DIR) \
--values $(CERT_VALUES) \
--values $(EXP_VALUES) \
--values $(E2E_VALUES) \
> $(EXPERIMENTAL_E2E_MANIFEST)
(IF we need)
# Single feature-gate (operatorController only, as example)
# Usage:
# make fg-oc FG='["MyFeatureGate"]'
FG ?= []
fg-oc: dirs
$(HELM) template $(CHART_NAME) $(CHART_DIR) \
--values $(CERT_VALUES) \
--values $(EXP_VALUES) \
--set 'features.operatorController.experimental.enabled=true' \
--set-string 'features.operatorController.featureGates=$(FG)' \
> $(OUT_DIR)/experimental-fg-operator-controller.yaml
disclaimer:
Assisted-by: AI to help build the examples to ilustrate the suggestion
There was a problem hiding this comment.
You know, I tried to put files into individual directories, and it wasn't working. I will try again.
experimental has to be a global option, so I think I will be keeping that as is; I want to keep the inputs simple, and not have to manage multiple experimental flags.
There was a problem hiding this comment.
Although, I like the idea of being able to add sets...
There was a problem hiding this comment.
So, now there is the capability to do --set via environment variable.
| @@ -24,7 +24,7 @@ To enable the Helm Chart support feature gate, you need to patch the `operator-c | |||
| 2. **Wait for the controller manager pods to be ready:** | |||
|
|
|||
| ```bash | |||
| $ kubectl -n olmv1-system wait --for condition=ready pods -l control-plane=operator-controller-controller-manager | |||
| $ kubectl -n olmv1-system wait --for condition=ready pods -l apps.kubernetes.io/name=operator-controller | |||
There was a problem hiding this comment.
I think you added it in the RFC, but could it not be a breaking change?
Should we keep both?
There was a problem hiding this comment.
I don't think it's a breaking change, it's only in our draft documentation.
There was a problem hiding this comment.
I mean, people could teorically have scripts and etc to filter things with control-plane=operator-controller-controller-manager
There was a problem hiding this comment.
theoretically... but I think our user base is very small and OCP focused. Also, I don't think we consider our YAML a published API.
aa16e93 to
7beca6e
Compare
This does not remove the kustomize config, but instead puts a helm chart into the repo, that should give very close (but not identical) results. * Adds a new chart: helm/olmv1/ - standard - experimental - openshift - cert-manager - e2e - tilt * Adds "values" files in helm/ * Adds helm executable to .bingo/ * Updates documents int docs/drafts/ * Update tests in tests/ * Update `make manifests` to use helm chart - Update the checked-in manifests - Use a tool like `dyff` to properly diff the manifests * Pull RBAC and WebHook config out of the goland code - controller-tools is not longer used to generate RBAC/Wehbooks - These resources are not part of the helm chart - The CRDs are still generated via kubebuilder Significant changes to the resulting manifests are listed in the RFC. Signed-off-by: Todd Short <tshort@redhat.com>
Signed-off-by: Todd Short <tshort@redhat.com>
Signed-off-by: Todd Short <tshort@redhat.com>
Signed-off-by: Todd Short <tshort@redhat.com>
|
|
||
| TMPDIR="$(mktemp -d)" | ||
| trap 'echo "Cleaning up $TMPDIR"; rm -rf "$TMPDIR"' EXIT | ||
| #trap 'echo "Cleaning up $TMPDIR"; rm -rf "$TMPDIR"' EXIT |
There was a problem hiding this comment.
Should it be really commented?
| # incremented each time you make changes to the application. Versions are not expected to | ||
| # follow Semantic Versioning. They should reflect the version the application is using. | ||
| # It is recommended to use it with quotes. | ||
| appVersion: "1.3.0" |
There was a problem hiding this comment.
That is interesting, how we update this value when we do a new release?
We might want a github action that will open a PR for we do not forget that
| olm.operatorframework.io/feature-set: experimental | ||
| name: operator-controller-controller-manager | ||
| namespace: olmv1-system | ||
| olm.operatorframework.io/feature-set: experimental-e2e |
There was a problem hiding this comment.
Seems that we are indeed fixing it 👍
| app.kubernetes.io/name: e2e | ||
| app.kubernetes.io/part-of: olm | ||
| name: e2e-registries-conf | ||
| namespace: olmv1-system |
There was a problem hiding this comment.
Cool it is showing up only in the e2e ones 👍
This does not remove the kustomize config, but instead puts a helm chart into the repo, that should give very close (but not identical) results.
make manifeststo use helm chartdyffto properly diff the manifestsSignificant changes to the resulting manifests are listed in the RFC.
Description
Reviewer Checklist