docs(helm): document null as the way to remove an inherited env key - #6801
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryLow Risk Overview Docs now explain that only non-empty Tests ( Chart version bumps to 1.5.4; no Helm template logic changes in this PR. Reviewed by Cursor Bugbot for commit f5316c5. Configure here. |
Greptile SummaryThe PR documents Helm’s
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| helm/sim/README.md | Documents layered null deletion, secret-mode behavior, deployment-tool caveats, and a local-chart verification command; the previously reported issues are addressed. |
| apps/docs/content/docs/en/platform/self-hosting/environment-variables.mdx | Adds matching self-hosting guidance for removing inherited limits and handling ESO, reuse-values, and Argo CD. |
| helm/sim/tests/env-null-deletion_test.yaml | Adds focused regression tests covering empty overrides, deletion from both configuration layers, Secret omission, and boot-critical validation. |
| helm/sim/values.yaml | Adds comments directing operators to use null rather than an empty string when deleting inherited defaults. |
| helm/sim/Chart.yaml | Bumps the chart patch version to publish the documentation and regression-test update. |
Reviews (3): Last reviewed commit: "docs(helm): say null must be applied in ..." | Re-trigger Greptile
|
@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 ba302bc. Configure here.
Setting `app.env.KEY: ""` cannot clear a key that `app.envDefaults` sets: the Secret template drops empty values, and the deployment template treats an empty override as "not overridden" and still inlines the default. Helm's own `KEY: null` deletion is the supported mechanism and already works. The empty-string behavior is load-bearing, not a bug — every key under `app.env` ships as a "" placeholder, and ten collide with a real `envDefaults` value (NEXT_PUBLIC_APP_URL, BETTER_AUTH_URL, ...), so "" has to read as "unspecified" or a default install would blank them out. - README: document `null`, with the --reuse-values and Argo CD valuesObject caveats; correct the claim that `app.env` always wins over `app.envDefaults` - values.yaml + self-hosting docs: same guidance where operators look - sim-helm skill: record why an unset list is the wrong shape here - tests: lock in that null removes a key and "" does not
…uired-secret claim - The verify snippet used a `sim/sim` repo alias that this chart never publishes; every other instruction installs from the local `./helm/sim` path, so the command could not run as written - Nulling a boot-critical key only fails at template time with the chart-managed Secret. `existingSecret` mode skips that validation entirely (the chart cannot read a pre-created Secret), and under ESO the key must instead be mapped in externalSecrets.remoteRefs.app
`null` deletes a key from the map it is applied to, not from the pod. A key set in both `app.env` and `app.envDefaults` survives a null on the app.env entry alone — the deployment then inlines the envDefaults value again. Under ESO a retained `externalSecrets.remoteRefs.app` mapping keeps syncing the key regardless of app.env. - README and self-hosting docs: drop the "works in all three secret modes" shorthand and spell out that every layer setting the key must be nulled, including the ESO remote mapping - tests: cover both halves — nulling only app.env restores the envDefault, nulling both actually removes the key - chart 1.5.4; staging took 1.5.3 in the meantime
ba302bc to
f5316c5
Compare
|
@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 f5316c5. Configure here.
Summary
app.env.KEY: ""cannot clear a key thatapp.envDefaultssets — the Secret template drops empty values, and the deployment template treats an empty override as "not overridden" and still inlines the default (inlineenvbeatsenvFrom). Helm's ownKEY: nulldeletion is the supported mechanism and already works today, on charts already deployedapp.envkeys ship as""placeholders, and 10 collide with a realenvDefaultsvalue (NEXT_PUBLIC_APP_URL,BETTER_AUTH_URL,NEXT_PUBLIC_BRAND_NAME,VERTEX_LOCATION,EMAIL_VERIFICATION_ENABLED, …). If""meant "delete", a default install would blank those outnull, including the--reuse-valuesand Argo CDvaluesObjectcaveats. Corrects the existing claim that a value inapp.env"always wins" overapp.envDefaults— true only for non-empty valuesvalues.yamland the self-hosting docs: same guidance where operators actually looksim-helmskill: record why a chart-level "unset list" is the wrong shape here (Helm merges dicts but not lists, so it can't be overridden downstream)nullremoves a key and""does not, so a future template edit can't silently break cooperation with Helm's mergeNo template logic changed — this is docs plus a regression net. Most common real-world case is a free-tier cap inherited from a chart older than 1.5.0, which preset
FREE_TABLES_LIMIT: "3"andFREE_TABLE_ROWS_LIMIT: "1000"underenvDefaults.Type of Change
Testing
helm lintclean and 105/105 chart unit tests pass, verified against the digest-pinned helm-unittest image CI uses, not just the local plugin. Also confirmednulldeletion end-to-end against an unmodified chart copy with the old presets restored (values-file and--setforms,envDefaultsandapp.envpaths), and that stripping the placeholders renders byte-identical output. Repo-wide:bun run lint, block-registry check, and all 29check:auditspass.Checklist