-
Notifications
You must be signed in to change notification settings - Fork 29
chore(helm): add helm charts for operator installation #608
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
chore(helm): add helm charts for operator installation #608
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v1alpha1 #608 +/- ##
=========================================
Coverage 77.26% 77.26%
=========================================
Files 11 11
Lines 1267 1267
=========================================
Hits 979 979
Misses 258 258
Partials 30 30 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
23c227d
to
1db40dd
Compare
1db40dd
to
5d4660f
Compare
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.
Pull Request Overview
This PR adds comprehensive Helm chart support for the Kepler Operator, enabling deployment via Helm in addition to the existing kustomize-based installation method.
- Adds complete Helm chart with templates for all operator resources
- Implements CRD synchronization and validation tooling
- Includes comprehensive documentation and CI integration
Reviewed Changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
manifests/helm/kepler-operator/values.yaml | Default configuration values for operator deployment |
manifests/helm/kepler-operator/templates/*.yaml | Template files for all Kubernetes resources |
manifests/helm/kepler-operator/crds/*.yaml | Custom Resource Definitions synced from config/crd/bases |
manifests/helm/kepler-operator/Chart.yaml | Chart metadata and dependencies |
docs/developer/helm-chart-maintenance.md | Developer guide for maintaining the Helm chart |
hack/helm-validate.sh | Validation script for chart consistency |
Makefile | Helm-related targets for installation and testing |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
hack/helm/validate.sh
Outdated
helm template kepler-operator "$HELM_CHART_DIR" \ | ||
--namespace kepler-operator \ | ||
--set image.tag=0.21.0 \ | ||
--set kepler.image.tag=latest \ | ||
--set kubeRbacProxy.image.tag=v0.18.1 \ | ||
--set metrics.serviceMonitor.enabled=true |
Copilot
AI
Oct 2, 2025
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.
Same incorrect value paths as above. These --set parameters don't match the values.yaml structure.
Copilot uses AI. Check for mistakes.
.github/workflows/pr-checks.yaml
Outdated
- name: Use Kepler Action for Kind cluster | ||
uses: sustainable-computing-io/[email protected] | ||
with: | ||
ebpfprovider: libbpf | ||
cluster_provider: kind | ||
env: | ||
PROMETHEUS_ENABLE: "true" |
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.
We should use make cluster-up
which will also deploy cert-manager so no need to manually deploy and verify in .github/helm-e2e/action.yaml
Signed-off-by: Sunil Thaha <[email protected]>
ec5b8ac
to
768feb9
Compare
768feb9
to
ee2f5a2
Compare
hack/helm/validate.sh
Outdated
info "Validating Helm templates render successfully..." | ||
if helm template kepler-operator "$HELM_CHART_DIR" \ | ||
--namespace kepler-operator \ | ||
--set operator.image=quay.io/sustainable_computing_io/kepler-operator:0.21.0 \ |
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.
Can we keep the version's in variable?
hack/helm/validate.sh
Outdated
ok "Helm templates render successfully" | ||
else | ||
fail "Helm template rendering failed" | ||
helm template kepler-operator "$HELM_CHART_DIR" \ |
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.
Since its the same command can we define it once?
hack/helm/validate.sh
Outdated
) | ||
|
||
local rendered | ||
rendered=$(helm template kepler-operator "$HELM_CHART_DIR" \ |
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.
Same here as well
.github/workflows/pr-checks.yaml
Outdated
KIND_VERSION: 0.27.0 | ||
GO111MODULE: on | ||
KUBECONFIG: /tmp/.kube/config |
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.
We can remove these?
.github/helm-e2e/action.yaml
Outdated
env: | ||
VERSION: ${{ inputs.version }} | ||
|
||
- name: Capture cluster state |
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.
Shouldn't we capture this in the script itself?
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.
yes, and no .. I think it is best to have this step run no matter what ( if: always ). yes, we can move this to a hack/ci/capture.sh (perhaps in a separate pr? )
tests/helm.sh
Outdated
--set kepler.image=quay.io/sustainable_computing_io/kepler:latest \ | ||
--set kube-rbac-proxy.image=quay.io/brancz/kube-rbac-proxy:v0.19.0 \ |
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.
Versions as variable?
tests/helm.sh
Outdated
header "Deploy PowerMonitor" | ||
|
||
# Create fake CPU meter ConfigMap | ||
create_fake_cpu_configmap "$POWERMONITOR_NS" |
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.
Shouldn't we control this via flag just like in operator e2e?
Something like:
if --running-on-vm
then create_fake_cpu_configmap
.github/workflows/pr-checks.yaml
Outdated
uses: ./.github/compute-version | ||
id: version | ||
|
||
- name: Build and load operator image |
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.
e2e supports building operator. Shouldn't we use that instead of doing it here?
hack/cert-manager/verify.sh
Outdated
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.
good to remove this?
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.
same for this
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.
🤔
hack/ci/powermonitor-fake-cpu.yaml
Outdated
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.
same for this
88171b0
to
2312f79
Compare
Makefile
Outdated
HELM_TIMEOUT ?= 2m | ||
|
||
.PHONY: check-cert-manager | ||
check-cert-manager: ## Check if cert-manager is installed |
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.
Do we need a target for this?
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.
No, used to need it when the setup was only using kind.
Makefile
Outdated
cp config/crd/bases/kepler.system.sustainable.computing.io_powermonitors.yaml \ | ||
$(HELM_CHART_DIR)/crds/ | ||
cp config/crd/bases/kepler.system.sustainable.computing.io_powermonitorinternals.yaml \ |
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.
cp the whole dir instead of individual?
Makefile
Outdated
|
||
.PHONY: helm-lint | ||
helm-lint: helm ## Lint the Helm chart | ||
helm lint $(HELM_CHART_DIR) |
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.
HELM ?= $(LOCALBIN)/helm
We need to use $(HELM) instead of helm
Add Helm chart for deploying Kepler Operator with automated validation and testing infrastructure. Changes: - Add Helm chart in manifests/helm/kepler-operator/ with templates for all operator resources (deployment, RBAC, webhooks, cert-manager integration) - Add Makefile targets for Helm operations (lint, template, install, validate) - Add hack/helm/validate.sh for three-layer validation (syntax, rendering, consistency with kustomize) - Add hack/ci/ resources for CI testing with fake CPU meter - Add hack/cert-manager/ utilities for certificate verification - Add comprehensive documentation in docs/developer/helm-chart-maintenance.md - Update pre-commit hooks to validate Helm chart syntax - Update README.md with Helm deployment option The chart follows hybrid automation approach: manual templates with automated CRD sync and validation against kustomize output. Signed-off-by: Sunil Thaha <[email protected]>
2312f79
to
ba4b4d8
Compare
No description provided.