jlab-cluster/helm-charts
3
0 Fork 0
Code Issues 15 Pull Requests 5 Actions Packages Wiki Activity

satisfactory: dev and server fall back instead of aborting (7.0.1) #37

Merged
jklein merged 2 commits from fix/satisfactory-null-safe-dev-server into main 2026-09-23 22:45:25 +02:00
Conversation 1 Commits 2 Files Changed 9
+124 -34
jlab-bot commented 2026-09-22 22:01:57 +02:00
Member
Copy link
Copy source

Fixes #16

--set dev=null aborted the render at .Values.dev.debug, --set server=null at .Values.server.replicas, while the third block, game, has fallen back to the chart defaults since 7.0.0. Half-safe is the worse of the two states: whoever sees game=null fall back cleanly assumes server=null does too.

Both blocks now run through satisfactory.defaultedDict, via two new helpers next to satisfactory.game.

Wider than the issue body

server is read in four files, not one — satisfactory.sfs.yaml, satisfactory.service.yaml, satisfactory.pvc.yaml and templates/NOTES.txt; dev.debug in the StatefulSet and in the notes. All four go through the helpers.

helm template never renders NOTES.txt, so a fix confined to the manifests would have looked green in every render and still aborted on a real helm install. Measured, not assumed — see "NOTES.txt" below.

The quieter half of the bug, fixed with it

The block-level abort was the loud failure. A single erased key was the silent one, and this PR closes it too:

Erased key 7.0.0 rendered now
server.gamePort / server.messagingPort Service with port: and nodePort: empty, StatefulSet with an empty containerPort falls back to 30777 / 30888
server.replicas already fell back to 1 (the replicas helper was null-safe) unchanged
server.storage PVC requesting no size unchanged — deliberately, see below

Measured on main before the change:

$ helm template t satisfactory --set server.gamePort=null | grep -n 'port:'
26:      port:
28:      nodePort:

