[helm] Support a name-keyed map for dagster-user-deployments.deployments#34022
[helm] Support a name-keyed map for dagster-user-deployments.deployments#34022liadsh-twine wants to merge 1 commit into
Conversation
Greptile SummaryThis PR adds support for supplying
Confidence Score: 5/5Safe to merge; the array-form code path is structurally unchanged and the new map-form helper is well-tested with equivalence checks against the list form. All six template files that iterate over deployments receive the same mechanical substitution. The helper's merge semantics (dst-wins, deepCopy prevents mutation) are correct. The schema correctly enforces name for list entries via allOf while relaxing it for the map form. The equivalence test, which includes the config checksum annotation, confirms no regression for existing array users. No files require special attention; both helper copies are identical as documented by the sync comments.
|
| Filename | Overview |
|---|---|
| helm/dagster/charts/dagster-user-deployments/templates/helpers/_helpers.tpl | Adds dagsterUserDeployments.deploymentsList helper — correctly uses merge with dst-wins semantics and deepCopy to normalize maps into a list; identical copy in parent chart helpers is intentional per sync comment |
| helm/dagster/templates/helpers/_helpers.tpl | Parent-chart copy of the same helper; verbatim duplicate required so the template is available both when the subchart runs standalone and when rendered under the umbrella |
| helm/dagster/schema/schema/charts/dagster_user_deployments/subschema/user_deployments.py | Makes UserDeployment.name optional, introduces _RequireName annotation to re-add required: ["name"] only for the list branch via allOf — keeps schema tight for existing list users while supporting the new map form |
| helm/dagster/schema/schema_tests/test_user_deployments.py | Adds six map-form tests covering render, service, configmap-env, key-wins-over-name, list/map equivalence (including checksum), and the missing-name failure case; backstop template-fail path is not directly tested (see comment) |
| helm/dagster/charts/dagster-user-deployments/values.schema.json | deployments property correctly becomes anyOf[array, object]; array branch restores required: ["name"] via allOf so helm lint catches missing names; map branch allows name to be absent |
| helm/dagster/values.schema.json | Umbrella chart schema mirrors the subchart changes; anyOf with allOf+required for list form and additionalProperties for map form is correctly regenerated |
| helm/dagster/templates/configmap-workspace.yaml | Single-line change to normalize deployments through the helper before iterating; ternary evaluates both branches eagerly but that is pre-existing behavior |
| helm/dagster/schema/schema_tests/test_workspace.py | New test verifies workspace configmap renders correctly from map-form deployments with sorted-key ordering for grpc_server entries |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[dagsterUserDeployments.deploymentsList] --> B{kindIs map?}
B -- Yes / map form --> C[Initialize empty list]
C --> D[range name, deployment over map in sorted key order]
D --> E["merge dict(name=key) with deepCopy(deployment) - key wins over entry name"]
E --> F[append entry to list]
F --> D
D -- done --> G[emit list as YAML]
B -- No / list form --> H[range each deployment in list]
H --> I{deployment.name set?}
I -- No --> J[fail: each user deployment must set name]
I -- Yes --> H
H -- done --> K[emit list as YAML unchanged]
G --> L[fromYamlArray at call site produces canonical list]
K --> L
L --> M[range in deployment-user, service-user, configmap-env-user, configmap-workspace, daemon and webserver init containers]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[dagsterUserDeployments.deploymentsList] --> B{kindIs map?}
B -- Yes / map form --> C[Initialize empty list]
C --> D[range name, deployment over map in sorted key order]
D --> E["merge dict(name=key) with deepCopy(deployment) - key wins over entry name"]
E --> F[append entry to list]
F --> D
D -- done --> G[emit list as YAML]
B -- No / list form --> H[range each deployment in list]
H --> I{deployment.name set?}
I -- No --> J[fail: each user deployment must set name]
I -- Yes --> H
H -- done --> K[emit list as YAML unchanged]
G --> L[fromYamlArray at call site produces canonical list]
K --> L
L --> M[range in deployment-user, service-user, configmap-env-user, configmap-workspace, daemon and webserver init containers]
Reviews (2): Last reviewed commit: "[helm] Support a name-keyed map for dags..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
| }, | ||
| "required": [ | ||
| "name", | ||
| "image", |
There was a problem hiding this comment.
name for list-form entries
Removing name from the required array means helm lint and JSON-Schema validators will silently accept a list-form UserDeployment without a name. The only enforcement is the fail inside the Helm template helper, which runs only at helm template / helm install time. Before this PR, a schema validator would catch the missing field immediately; now it passes validation and only fails on render. Consider keeping the schema constraint tight by validating name conditionally — or at minimum noting in the error message that schema validation will not catch it.
There was a problem hiding this comment.
Addressed in the latest push. name now stays required for the list form: a _RequireName annotation overlays required: ["name"] onto just the array branch of the generated schema via allOf, reusing the single UserDeployment $def (no duplicate def). The map branch keeps name optional since the key supplies it. So a nameless list entry now fails at schema-validation / helm lint time again, not only at render. The template fail guard is kept as a backstop for when schema validation is skipped.
| port: 3030 | ||
| ``` | ||
|
|
||
| The map form is useful when you layer values files — for example a base file plus a per-environment override. Helm merges maps by key but replaces lists wholesale, so with the map form an override file can change a single field of a single deployment instead of restating the entire list. When the map form is used, the map key is the deployment name and the per-entry `name` field is optional. |
There was a problem hiding this comment.
Go templates iterate maps in sorted key order, so deployments in the map form are always rendered alphabetically by key — regardless of the order they appear in the YAML. This differs from the list form, where users control ordering explicitly. The init-container readiness probes, the workspace load_from list, and any other ordered downstream artifacts will all follow alphabetical order. Mentioning this helps users who care about ordering or who are migrating from a list and notice the order changed.
| The map form is useful when you layer values files — for example a base file plus a per-environment override. Helm merges maps by key but replaces lists wholesale, so with the map form an override file can change a single field of a single deployment instead of restating the entire list. When the map form is used, the map key is the deployment name and the per-entry `name` field is optional. | |
| The map form is useful when you layer values files — for example a base file plus a per-environment override. Helm merges maps by key but replaces lists wholesale, so with the map form an override file can change a single field of a single deployment instead of restating the entire list. When the map form is used, the map key is the deployment name and the per-entry `name` field is optional. Note that Helm renders map keys in alphabetical order, so deployments will always be emitted in sorted key order rather than declaration order. |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Good call — added a note that Helm renders map keys in alphabetical order, so deployments are emitted in sorted key order rather than declaration order.
| {{/* | ||
| Normalize the user-code deployments value into a list. | ||
| Accepts either a list of deployment entries or a map keyed by deployment name. | ||
| For the map form, the key becomes each entry's `name` (the key wins over any `name` field). | ||
| Emits YAML that call sites parse with `fromYamlArray`. | ||
| */}} | ||
| {{- define "dagsterUserDeployments.deploymentsList" -}} |
There was a problem hiding this comment.
This helper is defined identically in helm/dagster/templates/helpers/_helpers.tpl. Adding a cross-reference comment in both copies makes the intentional duplication visible and reminds future editors to keep both files in sync.
| {{/* | |
| Normalize the user-code deployments value into a list. | |
| Accepts either a list of deployment entries or a map keyed by deployment name. | |
| For the map form, the key becomes each entry's `name` (the key wins over any `name` field). | |
| Emits YAML that call sites parse with `fromYamlArray`. | |
| */}} | |
| {{- define "dagsterUserDeployments.deploymentsList" -}} | |
| {{/* | |
| Normalize the user-code deployments value into a list. | |
| Accepts either a list of deployment entries or a map keyed by deployment name. | |
| For the map form, the key becomes each entry's `name` (the key wins over any `name` field). | |
| Emits YAML that call sites parse with `fromYamlArray`. | |
| NOTE: this helper is intentionally duplicated verbatim in | |
| helm/dagster/templates/helpers/_helpers.tpl | |
| Keep both copies in sync when making changes. | |
| */}} | |
| {{- define "dagsterUserDeployments.deploymentsList" -}} |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Done — added a cross-reference note in both _helpers.tpl copies explaining the intentional duplication and to keep them in sync.
Allow `dagster-user-deployments.deployments` to be specified as a map keyed by deployment name, in addition to the existing array form. Helm merges maps by key but replaces arrays wholesale, so the map form lets layered values files (e.g. a base file plus per-environment overrides, as used with an ArgoCD ApplicationSet) override a single field of a single deployment instead of restating the entire list. A new `dagsterUserDeployments.deploymentsList` template helper normalizes the value (list or map) into a canonical list; when a map is used the key becomes the deployment name. Every existing `range` over deployments (three subchart templates and the parent chart's workspace configmap, daemon init containers, and webserver init containers) is routed through the helper, so array-form output is unchanged. The pydantic models are updated so `deployments` becomes `list[UserDeployment] | dict[str, UserDeployment]`. `name` is optional on the base model (the map key supplies it), but it stays required for the list form: a `_RequireName` annotation overlays `required: ["name"]` onto the array branch of the generated JSON schema via `allOf`, reusing the single `UserDeployment` $def, so a nameless list entry still fails schema validation / `helm lint`. Both values.schema.json files are regenerated via `dagster-helm schema apply`. Adds a commented map example to both values.yaml files and a docs section (noting map keys render in sorted order), plus pytest coverage for the map form (rendering, naming, array/map equivalence, key-wins-over-name, and the missing-name guard). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
29a3d8e to
47f4e05
Compare
|
Opened this PR to support #8172 |
1 participant
Summary & Motivation
The Dagster Helm chart only accepts
dagster-user-deployments.deploymentsas a YAML array. Helm merges arrays by replacement, not by key, so a basevalues.yamlplus per-environment overlays force you to duplicate every deployment entry in full for each environment.This PR makes
deploymentsaccept either the existing array or a map keyed by deployment name. The map form lets Helm deep-merge overlays by key — so an override file (e.g. a per-environment layer, as used with an ArgoCDApplicationSet) can change a single field of a single deployment instead of restating the whole list. Existing array users are unaffected.How it works
dagsterUserDeployments.deploymentsListnormalizes the value (list or map) into a canonical list. When a map is used, the map key becomes the deployment name (the per-entrynameis then optional; the key wins if both are present).rangeover deployments is routed through the helper — the three subchart templates (deployment-user,service-user,configmap-env-user) and the parent chart'sconfigmap-workspace,deployment-daemoninit containers, and webserver init containers._helpers.tpl(mirroring howdagster.fullname/dagster.labelsare already shared), so it's available whether the subchart is rendered via the umbrella or standalone — the subchart is conditional (condition: dagster-user-deployments.enableSubchart), so a helper defined only there would be unavailable to the parent when the subchart is disabled, and the parent's helpers are unavailable when the subchart is rendered on its own.Example
Schema
UserDeployments.deploymentsandDagsterUserDeploymentsHelmValues.deploymentsbecomelist[UserDeployment] | dict[str, UserDeployment], andUserDeployment.namebecomes optional (the map key supplies it; the templatesfailif a list entry omitsname). Bothvalues.schema.jsonfiles were regenerated withdagster-helm schema apply— thedeploymentsproperty now renders as ananyOfof array/object.Test Plan
helm lint helm/dagster --with-subcharts --strict— 0 failures.dagster-<key>, Services<key>, workspaceload_fromwithlocation_name=<key>, and daemon/webserver init containersinit-user-deployment-<key>.master.dagster-helm schema applyis idempotent (CI'sgit diff --exit-codegate passes).test_user_deployments.pyandtest_workspace.py: map rendering, deployment/service/configmap/workspace/daemon naming from keys, array↔map equivalence (including the config checksum annotation), key-wins-over-name, and a guard that a list entry withoutnamefails.ruffcheck + format clean.Changelog
The Helm chart's
dagster-user-deployments.deploymentsvalue now also accepts a map keyed by deployment name (in addition to a list), making it easier to override individual deployments across layered values files.