Conversation
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The deterministic rendering fix is correct, tested, documented, versioned, and reflected consistently in the refreshed baselines.
Review effort: Balanced
Findings: None
What changed in this PR
Refreshes Workbench snapshot baselines for issue #952 and makes PAM volume mounts deterministic.
Changes:
- Sorts PAM filenames before rendering volume mounts.
- Adds regression coverage for stable mount ordering.
- Bumps the chart to 0.22.4 and regenerates all 16 snapshots.
| File | Description |
|---|---|
charts/rstudio-workbench/templates/_helpers.tpl |
Sorts PAM mount keys. |
charts/rstudio-workbench/tests/deployment_test.yaml |
Tests deterministic PAM ordering. |
charts/rstudio-workbench/Chart.yaml |
Bumps chart version. |
charts/rstudio-workbench/NEWS.md |
Documents the fix. |
charts/rstudio-workbench/README.md |
Updates generated version references. |
charts/rstudio-workbench/snapshot/complex-values.yaml.lock |
Refreshes complex baseline. |
charts/rstudio-workbench/snapshot/default-sa-values.yaml.lock |
Refreshes default-SA baseline. |
charts/rstudio-workbench/snapshot/default.yaml.lock |
Refreshes default baseline. |
charts/rstudio-workbench/snapshot/empty-values.yaml.lock |
Refreshes empty-values baseline. |
charts/rstudio-workbench/snapshot/ingress-values.yaml.lock |
Refreshes ingress baseline. |
charts/rstudio-workbench/snapshot/ingress2-values.yaml.lock |
Refreshes alternate ingress baseline. |
charts/rstudio-workbench/snapshot/launcher-template-values.yaml.lock |
Refreshes launcher-template baseline. |
charts/rstudio-workbench/snapshot/license-file-secret-values.yaml.lock |
Refreshes secret license-file baseline. |
charts/rstudio-workbench/snapshot/license-file-values.yaml.lock |
Refreshes license-file baseline. |
charts/rstudio-workbench/snapshot/license-server-values.yaml.lock |
Refreshes license-server baseline. |
charts/rstudio-workbench/snapshot/license-values.yaml.lock |
Refreshes license baseline. |
charts/rstudio-workbench/snapshot/other-complex-values.yaml.lock |
Refreshes secondary complex baseline. |
charts/rstudio-workbench/snapshot/overrides-values-new.yaml.lock |
Refreshes newer overrides baseline. |
charts/rstudio-workbench/snapshot/overrides-values.yaml.lock |
Refreshes legacy overrides baseline. |
charts/rstudio-workbench/snapshot/simple-profiles-values.yaml.lock |
Refreshes profile baseline. |
charts/rstudio-workbench/snapshot/simple-values.yaml.lock |
Refreshes simple baseline. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
khusmann
added a commit
that referenced
this pull request
Sep 30, 2026
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #952 (second item: refresh the baselines).
With the generator repaired in #950, this regenerates the Workbench snapshot baselines. It also fixes a nondeterministic render that surfaced while doing so, because a baseline that differs from run to run can't be compared.
Baseline refresh
The
.lockfiles dated from roughly Workbench 2023.06.0. They still contained the graphite exporter,r-session-completeand therstudio/rstudio-workbench:ubuntu2204-2023.06.0image, and lackedpositron.conf,metrics-portand the sessions init container. They had also been hand-patched while the generator was broken, so they didn't match any chart version that ever shipped.All 16 baselines are regenerated with
just snapshot-rsw && just snapshot-rsw-lock(+8,168 / −3,838 lines). The diff is large but mechanical. It's the current chart's output for eachlint/values file, and there's nothing to review line by line.snapshot/is in.helmignore, so this commit on its own needs no version bump.Nondeterministic PAM mount order
_helpers.tplbuilt therstudio-pamvolume mounts fromkeys .Values.config.pam. Sprig'skeysreturns keys in Go map order, which is randomized, so when more than one PAM file is configured, the order of the mounts changes between renders.complex-values.yamlhas two PAM files, and its snapshot flipped between two consecutive runs.This affects users too, not just the tests. A different mount order is a different pod spec, so a
helm upgradewith no configuration change can restart Workbench pods for anyone with two or moreconfig.pamfiles.The fix pipes the keys through
sortAlpha. Workbench goes to 0.22.4, with a NEWS entry.Verification
should mount pam files in sorted order…) sets 12 PAM files in scrambled order and asserts the mounts by position.just snapshot-rswrun three times gives byte-identical output, andjust snapshot-rsw-diffreports "All snapshots match their .lock baselines".make lintpasses for all 15lint/values files, andjust test rstudio-workbenchpasses all 174 tests.Follow-up
The third item in #952 is running the snapshot diff in CI. That can go next, now that the baselines are current and deterministic.