server.storage stays the documented exception (#39)

values.schema.json documents server.storage as deliberately not null-safe — "an erased value yields an invalid PVC rather than a silently degraded workload" — so an erased size, server: null included, still renders a PVC without one and fails when it is applied.

An earlier revision of this branch pulled storage into the fallback along with everything else. That was walked back in 839b882. The reasoning is worth recording, because it is the more interesting half of this PR: the inconsistency is real and the argument for defaulting it is good, but it is a reasoned decision someone else wrote down, and overturning it here would have made this the only change in the release that alters rendered output — costing the patch exactly the property that makes it cheap to review. The trade is argued in #39 instead, with the measurement from this branch as evidence.

One detail from that discussion that belongs here: the difference only matters on a new install. For an existing release a fallback to 20Gi would fail too, because a PVC cannot shrink.

What this PR does do is make the gap legible as intent rather than oversight — the schema note, values.yaml and the chart's wiki page each name it as the exception and point at #39.

What is deliberately not duplicated

server.resources gets no defaults in the new server helper either. It already runs through satisfactory.effectiveResources, which carries limitFactor: 2 / 4Gi / 1 / 5Gi and is null-safe on its own; a second copy would be a third place for those numbers to drift. defaultedDict passes keys the defaults do not mention through unchanged, so an erased server: simply leaves resources unset and effectiveResources supplies it. Verified: --set server=null and --set server.resources=null both render the computed 8Gi / 10Gi limits.

Before/after: byte-identical for valid values

Rendered origin/main's chart and this branch's and diffed them, twice — against the chart defaults and against a full override set (every server key, dev.debug, game, a custom limitFactor with an explicit CPU limit, two env entries, all three probes and both security contexts overridden):

$ diff before-defaults.yaml after-defaults.yaml && echo IDENTICAL
IDENTICAL
$ diff before-normal.yaml  after-normal.yaml  && echo IDENTICAL
IDENTICAL

Same for the rendered NOTES.txt in both cases: identical. There is no rendered-output change in this PR at all — for valid values it is byte-identical, and for the one erased value that is not defaulted it reproduces 7.0.0 exactly:

$ helm template t <branch>   --set server=null         | grep -A1 'requests:' | tail -1
      storage:
$ helm template t <7.0.0>    --set server.storage=null | grep -A1 'requests:' | tail -1
      storage:

So the issue's condition holds and the bump is a patch, 7.0.0 → 7.0.1, appVersion untouched.

The one thing that needed care for this: the fallback helpers pass values through fromYaml, which turns every number into float64, and %v flips float64 to exponential notation from about 1e6 (#157). Ports and replicas therefore go through the existing satisfactory.formatNumber rather than being printed raw. Checked at the boundary: --set server.replicas=1000000 renders replicas: 1000000, not 1e+06.

Proof

Every null shape renders, in the manifests and in the notes:

OK  --set dev=null
OK  --set server=null
OK  --set dev=null --set server=null
OK  --set dev=null --set server=null --set game=null
OK  --set dev.debug=null
OK  --set server.replicas=null
OK  --set server.gamePort=null
OK  --set server.messagingPort=null
OK  --set server.storage=null
OK  --set server.resources=null
OK  --set server=null --set scaleDown=true
      -> replicas=1 DEBUG="false" SERVERGAMEPORT="30777" SERVERMESSAGINGPORT="30888"
         memory limit 8Gi, ephemeral-storage limit 10Gi
         storage=20Gi, except where storage itself was erased -> empty, as documented
         (last case: replicas=0 - scaleDown still wins over the defaulted 1)

Also checked:

  • the real-world shape from #99, a values file with dev:, game:, server:, securityContext:, and all three probes written with no children: aborts on origin/main; on this branch it renders, with the sizeless PVC as the only difference from the default render.
  • an explicit server.replicas: 0 is still kept, not swallowed by the fallback.
  • a scalar in place of a block (--set server=foo, --set dev=7) is still rejected by the schema.
  • helm lint --strict satisfactory — 0 failures.
  • ci/run-quantity-render-test.sh satisfactory — ok, both cases exercised the resource helper, no exponential notation.

NOTES.txt

helm template does not render it and helm install --dry-run=client is no way around that — on helm v3.22.0 it still wants a reachable API server (Kubernetes cluster unreachable), and there is no cluster available here. What worked without one: a throwaway copy of the chart in which the verbatim NOTES.txt body is wrapped in a {{- define }} and included into a scratch ConfigMap, which helm template does render — same engine, same values, same root context ($ == . inside a define invoked with .), only the sink differs.

That reproduced the pre-state at the notes' own line numbers:

main, --set server=null:  executing "notesprobe" at <.Values.server.replicas>: nil pointer ...  (NOTES.txt:1)
main, --set dev=null:     executing "notesprobe" at <.Values.dev.debug>:       nil pointer ...  (NOTES.txt:24)

and both render on this branch. With server: null the notes print the volume as unset, through the | default "unset" the notes already carried — honest about the sizeless PVC rather than quietly printing a size the manifest does not contain. So the NOTES changes are verified, not read.

No install-test covers this

satisfactory is in ci/excluded-charts.txt (multi-GB image, 10 GB+ Steam download), so the PR install-test will not touch this chart — it is dropped from the matrix, not skipped in a job. Strict lint, package, the version-bump check and the quantity render test still run; the local rendering above is the only proof there is that the workload manifests are right.

Docs

  • Wiki, already pushed (313ec47, 72fdda3): the satisfactory page (version snapshot, dev and server documented as erasable, the erased-key table with a "now" column, a section on why the storage exception stands and where it is argued, why server.resources has no default there, and a CI note that no stage of this chart's pipeline renders the notes); Chart-Style-Guide §2 generalised from "probes, security contexts and resources" to every chart-owned block, plus "an exception is allowed, an unmarked one is not" and the NOTES.txt blind spot; a new Lessons-Learned entry for the half-protected chart, for notes as template code the pipeline cannot see, and for a bugfix that stops being one when it settles a design question.
  • README.md §12.2 — the code-side copy of the same rules.
  • values.yaml and values.schema.json document the fallbacks at every affected key, and the exception at the one key that has none.
Fixes #16 `--set dev=null` aborted the render at `.Values.dev.debug`, `--set server=null` at `.Values.server.replicas`, while the third block, `game`, has fallen back to the chart defaults since 7.0.0. Half-safe is the worse of the two states: whoever sees `game=null` fall back cleanly assumes `server=null` does too. Both blocks now run through `satisfactory.defaultedDict`, via two new helpers next to `satisfactory.game`. ## Wider than the issue body `server` is read in **four** files, not one — `satisfactory.sfs.yaml`, `satisfactory.service.yaml`, `satisfactory.pvc.yaml` and **`templates/NOTES.txt`**; `dev.debug` in the StatefulSet and in the notes. All four go through the helpers. `helm template` never renders `NOTES.txt`, so a fix confined to the manifests would have looked green in every render and still aborted on a real `helm install`. Measured, not assumed — see "NOTES.txt" below. ## The quieter half of the bug, fixed with it The block-level abort was the loud failure. A *single* erased key was the silent one, and this PR closes it too: | Erased key | 7.0.0 rendered | now | | --- | --- | --- | | `server.gamePort` / `server.messagingPort` | Service with `port:` and `nodePort:` empty, StatefulSet with an empty `containerPort` | falls back to `30777` / `30888` | | `server.replicas` | already fell back to `1` (the replicas helper was null-safe) | unchanged | | `server.storage` | PVC requesting no size | **unchanged — deliberately, see below** | Measured on `main` before the change: ``` $ helm template t satisfactory --set server.gamePort=null | grep -n 'port:' 26: port: 28: nodePort: ``` ## `server.storage` stays the documented exception (#39) `values.schema.json` documents `server.storage` as deliberately **not** null-safe — "an erased value yields an invalid PVC rather than a silently degraded workload" — so an erased size, `server: null` included, still renders a PVC without one and fails when it is applied. An earlier revision of this branch pulled `storage` into the fallback along with everything else. That was walked back in `839b882`. The reasoning is worth recording, because it is the more interesting half of this PR: the inconsistency is real and the argument for defaulting it is good, but it is a **reasoned decision someone else wrote down**, and overturning it here would have made this the only change in the release that alters rendered output — costing the patch exactly the property that makes it cheap to review. The trade is argued in **#39** instead, with the measurement from this branch as evidence. One detail from that discussion that belongs here: **the difference only matters on a new install.** For an existing release a fallback to `20Gi` would fail too, because a PVC cannot shrink. What this PR does do is make the gap legible as intent rather than oversight — the schema note, `values.yaml` and the chart's wiki page each name it as the exception and point at #39. ## What is deliberately *not* duplicated `server.resources` gets no defaults in the new `server` helper either. It already runs through `satisfactory.effectiveResources`, which carries `limitFactor: 2` / `4Gi` / `1` / `5Gi` and is null-safe on its own; a second copy would be a third place for those numbers to drift. `defaultedDict` passes keys the defaults do not mention through unchanged, so an erased `server:` simply leaves `resources` unset and `effectiveResources` supplies it. Verified: `--set server=null` and `--set server.resources=null` both render the computed `8Gi` / `10Gi` limits. ## Before/after: byte-identical for valid values Rendered `origin/main`'s chart and this branch's and diffed them, twice — against the chart defaults and against a full override set (every `server` key, `dev.debug`, `game`, a custom `limitFactor` with an explicit CPU limit, two `env` entries, all three probes and both security contexts overridden): ``` $ diff before-defaults.yaml after-defaults.yaml && echo IDENTICAL IDENTICAL $ diff before-normal.yaml after-normal.yaml && echo IDENTICAL IDENTICAL ``` Same for the rendered `NOTES.txt` in both cases: identical. **There is no rendered-output change in this PR at all** — for valid values it is byte-identical, and for the one erased value that is not defaulted it reproduces 7.0.0 exactly: ``` $ helm template t <branch> --set server=null | grep -A1 'requests:' | tail -1 storage: $ helm template t <7.0.0> --set server.storage=null | grep -A1 'requests:' | tail -1 storage: ``` So the issue's condition holds and the bump is a **patch**, `7.0.0` → `7.0.1`, `appVersion` untouched. The one thing that needed care for this: the fallback helpers pass values through `fromYaml`, which turns every number into `float64`, and `%v` flips `float64` to exponential notation from about `1e6` (#157). Ports and replicas therefore go through the existing `satisfactory.formatNumber` rather than being printed raw. Checked at the boundary: `--set server.replicas=1000000` renders `replicas: 1000000`, not `1e+06`. ## Proof Every null shape renders, in the manifests **and** in the notes: ``` OK --set dev=null OK --set server=null OK --set dev=null --set server=null OK --set dev=null --set server=null --set game=null OK --set dev.debug=null OK --set server.replicas=null OK --set server.gamePort=null OK --set server.messagingPort=null OK --set server.storage=null OK --set server.resources=null OK --set server=null --set scaleDown=true -> replicas=1 DEBUG="false" SERVERGAMEPORT="30777" SERVERMESSAGINGPORT="30888" memory limit 8Gi, ephemeral-storage limit 10Gi storage=20Gi, except where storage itself was erased -> empty, as documented (last case: replicas=0 - scaleDown still wins over the defaulted 1) ``` Also checked: - the real-world shape from #99, a values **file** with `dev:`, `game:`, `server:`, `securityContext:`, and all three probes written with no children: aborts on `origin/main`; on this branch it renders, with the sizeless PVC as the only difference from the default render. - an explicit `server.replicas: 0` is still kept, not swallowed by the fallback. - a scalar in place of a block (`--set server=foo`, `--set dev=7`) is still rejected by the schema. - `helm lint --strict satisfactory` — 0 failures. - `ci/run-quantity-render-test.sh satisfactory` — ok, both cases exercised the resource helper, no exponential notation. ### NOTES.txt `helm template` does not render it and `helm install --dry-run=client` is no way around that — on helm v3.22.0 it still wants a reachable API server (`Kubernetes cluster unreachable`), and there is no cluster available here. What worked without one: a throwaway copy of the chart in which the verbatim `NOTES.txt` body is wrapped in a `{{- define }}` and included into a scratch ConfigMap, which `helm template` *does* render — same engine, same values, same root context (`$` == `.` inside a `define` invoked with `.`), only the sink differs. That reproduced the pre-state at the notes' own line numbers: ``` main, --set server=null: executing "notesprobe" at <.Values.server.replicas>: nil pointer ... (NOTES.txt:1) main, --set dev=null: executing "notesprobe" at <.Values.dev.debug>: nil pointer ... (NOTES.txt:24) ``` and both render on this branch. With `server: null` the notes print the volume as `unset`, through the `| default "unset"` the notes already carried — honest about the sizeless PVC rather than quietly printing a size the manifest does not contain. So the NOTES changes are **verified, not read**. ## No install-test covers this `satisfactory` is in `ci/excluded-charts.txt` (multi-GB image, 10 GB+ Steam download), so **the PR install-test will not touch this chart** — it is dropped from the matrix, not skipped in a job. Strict lint, package, the version-bump check and the quantity render test still run; the local rendering above is the only proof there is that the workload manifests are right. ## Docs - **Wiki, already pushed** (`313ec47`, `72fdda3`): the `satisfactory` page (version snapshot, `dev` and `server` documented as erasable, the erased-key table with a "now" column, a section on why the `storage` exception stands and where it is argued, why `server.resources` has no default there, and a CI note that no stage of this chart's pipeline renders the notes); `Chart-Style-Guide` §2 generalised from "probes, security contexts and resources" to every chart-owned block, plus "an exception is allowed, an unmarked one is not" and the `NOTES.txt` blind spot; a new `Lessons-Learned` entry for the half-protected chart, for notes as template code the pipeline cannot see, and for a bugfix that stops being one when it settles a design question. - **README.md** §12.2 — the code-side copy of the same rules. - `values.yaml` and `values.schema.json` document the fallbacks at every affected key, and the exception at the one key that has none.
jlab-bot added 1 commit 2026-09-22 22:01:58 +02:00
satisfactory: dev and server fall back instead of aborting (7.0.1)
Validate Helm Charts / Detect changed charts (pull_request) Successful in 3s
Details
Validate Helm Charts / Resource quantities (pull_request) Successful in 8s
Details
Validate Helm Charts / Build ${{ fromJSON(needs.detect-changed-charts.outputs.charts) }} (pull_request) Successful in 3s
Details
Validate Helm Charts / Install-test ${{ fromJSON(needs.detect-changed-charts.outputs.install-charts) }} (pull_request) Skipped
Details
Validate Helm Charts / Trivy config ${{ fromJSON(needs.detect-changed-charts.outputs.scan-charts) }} (pull_request) Successful in 9s
Details
57e54f93fc 
--set dev=null aborted the render at .Values.dev.debug and --set server=null
at .Values.server.replicas, while the third block, game, has fallen back to
the chart defaults since 7.0.0. Half-safe is the worse state of the two:
whoever sees game=null work assumes server=null does too.

Both blocks now run through satisfactory.defaultedDict like game, via two new
helpers next to it. server.resources is deliberately NOT re-defaulted there -
it already runs through satisfactory.effectiveResources, which carries those
defaults and is null-safe on its own, and a second copy of limitFactor: 2 /
4Gi / 1 / 5Gi would be a third place for them to drift.

server is read in four files, not one, and the fourth is NOTES.txt, which
helm template never renders - a fix confined to the manifests would have
looked green and still aborted on a real helm install. All four go through
the helpers.

The single erased key was the quieter half of the bug and is fixed with it:
a nulled server.gamePort rendered "port:" and "nodePort:" empty into the
Service and an empty containerPort into the StatefulSet, and a nulled
server.storage a PVC asking for no size - manifests rejected by nothing until
the API server saw them. server.storage was documented in values.schema.json
as deliberately not null-safe; that reasoning does not survive the block-level
case, because it made "server: null" render a sizeless PVC instead of falling
back like everything else. The schema note is replaced rather than left lying.

Rendered output for valid values is unchanged: helm template against the
chart defaults and against a full override set is byte-identical to 7.0.0,
and so is NOTES.txt. Hence a patch bump, appVersion untouched.

Fixes #16
jklein added 1 commit 2026-09-22 22:09:37 +02:00
satisfactory: server.storage stays the documented exception (#39)
Validate Helm Charts / Detect changed charts (pull_request) Successful in 4s
Details
Validate Helm Charts / Resource quantities (pull_request) Successful in 7s
Details
Validate Helm Charts / Build ${{ fromJSON(needs.detect-changed-charts.outputs.charts) }} (pull_request) Successful in 4s
Details
Validate Helm Charts / Install-test ${{ fromJSON(needs.detect-changed-charts.outputs.install-charts) }} (pull_request) Skipped
Details
Validate Helm Charts / Trivy config ${{ fromJSON(needs.detect-changed-charts.outputs.scan-charts) }} (pull_request) Successful in 10s
Details
839b8825ed 
Walks back the one part of the previous commit that changed rendered output:
storage was pulled into the server fallback, so an erased size fell back to
20Gi instead of rendering a PVC without one.

values.schema.json had documented the opposite decision - "an erased value
yields an invalid PVC rather than a silently degraded workload" - and that is
someone's reasoned trade, not collateral of the nil-pointer bug next to it.
Overturning it inside a patch would have made this the only release where
valid values render differently, and it would have settled an open design
question in a PR nobody opened for that. It goes to #39 instead.

So storage is out of the defaults again and the schema note is restored
verbatim, extended only by where the exception is argued. values.yaml marks
it as the one key of the block that is deliberately not null-safe. The
README rule that had leaned on this example now says the thing that actually
holds: an exception is fine, an unmarked one is not.

Everything else stands - dev and server null-safe, the single-key findings on
the ports, formatNumber against exponential notation, 7.0.1.

Re-measured after the walk-back: rendered output against the chart defaults
and against a full override set is byte-identical to 7.0.0, NOTES.txt
included, and "--set server=null" now renders exactly the sizeless PVC that
"--set server.storage=null" rendered on 7.0.0.
jklein approved these changes 2026-09-23 22:45:20 +02:00
jklein merged commit 4715d85b99 into main 2026-09-23 22:45:25 +02:00
jklein deleted branch fix/satisfactory-null-safe-dev-server 2026-09-23 22:45:25 +02:00
Sign in to join this conversation.
Reviewers
No Reviewers
jklein
Labels
Clear labels
inhouse-chart
Inhouse-Chart aus der JLab-Git-Paketregistry
 renovate
Von Renovate angelegt
No labels
Milestone
No items
No Milestone
Assignees
Clear assignees
jklein (Justin Klein) jlab-bot (jlab-git-bot) jlab-renovate-bot (jlab-renovate-bot)
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: jlab-cluster/helm-charts#37