Skip to content

Accept ordered config files in Workbench and deprecate the map form - #953

Draft
khusmann wants to merge 20 commits into
ordered-config-libraryfrom
ordered-config-workbench
Draft

khusmann wants to merge 20 commits into
ordered-config-libraryfrom
ordered-config-workbench

Conversation

@khusmann

@khusmann khusmann commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Step 2 of #944, and resolves #948. Stacked on #945 — base is ordered-config-library, so this diff shows only the Workbench changes. Retarget to main once #945 merges.

Important

CI fails until #945 merges and publishes rstudio-library 0.1.38. Chart.yaml requires 0.1.38, helm dependency update resolves from helm.rstudio.com, and the newest published is 0.1.37. Chart.lock is therefore still pinned at 0.1.35 and can only be regenerated after that release. Draft until then.

Ordered config files

Adopts the list form from #945 for the files whose behavior depends on the order of their sections or entries — config.server.profiles, config.server.launcher.*.resources.conf, config.profiles.launcher.*.profiles.conf, config.session.repos.conf:

config:
  server:
    profiles:
      - jsmith:
          max-memory-mb: 8192
      - "@analysts":
          max-memory-mb: 4096
      - "*":
          max-memory-mb: 1024

Previously that rendered alphabetically, so a user named jsmith lost their own settings to every group they belong to.

configmap-general.yaml — the chart merges its default [*] section into config.profiles, which a plain mergeOverwrite would clobber when the value is a list. It now merges into the written [*] entry in place, keeping its position, or prepends one if the admin wrote none.

NOTES.txt warns in both directions, because which form is right is decided by the file rather than by preference:

map list
order-sensitive ini ⚠️ sorts by name, changing what the file does ✅
any other ini ✅ merges with the chart's defaults ⚠️ replaces them

The second half is the one that is easy to miss: Helm merges a map but replaces a list, so writing an order-agnostic file as a list silently drops whatever default the chart ships for it — launcher.conf loses its [server] section, which the launcher needs, with no error. Neither warning has an arity gate, so a single-section map warns too.

Files that are not ini — r-versions, notifications.conf, *.json — are exempt, since a list is how you legitimately write those.

Filename dispatch lives in one place. _helpers.tpl defines config.orderSensitive and config.nonIni; configmap-session.yaml uses them to pick a renderer and NOTES.txt uses them to raise both warnings. Three consumers, one source of truth — the alternative was three copies of the same filename lists, which is how ci/ and lint/ drifted apart in #952.

values.yaml — config.session.repos\.conf becomes a single-entry list, since otherwise every default install would warn about the chart's own default.

Session files are routed by format — resolves #948

configmap-session.yaml handed the whole config.session map to config.ini, regardless of each file's actual format. They are not all ini:

Files Renderer
r-versions, notifications.conf config.dcf — Key: Value, blank-line separated records
*.json config.json
everything else config.ini, unchanged

r-versions therefore rendered as Key=Value, which Workbench cannot parse: it falls back to a legacy mode, treats each line as a directory path, and logs does not point to a valid directory per line. No R versions were registered. notifications.conf has the same shape and the same problem; rstudio-prefs.json was latent, working only because its default is a raw string.

All three renderers already existed in rstudio-library, so this is routing rather than new rendering logic. Two details:

  • A raw string always passes through unchanged, so it stays with the ini renderer. Routing a string to config.json would re-encode it with toPrettyJson and turn {} into the quoted string "{}", breaking the current rstudio-prefs.json default.
  • An empty value stays with ini. Nothing renders either way, and routing the empty notifications.conf default to DCF added a stray blank line.

repos.conf must contain a CRAN entry

Making the default a list means a user-supplied value replaces it rather than merging. Ordering cannot be expressed by merging, so replacement is the only coherent semantics — but it removes an accidental safety net. Workbench discards repos.conf entirely when nothing is named CRAN (SessionOptions.cpp:588), taking the admin's own repositories with it, and nothing in the rendered file shows this.

NOTES.txt now warns when the key is absent, in any form — map, list, or raw string — and stays quiet when the file is suppressed with repos.conf: null. The chart does not inject a CRAN entry, because injecting a public URL is wrong for air-gapped installs; the warning points at the fix instead, which is to name one of your own repositories CRAN.

Also fixed here

  • _helpers.tpl — with more than one config.pam file, the pam volumeMounts were emitted in Go map order (sprig keys without sortAlpha). Three renders of the identical command gave two different orderings, so helm template was not reproducible and the Deployment's pod template changed between renders with no configuration change. Now sorted.
  • README.md.gotmpl — the launcher.kubernetes.profiles.conf example claimed the [*] section's arrays are "appended" to user and group sections, and showed output with : separators and values that appear in no input. No such appending exists — only job-json-overrides is merged across sections — and profiles render with =. Corrected, along with a note that not merging is correct, since profiles are resolved by precedence.

Documentation

The list form, that only sections are ordered while options within them are still sorted, that repos.conf replaces rather than merges and must contain CRAN, and a new section explaining that r-versions is DCF.

Verification

  • 205 workbench tests pass, up from 173.
  • A guard test asserts that every file documented as a list ships no chart default, since a list replaces rather than merges. Verified it fails if a default is added to config.server.profiles. Its map-form half is derived from NOTES.txt's own list rather than restated: a map default for an order-sensitive file would make a default install warn.
  • Rendering is unchanged for every existing config across all values files under lint/ and ci/, except where a previously-broken shape now renders correctly. The lint/ fixtures are byte-identical to main — routing means the r-versions records they already contained now render valid DCF for the first time since 2021.
  • The CRAN guard verified against all seven shapes: list, map, and string with and without CRAN, plus null and a default install.

@khusmann
khusmann force-pushed the ordered-config-workbench branch from cd4bc04 to 84e924f Compare September 30, 2026 00:37

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant