improvement(helm): hygiene pass — CI gating, strict values schema, ESO v1 default#5939
Conversation
…O v1 default, ci values Closes the gaps from a best-practices audit of the chart (template-level conformance was already clean: full label set, 82 unit tests, kubeconform- valid renders): - new Helm Chart workflow gates every chart change: helm lint, the 82 helm-unittest cases, kubeconform validation of default + all-components renders (k8s 1.29 strict, CRD catalog), and a render of all 10 example values files - helm/sim/ci/ values files (chart-testing convention) so the chart lints and templates cleanly out of the box with dummy secrets - values.schema.json declares all 30 top-level keys (16 were invisible) and sets root additionalProperties: false, so top-level typos fail fast - externalSecrets.apiVersion defaults to v1: current ESO releases removed the v1beta1 compatibility path in 2026, so the old default produced rejected manifests on new installs; NOTES/values comments updated - wait-for-postgres init container gets requests/limits (the only container in the chart without them; broke ResourceQuota'd namespaces) - drop the telemetry Prometheus scrape config for app/realtime — neither exposes /metrics, so it was dead config that also rendered a realtime target with realtime disabled - chart 1.2.0 with README upgrade notes
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@cursor review |
PR SummaryMedium Risk Overview Ships Chart 1.2.0 sets
Removes the telemetry collector’s Prometheus scrape jobs for app/realtime (no Reviewed by Cursor Bugbot for commit 1cfaf0f. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 187e266. Configure here.
…gth suite Helm v4 phrases schema rejections as 'minLength: got N, want 32' while v3 says 'String length must be greater than or equal to 32'. The suite grepped for 'minLength' only, so it passed on local helm v4 and failed on CI's v3.16.4 — the enforcement itself works on both. Patterns now assert the key name plus either wording. Caught by the new Helm Chart workflow on its very first run.
|
@cursor review |
Greptile SummaryThe follow-up changes complete the previously requested Helm CI and strict-schema fixes.
Confidence Score: 5/5The PR appears safe to merge because no blocking failures remain in the fixes covered by the previous review threads. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| .github/workflows/helm.yml | Adds Helm linting, unit tests, schema validation, version gating, and kind installation with immutable CI dependencies. |
| helm/sim/values.schema.json | Enables strict root-key validation while declaring the standard Helm naming overrides. |
| helm/sim/tests/smoke_test.yaml | Adds render coverage confirming that both naming overrides affect generated resource names. |
| helm/sim/values.yaml | Changes the default External Secrets API version to v1 and updates image-tag guidance. |
| helm/sim/templates/telemetry.yaml | Removes the unused Prometheus scrape receiver while retaining OTLP metrics ingestion. |
Reviews (7): Last reviewed commit: "fix(helm): appVersion points at a real G..." | Re-trigger Greptile
Greptile SummaryThe PR adds Helm-chart CI validation and stricter values checking while updating ESO and chart runtime defaults.
Confidence Score: 3/5The PR should not merge until the strict schema accepts the chart's supported name overrides; the CI dependency pinning concern is non-blocking. Root schema strictness deterministically rejects nameOverride and fullnameOverride before templates can consume them, breaking installs and upgrades that use those supported Helm naming controls. helm/sim/values.schema.json and .github/workflows/helm.yml
|
| Filename | Overview |
|---|---|
| helm/sim/values.schema.json | Adds root strictness but omits the template-supported nameOverride and fullnameOverride keys, breaking those installations. |
| .github/workflows/helm.yml | Adds comprehensive chart validation, with a non-blocking integrity gap from mutable action references and an unchecked binary download. |
| helm/sim/templates/telemetry.yaml | Removes dead Prometheus scraping while retaining OTLP ingestion and configured metrics exporters. |
| helm/sim/templates/external-secret-app.yaml | Updates the generated ExternalSecret API default to v1 consistently with chart values and documentation. |
| helm/sim/templates/job-copilot-migrations.yaml | Adds fixed requests and limits to the PostgreSQL readiness init container. |
| helm/sim/values.yaml | Changes the External Secrets Operator API default to v1 and documents the legacy override. |
Reviews (1): Last reviewed commit: "fix(helm): version-agnostic schema-error..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Benchmarked against the flagship OSS charts (ingress-nginx, argo-cd, kube-prometheus-stack, grafana, bitnami, cert-manager): sim already exceeds most of them on validation rigor (strict schema — 4 of 6 ship none; 82 unit tests vs argo-cd's zero; kubeconform manifest validation none of them run), but every top community chart repo actually installs the chart on a kind cluster in chart CI — the one majority practice we lacked. Adds: - install job: kind cluster, helm install with ci/default + a new small-footprint ci/kind-values.yaml overlay (default app requests of 4Gi can't schedule on a CI node), --wait, then the chart's helm test hook, with pod/event/log diagnostics on failure - version-bump job (PR-only): fails when helm/sim/** changes without a Chart.yaml version increment — argo/kps/grafana all enforce this
…actions - nameOverride/fullnameOverride are consumed by sim.name/sim.fullname but were never in values.yaml, so root additionalProperties: false rejected Helm's standard naming overrides — both now declared (a helper-wide sweep confirmed they were the only template-read keys missing), with a smoke regression test that installs under the strict schema and asserts the override lands in resource names - CI supply-chain hardening: checkout/setup-helm/kind-action pinned to full commit SHAs (tag comments retained) and the kubeconform archive verified against its published sha256
|
@cursor review |
Greptile SummaryAdds comprehensive Helm-chart CI and tightens chart defaults and validation.
Confidence Score: 4/5The PR appears safe to merge, with a non-blocking opportunity to integrity-pin the executable tooling introduced in the Helm workflow. The chart behavior and strict-schema changes remain internally consistent, while the only accepted concern is conditional exposure of the read-only CI runner if an unverified upstream executable artifact is compromised. .github/workflows/helm.yml
|
| Filename | Overview |
|---|---|
| .github/workflows/helm.yml | Adds useful chart validation gates, but executable third-party tooling is fetched without immutable or integrity-verified pinning. |
| helm/sim/values.schema.json | Adds compatible declarations for previously omitted top-level chart values and rejects unknown root keys. |
| helm/sim/templates/telemetry.yaml | Removes the ineffective app/realtime Prometheus receiver while preserving OTLP ingestion and remote-write exporting. |
| helm/sim/templates/external-secret-app.yaml | Switches ExternalSecret rendering to the current v1 API while retaining configurable legacy-version support. |
| helm/sim/templates/job-copilot-migrations.yaml | Adds explicit resource requests and limits to the PostgreSQL readiness init container. |
Reviews (3): Last reviewed commit: "fix(helm): declare naming overrides in t..." | Re-trigger Greptile
- helm-unittest runs via the project's official docker image pinned by immutable sha256 digest instead of a plugin install from a mutable git tag - the version-bump gate fetches the full base ref (a --depth=1 fetch could leave no merge base), computes the merge-base and diff outside the if so any git failure fails the job instead of falling into the skip branch
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit e4b34ca. Configure here.
The digest-pinned image runs as a non-root user that cannot write into the runner-owned bind mount (it creates tests/__snapshot__, absent from the checkout since empty dirs aren't tracked). Standard bind-mount pattern: --user "$(id -u):$(id -g)" with HOME=/tmp for helm's cache.
This comment has been minimized.
This comment has been minimized.
The kind install job caught this on its first full run: the default image tag (Chart.AppVersion 0.6.73) returns 404 on GHCR for all three images — the registry's tags are v-prefixed — so an unpinned default install could never pull. Updated appVersion to v0.7.44 (verified 200 for simstudio, realtime, and migrations manifests), stale values comment refreshed, and an upgrade note added. The CI kind values deliberately stay tag-free so the job keeps exercising the true default path.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 1cfaf0f. Configure here.
1 participant
Summary
Closes the gaps from a best-practices audit of the chart against the official Helm guidance and current ecosystem tooling norms (template-level conformance was already clean: full
app.kubernetes.io/*label coverage, 82 unit tests, kubeconform-valid renders, complete Chart.yaml):Helm Chartworkflow onhelm/sim/**changes —helm lint, the 82 helm-unittest cases, kubeconform validation (k8s 1.29 strict + CRD catalog) of the default and all-components renders, and a render of all 10 example values files. Until now the chart's test suite only ran when someone remembered to run it locally.helm/sim/ci/values files (chart-testing convention) with dummy secrets, so the chart lints/templates cleanly out of the box — previouslyhelm templatewith no values errored on the required-secret guards.networkPolicy,monitoring,serviceAccount,migrations,cronjobs, etc. — were invisible to validation) plus rootadditionalProperties: false, so a top-level typo likenetworkPolciy:fails at install time instead of being silently ignored.v1beta1→v1(time-sensitive): External Secrets Operator removed the v1beta1 compatibility path upstream in 2026, so the old default produced rejected manifests for anyone on a current ESO.v1beta1remains available viaexternalSecrets.apiVersionfor ESO < 0.17; NOTES and values comments updated.wait-for-postgresinit container gets requests/limits — the only container in the chart without them (broke ResourceQuota'd namespaces)./metricson app/realtime, which neither exposes, and rendered a realtime target even with realtime disabled; OTLP ingestion unchanged.Deliberately deferred (documented in the audit):
helm.sh/resource-policy: keepon standalone PVCs (README documents uninstall behavior), HPA null-metrics guard (unreachable with defaults), helm-docs table + OCI publishing (chart is git-clone-consumed by design), telemetry collector image bump (needs its own config-migration change).Type of Change
Testing
Checklist