From b707e940f7d05acbf8924fcde97ff8783fb9f606 Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Fri, 13 Nov 2026 02:40:06 -0300 Subject: [PATCH 1/2] fix(features): keep JS property order in metadata, options and compose env The reference CLI writes these objects in JS property order (array-index keys ascending, then the others where each was first set); this CLI sorted them: - each entry's containerEnv in the devcontainer.metadata label, both a base image's entries as its label lists them and the configuration's as devcontainer.json declares it; - a Feature's value in read-configuration's featuresConfiguration; - the options of features resolve-dependencies' installOrder; - the environment of the compose override, which is the merged containerEnv, where a key keeps the position it first had (Object.assign over the base image, Features and configuration). An imagemeta.Entry now records its containerEnv's member order when it is decoded or built from the configuration, keeps it through host substitution, and MergeConfiguration carries the merged order. A Feature's value and the resolve-dependencies options are written through features.JSValue with the recorded option order. Co-Authored-By: Claude Opus 5.5 --- docs/DIVERGENCES.md | 7 +- docs/parity/parity-matrix.yaml | 55 +++++++++++++ internal/cli/build.go | 2 +- internal/cli/compose_override.go | 9 +- internal/cli/compose_override_test.go | 16 ++++ internal/cli/features_resolve_deps.go | 4 +- internal/cli/metadata_label.go | 6 +- internal/cli/metadata_label_test.go | 31 ++++++- internal/cli/read_configuration.go | 1 + internal/cli/up.go | 2 +- internal/features/jsobject.go | 18 ++++ internal/features/jsobject_test.go | 21 +++++ internal/features/types.go | 5 +- internal/features/types_order_test.go | 9 ++ internal/imagemeta/merge.go | 5 ++ internal/imagemeta/metadata.go | 42 ++++++++++ internal/imagemeta/metadata_test.go | 2 +- internal/imagemeta/reference_oracle_test.go | 12 +++ .../testdata/ts-oracle/imagemeta.json | 82 ++++++++++++++++++- tools/tsoracle/oracle.ts | 2 + 20 files changed, 310 insertions(+), 21 deletions(-) diff --git a/docs/DIVERGENCES.md b/docs/DIVERGENCES.md index 01bc17d..ede9010 100644 --- a/docs/DIVERGENCES.md +++ b/docs/DIVERGENCES.md @@ -117,9 +117,10 @@ touches a compared surface, reflected in the parity matrix. order (`pickFeatureProperties` for a Feature, `pickConfigProperties` for the configuration) and `&`, `<`, `>` unescaped, as in TS. Within a property's object value (a mount object, `customizations`, `portsAttributes`, …) the keys are sorted, because a - decoded Go map keeps no order, where TS keeps the order they were written in; an entry - given in another order (a base image's label written by another tool) is likewise - rewritten in pick order. The parsed metadata is the same; only the label's bytes differ + decoded Go map keeps no order, where TS keeps the order they were written in; + `containerEnv` alone keeps TS's order (the array-index keys ascending, then the others + as written). An entry given in another order (a base image's label written by another + tool) is likewise rewritten in pick order. The parsed metadata is the same; only the label's bytes differ (`internal/imagemeta/testdata/ts-oracle/imagemeta.json` records both cases). - **Command results write nested objects with their keys sorted.** TS writes each result with `JSON.stringify`, so properties come in insertion order. Go matches that order for diff --git a/docs/parity/parity-matrix.yaml b/docs/parity/parity-matrix.yaml index 0307aa9..c33eb74 100644 --- a/docs/parity/parity-matrix.yaml +++ b/docs/parity/parity-matrix.yaml @@ -199,6 +199,19 @@ initial_cases: notes: "readLockfile rethrows JSON.parse's SyntaxError and runAsyncHandler console.errors it: 'SyntaxError: Expected ',' or '}' after property value in JSON at position 15 (line 1 column 16)' on stderr, exit 1, nothing on stdout. Go used to ignore the lockfile and print the graph." # --- Wave A: new cases (some uncovered divergences, now fixed) --- + - id: features.resolve-dependencies-option-key-order-success + lane: contract + command: features + priority: p1 + docker_required: false + setup_cmd: "WORKSPACE=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $WORKSPACE; mkdir -p $WORKSPACE/.devcontainer/f $WORKSPACE/.devcontainer/g; printf '%s' '{\"id\":\"f\",\"version\":\"1.0.0\",\"dependsOn\":{\"./g\":{\"yy\":\"1\",\"3\":\"d\",\"bb\":\"2\"}}}' > $WORKSPACE/.devcontainer/f/devcontainer-feature.json; printf '%s' '{\"id\":\"g\",\"version\":\"1.0.0\"}' > $WORKSPACE/.devcontainer/g/devcontainer-feature.json; printf '%s' '{\"image\":\"alpine:3\",\"features\":{\"./f\":{\"zz\":\"1\",\"10\":\"t\",\"a\":\"b\",\"2\":\"s\"}}}' > $WORKSPACE/.devcontainer/devcontainer.json; echo WORKSPACE=$WORKSPACE" + cleanup_cmd: "rm -rf /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}" + ts_cmd: "features resolve-dependencies --workspace-folder ${WORKSPACE}" + asserts: [exit_code, stdout_text] + class: output-shape + current_status: match + notes: "installOrder's options are each Feature's value, the user's options object from devcontainer.json or the dependsOn options from devcontainer-feature.json, as JSON.parse built it: the array-index keys first, ascending, then the others as declared. Go sorted them." + - id: features-publish.missing-required lane: contract command: features-publish @@ -1135,6 +1148,20 @@ initial_cases: current_status: match notes: "TS getDevcontainerMetadata writes the Feature's raw values to the devcontainer.metadata label (imageMetadata.ts uses .raw), so a Feature mount and lifecycle command keep ${localEnv:HOME} and are resolved on the host that runs `up`. Go resolved the Feature's mounts before building the label, baking the build host's HOME into the image." + - id: build.metadata-label-container-env-order-success + lane: runtime + command: build + priority: p1 + docker_required: true + setup_cmd: "WORKSPACE=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; IMG=parity-env-base-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $WORKSPACE; mkdir -p $WORKSPACE/img $WORKSPACE/.devcontainer/f; printf 'FROM alpine:3\\nLABEL devcontainer.metadata=\\047[{\"containerEnv\":{\"ZB\":\"base\",\"10\":\"base\",\"AB\":\"base\"}}]\\047\\n' > $WORKSPACE/img/Dockerfile; docker build -q -t $IMG $WORKSPACE/img >/dev/null; printf '%s' '{\"id\":\"f\",\"version\":\"1.0.0\"}' > $WORKSPACE/.devcontainer/f/devcontainer-feature.json; printf '#!/bin/sh\\ntrue\\n' > $WORKSPACE/.devcontainer/f/install.sh; chmod +x $WORKSPACE/.devcontainer/f/install.sh; printf '%s' '{\"image\":\"'$IMG'\",\"containerEnv\":{\"ZED\":\"cfg\",\"AB\":\"cfg\",\"2\":\"cfg\",\"ALPHA\":\"cfg\"},\"features\":{\"./f\":{}}}' > $WORKSPACE/.devcontainer/devcontainer.json; echo WORKSPACE=$WORKSPACE" + ts_cmd: "build --workspace-folder ${WORKSPACE} --image-name parity-${PARITY_CASE_ID}-${PARITY_SIDE}" + verify_cmd: "docker image inspect -f '{{index .Config.Labels \"devcontainer.metadata\"}}' ${IMAGE_NAME}" + cleanup_cmd: "docker rmi -f parity-${PARITY_CASE_ID}-${PARITY_SIDE} parity-env-base-${PARITY_CASE_ID}-${PARITY_SIDE} >/dev/null 2>&1; rm -rf /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; true" + asserts: [exit_code, stdout_normalized] + class: build-runtime + current_status: match + notes: "TS getDevcontainerMetadata writes the base image's entries as JSON.parse read them from its label and the config's raw containerEnv as devcontainer.json declares it, so each containerEnv keeps JS property order (the array-index keys 2 and 10 first, ascending, then the others as declared). Go sorted each entry's containerEnv (10 before 2, AB before ZB, ALPHA before ZED)." + - id: up.compose-feature-host-variables-raw-label-success lane: runtime command: up @@ -1149,6 +1176,20 @@ initial_cases: current_status: match notes: "The compose service's image and container labels keep the Feature's ${localEnv:HOME} raw, as TS writes them; the bind mount the container gets is resolved on this host (TS getDevcontainerMetadata's featureRaw.map(substitute)). Go resolved the Feature's mounts before building the label." + - id: up.compose-container-env-order-success + lane: runtime + command: up + priority: p1 + docker_required: true + setup_cmd: "WORKSPACE=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; IMG=parity-env-base-${PARITY_CASE_ID}-${PARITY_SIDE}; USER_DATA_FOLDER=$WORKSPACE/user-data; rm -rf $WORKSPACE; mkdir -p $WORKSPACE/img $WORKSPACE/.devcontainer/f $USER_DATA_FOLDER; printf 'FROM alpine:3\\nLABEL devcontainer.metadata=\\047[{\"containerEnv\":{\"ZB\":\"base\",\"10\":\"base\",\"AB\":\"base\"}}]\\047\\n' > $WORKSPACE/img/Dockerfile; docker build -q -t $IMG $WORKSPACE/img >/dev/null; printf '%s' '{\"id\":\"f\",\"version\":\"1.0.0\"}' > $WORKSPACE/.devcontainer/f/devcontainer-feature.json; printf '#!/bin/sh\\ntrue\\n' > $WORKSPACE/.devcontainer/f/install.sh; chmod +x $WORKSPACE/.devcontainer/f/install.sh; printf 'services:\\n app:\\n image: %s\\n command: sleep infinity\\n' $IMG > $WORKSPACE/.devcontainer/docker-compose.yml; printf '%s' '{\"dockerComposeFile\":\"docker-compose.yml\",\"service\":\"app\",\"workspaceFolder\":\"/\",\"containerEnv\":{\"ZED\":\"cfg\",\"AB\":\"cfg\",\"2\":\"cfg\",\"ALPHA\":\"cfg\"},\"features\":{\"./f\":{}}}' > $WORKSPACE/.devcontainer/devcontainer.json; echo WORKSPACE=$WORKSPACE; echo USER_DATA_FOLDER=$USER_DATA_FOLDER" + ts_cmd: "up --workspace-folder ${WORKSPACE} --skip-post-create --update-remote-user-uid-default never --id-label parity.case=${PARITY_CASE_ID}-${PARITY_SIDE} --user-data-folder ${USER_DATA_FOLDER}" + verify_cmd: "grep -A6 '^ environment:' ${USER_DATA_FOLDER}/docker-compose/docker-compose.devcontainer.containerFeatures-*; docker inspect -f '{{index .Config.Labels \"devcontainer.metadata\"}}' ${CONTAINER_ID}; docker image inspect -f '{{index .Config.Labels \"devcontainer.metadata\"}}' $(docker inspect -f '{{.Image}}' ${CONTAINER_ID})" + cleanup_cmd: "docker compose --project-name ${COMPOSE_PROJECT_NAME} -f ${WORKSPACE}/.devcontainer/docker-compose.yml down -t 0 >/dev/null 2>&1; docker rmi -f parity-env-base-${PARITY_CASE_ID}-${PARITY_SIDE} >/dev/null 2>&1; rm -rf /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; true" + asserts: [exit_code, stdout_normalized] + class: up-runtime + current_status: match + notes: "TS writes the compose override's environment from the merged containerEnv (mergeConfiguration's Object.assign over the base image's label, the Features and the config): a key keeps the position it first had, the array-index keys 2 and 10 first, and the value of the last entry that sets it. The container and image labels keep each entry's containerEnv in JS property order. Go sorted the environment and each label entry's containerEnv." + - id: read-configuration.feature-host-variables-merged-success lane: runtime command: read-configuration @@ -1396,6 +1437,20 @@ initial_cases: current_status: match notes: "A local Feature entry is devcontainer-feature.json as JSON.parse reads it, every property kept (keywords, x, empty, an empty option), a repeated one at its first position with its last value, the array-index properties 2 and 10 first, numbers as JS writes them (1.5), and the options' properties in the file's order; then name, value, included, cachePath and consecutiveId (processFeatureIdentifier, fetchFeatures). updateFromOldProperties (containerFeaturesConfiguration.ts) moves extensions and settings into customizations.vscode, settings spread under vscode's. Go used to write only the properties it has fields for, in the struct's order, with options sorted and the deprecated properties dropped." + - id: read-configuration.features-configuration-value-key-order-success + lane: contract + command: read-configuration + priority: p1 + docker_required: false + setup_cmd: "WORKSPACE=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $WORKSPACE; mkdir -p $WORKSPACE/.devcontainer/f; printf '%s' '{\"id\":\"f\",\"version\":\"1.0.0\",\"options\":{\"a\":{\"type\":\"string\",\"default\":\"z\"}}}' > $WORKSPACE/.devcontainer/f/devcontainer-feature.json; printf '%s\\n' '#!/bin/sh' 'true' > $WORKSPACE/.devcontainer/f/install.sh; printf '%s' '{\"image\":\"alpine:3\",\"features\":{\"./f\":{\"zz\":\"1\",\"10\":\"t\",\"a\":\"b\",\"2\":\"s\"}}}' > $WORKSPACE/.devcontainer/devcontainer.json; echo WORKSPACE=$WORKSPACE" + cleanup_cmd: "rm -rf /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}" + ts_cmd: "read-configuration --workspace-folder ${WORKSPACE} --include-features-configuration" + key_order: ["featuresConfiguration.featureSets.0.features.0.value"] + asserts: [exit_code, stdout_normalized] + class: output-shape + current_status: match + notes: "A Feature's value is the user's options object from devcontainer.json as JSON.parse built it, so it keeps JS property order: the array-index keys 2 and 10 first, ascending, then zz and a as declared. Go sorted it (10, 2, a, zz)." + - id: read-configuration.features-configuration-oci-feature-key-order-success lane: runtime command: read-configuration diff --git a/internal/cli/build.go b/internal/cli/build.go index 6b794b6..13b395f 100644 --- a/internal/cli/build.go +++ b/internal/cli/build.go @@ -186,7 +186,7 @@ func runBuild(ctx context.Context, out Output, opts *buildOpts) error { defer engine.Close() run := &buildRunner{log: logger, docker: dockerClient, engine: engine, opts: opts} - if run.configLabel, err = configLabelEntry(loadResult.RawConfig, false); err != nil { + if run.configLabel, err = configLabelEntry(loadResult.RawConfig, loadResult.Config.ContainerEnvOrder, false); err != nil { return writeErrorResult(out, err.Error()) } diff --git a/internal/cli/compose_override.go b/internal/cli/compose_override.go index 9155aeb..ff6d90d 100644 --- a/internal/cli/compose_override.go +++ b/internal/cli/compose_override.go @@ -6,9 +6,9 @@ import ( "fmt" "os" "regexp" - "slices" "strings" + "github.com/devcontainers/cli/internal/features" "github.com/devcontainers/cli/internal/imagemeta" ) @@ -212,12 +212,7 @@ func generateComposeFeaturesOverride(in composeFeaturesOverride) string { } if len(merged.ContainerEnv) > 0 { b.WriteString("\n environment:") - keys := make([]string, 0, len(merged.ContainerEnv)) - for k := range merged.ContainerEnv { - keys = append(keys, k) - } - slices.Sort(keys) - for _, k := range keys { + for _, k := range features.PropertyOrder(merged.ContainerEnv, merged.ContainerEnvOrder) { v := strings.ReplaceAll(merged.ContainerEnv[k], "\n", `\n`) v = escapeComposeDollar(v) v = strings.ReplaceAll(v, "'", "''") diff --git a/internal/cli/compose_override_test.go b/internal/cli/compose_override_test.go index a4febc8..2ed27cf 100644 --- a/internal/cli/compose_override_test.go +++ b/internal/cli/compose_override_test.go @@ -215,3 +215,19 @@ func TestComposeFeaturesBuild(t *testing.T) { t.Errorf("label-only content = %q, want %q", label.DockerfileContent, want) } } + +func TestGenerateComposeFeaturesOverrideWritesTheMergedEnvInPropertyOrder(t *testing.T) { + entries := imagemeta.ReadMetadataFromLabels(map[string]string{ + imagemeta.MetadataLabel: `[{"containerEnv":{"ZB":"base","10":"base","AB":"base"}}]`, + }, nil) + entries = append(entries, imagemeta.Entry{ + ContainerEnv: map[string]string{"ZED": "cfg", "AB": "cfg", "2": "cfg", "ALPHA": "cfg"}, + ContainerEnvOrder: []string{"ZED", "AB", "2", "ALPHA"}, + }) + got := generateComposeFeaturesOverride(composeFeaturesOverride{Service: "app", Merged: imagemeta.MergeConfiguration(entries)}) + _, env, _ := strings.Cut(got, " environment:\n") + want := " - '2=cfg'\n - '10=base'\n - 'ZB=base'\n - 'AB=cfg'\n - 'ZED=cfg'\n - 'ALPHA=cfg'\n" + if env != want { + t.Errorf("environment:\n%s\nwant:\n%s", env, want) + } +} diff --git a/internal/cli/features_resolve_deps.go b/internal/cli/features_resolve_deps.go index 9ce5ce9..059c26a 100644 --- a/internal/cli/features_resolve_deps.go +++ b/internal/cli/features_resolve_deps.go @@ -81,7 +81,9 @@ func realFeaturesResolveDepsCmd() *cobra.Command { } var opts any if len(fs.Features) > 0 { - opts = fs.Features[0].Value + if opts, err = features.JSValue(fs.Features[0].Value, fs.Features[0].ValueOrder); err != nil { + return err + } } entries = append(entries, installEntry{ID: id, Options: opts}) } diff --git a/internal/cli/metadata_label.go b/internal/cli/metadata_label.go index 8f05cf5..e2741e5 100644 --- a/internal/cli/metadata_label.go +++ b/internal/cli/metadata_label.go @@ -17,7 +17,7 @@ var labelConfigProperties = []string{ "otherPortsAttributes", "forwardPorts", "shutdownAction", "updateRemoteUserUID", "hostRequirements", } -func configLabelEntry(raw map[string]any, omitRemoteEnv bool) (imagemeta.Entry, error) { +func configLabelEntry(raw map[string]any, envOrder []string, omitRemoteEnv bool) (imagemeta.Entry, error) { picked := map[string]any{} for _, k := range labelConfigProperties { if v, ok := raw[k]; ok && (!omitRemoteEnv || k != "remoteEnv") { @@ -32,6 +32,7 @@ func configLabelEntry(raw map[string]any, omitRemoteEnv bool) (imagemeta.Entry, if err != nil { return imagemeta.Entry{}, fmt.Errorf("devcontainer.metadata entry of the configuration: %w", err) } + entry.ContainerEnvOrder = envOrder return entry, nil } @@ -75,6 +76,9 @@ func hostMetadataSubstitution(hostSub config.HostSubContext, idLabels []string) if err != nil { return err } + for i := range resolved { + resolved[i].ContainerEnvOrder = (*entries)[i].ContainerEnvOrder + } *entries = resolved return nil } diff --git a/internal/cli/metadata_label_test.go b/internal/cli/metadata_label_test.go index 8cae016..8da13c1 100644 --- a/internal/cli/metadata_label_test.go +++ b/internal/cli/metadata_label_test.go @@ -25,7 +25,7 @@ func TestConfigLabelEntryPicksTheRawConfig(t *testing.T) { }`), &raw); err != nil { t.Fatal(err) } - entry, err := configLabelEntry(raw, false) + entry, err := configLabelEntry(raw, nil, false) if err != nil { t.Fatal(err) } @@ -34,14 +34,14 @@ func TestConfigLabelEntryPicksTheRawConfig(t *testing.T) { if string(got) != want { t.Errorf("entry = %s\nwant %s", got, want) } - omitted, err := configLabelEntry(raw, true) + omitted, err := configLabelEntry(raw, nil, true) if err != nil { t.Fatal(err) } if omitted.RemoteEnv != nil { t.Errorf("remoteEnv kept with omitRemoteEnv: %v", omitted.RemoteEnv) } - if _, err := configLabelEntry(map[string]any{"init": "yes"}, false); err == nil { + if _, err := configLabelEntry(map[string]any{"init": "yes"}, nil, false); err == nil { t.Error("an init that is no boolean: want an error") } } @@ -164,3 +164,28 @@ func TestHostMetadataSubstitutionLeavesTheRawEntriesRaw(t *testing.T) { } } } + +func TestMetadataEntriesKeepTheContainerEnvOrder(t *testing.T) { + raw := map[string]any{"containerEnv": map[string]any{"ZED": "1", "10": "2", "ALPHA": "${localEnv:U}", "2": "4"}} + order := []string{"ZED", "10", "ALPHA", "2"} + entry, err := configLabelEntry(raw, order, false) + if err != nil { + t.Fatal(err) + } + want := `[{"containerEnv":{"2":"4","10":"2","ZED":"1","ALPHA":"${localEnv:U}"}}]` + if got := containerMetadataLabel([]imagemeta.Entry{entry}); got != want { + t.Errorf("label = %s\nwant %s", got, want) + } + entries := []imagemeta.Entry{entry} + if err := hostMetadataSubstitution(config.HostSubContext{Env: map[string]string{"U": "me"}}, nil)(&entries); err != nil { + t.Fatal(err) + } + want = `[{"containerEnv":{"2":"4","10":"2","ZED":"1","ALPHA":"me"}}]` + if got := containerMetadataLabel(entries); got != want { + t.Errorf("substituted label = %s\nwant %s", got, want) + } + cfg := &config.DevContainer{ContainerEnv: map[string]string{"ZED": "1", "ALPHA": "3"}, ContainerEnvOrder: []string{"ZED", "ALPHA"}} + if got, want := containerMetadataLabel([]imagemeta.Entry{configMetadataEntry(cfg, false)}), `[{"containerEnv":{"ZED":"1","ALPHA":"3"}}]`; got != want { + t.Errorf("config entry label = %s\nwant %s", got, want) + } +} diff --git a/internal/cli/read_configuration.go b/internal/cli/read_configuration.go index a00b22e..70e3b2f 100644 --- a/internal/cli/read_configuration.go +++ b/internal/cli/read_configuration.go @@ -483,6 +483,7 @@ func configMetadataEntry(cfg *config.DevContainer, omitRemoteEnv bool) imagemeta ShutdownAction: cfg.ShutdownAction, UpdateRemoteUserUID: cfg.UpdateRemoteUserUID, ContainerEnv: cfg.ContainerEnv, + ContainerEnvOrder: cfg.ContainerEnvOrder, CapAdd: cfg.CapAdd, SecurityOpt: cfg.SecurityOpt, Mounts: metadataMounts(cfg), diff --git a/internal/cli/up.go b/internal/cli/up.go index b230f6a..ccb210b 100644 --- a/internal/cli/up.go +++ b/internal/cli/up.go @@ -315,7 +315,7 @@ func runUp(ctx context.Context, out Output, opts *upOpts) error { } run.idLabels = idLabels run.substitute = hostMetadataSubstitution(loadResult.HostSub, idLabels) - if run.configLabel, err = configLabelEntry(loadResult.RawConfig, opts.omitConfigRemoteEnvFromMeta); err != nil { + if run.configLabel, err = configLabelEntry(loadResult.RawConfig, loadResult.Config.ContainerEnvOrder, opts.omitConfigRemoteEnvFromMeta); err != nil { return writeErrorResult(out, err.Error()) } diff --git a/internal/features/jsobject.go b/internal/features/jsobject.go index 2f94401..089c66b 100644 --- a/internal/features/jsobject.go +++ b/internal/features/jsobject.go @@ -179,6 +179,24 @@ func toJS(v any) (any, error) { return parseJS(data) } +// JSValue is toJS of v, with the keys of a map v in +// PropertyOrder(v, declared). +func JSValue(v any, declared []string) (any, error) { + m, ok := v.(map[string]any) + if !ok { + return toJS(v) + } + obj := newJSObject() + for _, k := range PropertyOrder(m, declared) { + js, err := toJS(m[k]) + if err != nil { + return nil, err + } + obj.set(k, js) + } + return obj, nil +} + func jsTruthy(v any) bool { switch x := v.(type) { case nil: diff --git a/internal/features/jsobject_test.go b/internal/features/jsobject_test.go index e6b314a..014e846 100644 --- a/internal/features/jsobject_test.go +++ b/internal/features/jsobject_test.go @@ -64,3 +64,24 @@ func TestPropertyOrderListsKeysAsObjectKeys(t *testing.T) { t.Errorf("PropertyOrder of an empty map = %v", got) } } + +func TestJSValueOrdersAMapsKeys(t *testing.T) { + for _, tc := range []struct { + v any + order []string + want string + }{ + {map[string]any{"zz": "1", "10": "t", "a": map[string]any{"y": 1, "b": 2}, "2": "s", "m": nil}, []string{"zz", "gone", "10", "a", "2"}, `{"2":"s","10":"t","zz":"1","a":{"b":2,"y":1},"m":null}`}, + {"latest", []string{"x"}, `"latest"`}, + {true, nil, `true`}, + {nil, nil, `null`}, + } { + js, err := JSValue(tc.v, tc.order) + if err != nil { + t.Fatal(err) + } + if got, err := json.Marshal(js); err != nil || string(got) != tc.want { + t.Errorf("JSValue(%v, %v) = %s, %v; want %s", tc.v, tc.order, got, err, tc.want) + } + } +} diff --git a/internal/features/types.go b/internal/features/types.go index 74cff4f..f1df351 100644 --- a/internal/features/types.go +++ b/internal/features/types.go @@ -67,17 +67,18 @@ func (f Feature) MarshalJSON() ([]byte, error) { feature := newJSObject() for _, k := range f.SourceKeys { var v any + var order []string switch k { case "id": v = f.ID case "name": v = f.Name case "value": - v = f.Value + v, order = f.Value, f.ValueOrder case "included": v = f.Included } - js, err := toJS(v) + js, err := JSValue(v, order) if err != nil { return nil, err } diff --git a/internal/features/types_order_test.go b/internal/features/types_order_test.go index 942e6ac..61d0b58 100644 --- a/internal/features/types_order_test.go +++ b/internal/features/types_order_test.go @@ -87,3 +87,12 @@ func TestFeatureMarshalJSONMovesDeprecatedProperties(t *testing.T) { t.Error("extensions concatenated onto a number, which has no concat: want an error") } } + +func TestFeatureMarshalJSONWritesTheValueInPropertyOrder(t *testing.T) { + f := Feature{ID: "x", Included: true, SourceKeys: LocalFeatureKeys, + Value: map[string]any{"zz": "1", "10": "t", "a": true, "2": "s"}, ValueOrder: []string{"zz", "10", "a", "2"}} + got, err := json.Marshal(f) + if want := `{"id":"x","name":"","value":{"2":"s","10":"t","zz":"1","a":true},"included":true}`; err != nil || string(got) != want { + t.Errorf("got %s, %v\nwant %s", got, err, want) + } +} diff --git a/internal/imagemeta/merge.go b/internal/imagemeta/merge.go index c8729ac..7189df4 100644 --- a/internal/imagemeta/merge.go +++ b/internal/imagemeta/merge.go @@ -11,6 +11,8 @@ import ( "strconv" "strings" "unicode/utf16" + + "github.com/devcontainers/cli/internal/features" ) type MergedConfig struct { @@ -32,6 +34,7 @@ type MergedConfig struct { OtherPortsAttributes any `json:"otherPortsAttributes,omitempty"` ForwardPorts []any `json:"forwardPorts,omitempty"` ContainerEnv map[string]string `json:"containerEnv"` + ContainerEnvOrder []string `json:"-"` RemoteEnv map[string]*string `json:"remoteEnv"` Customizations map[string][]any `json:"customizations,omitempty"` Mounts []any `json:"mounts,omitempty"` @@ -83,6 +86,7 @@ func MergeConfiguration(entries []Entry) *MergedConfig { m.Mounts = append(m.Mounts, e.Mounts...) maps.Copy(m.ContainerEnv, e.ContainerEnv) + m.ContainerEnvOrder = append(m.ContainerEnvOrder, features.PropertyOrder(e.ContainerEnv, e.ContainerEnvOrder)...) maps.Copy(m.RemoteEnv, e.RemoteEnv) for k, v := range e.Customizations { m.Customizations[k] = append(m.Customizations[k], v) @@ -130,6 +134,7 @@ func MergeConfiguration(entries []Entry) *MergedConfig { } } + m.ContainerEnvOrder = features.KeyOrder(m.ContainerEnvOrder) m.Mounts = dedupeMountsByTarget(m.Mounts) m.ForwardPorts = dedupeForwardPorts(m.ForwardPorts) m.HostRequirements = mergeHostRequirements(entries) diff --git a/internal/imagemeta/metadata.go b/internal/imagemeta/metadata.go index 9c708c4..12df75f 100644 --- a/internal/imagemeta/metadata.go +++ b/internal/imagemeta/metadata.go @@ -6,6 +6,8 @@ import ( "fmt" "slices" + "github.com/devcontainers/cli/internal/features" + "github.com/devcontainers/cli/internal/jsonc" "github.com/devcontainers/cli/internal/log" ) @@ -19,6 +21,7 @@ type Entry struct { SecurityOpt []string `json:"securityOpt,omitempty"` Mounts []any `json:"mounts,omitempty"` ContainerEnv map[string]string `json:"containerEnv,omitempty"` + ContainerEnvOrder []string `json:"-"` ContainerUser string `json:"containerUser,omitempty"` RemoteEnv map[string]*string `json:"remoteEnv,omitempty"` RemoteUser string `json:"remoteUser,omitempty"` @@ -65,6 +68,13 @@ func (e Entry) MarshalJSON() ([]byte, error) { if err := json.Unmarshal(data.Bytes(), &fields); err != nil { return nil, err } + if _, ok := fields["containerEnv"]; ok { + env, err := containerEnvJSON(e.ContainerEnv, e.ContainerEnvOrder) + if err != nil { + return nil, err + } + fields["containerEnv"] = env + } order := configEntryKeys if e.ID != "" { order = append(slices.Clone(featureEntryKeys), configEntryKeys...) @@ -88,6 +98,38 @@ func (e Entry) MarshalJSON() ([]byte, error) { return out.Bytes(), nil } +func containerEnvJSON(env map[string]string, order []string) (json.RawMessage, error) { + var out bytes.Buffer + enc := json.NewEncoder(&out) + enc.SetEscapeHTML(false) + out.WriteByte('{') + for i, k := range features.PropertyOrder(env, order) { + if i > 0 { + out.WriteByte(',') + } + if err := enc.Encode(k); err != nil { + return nil, err + } + out.Truncate(out.Len() - 1) + out.WriteByte(':') + if err := enc.Encode(env[k]); err != nil { + return nil, err + } + out.Truncate(out.Len() - 1) + } + out.WriteByte('}') + return out.Bytes(), nil +} + +func (e *Entry) UnmarshalJSON(data []byte) error { + type plain Entry + if err := json.Unmarshal(data, (*plain)(e)); err != nil { + return err + } + e.ContainerEnvOrder = jsonc.MemberNames(data, "containerEnv") + return nil +} + func ReadMetadataFromLabels(labels map[string]string, logger log.Logger) []Entry { raw, ok := labels[MetadataLabel] if !ok || raw == "" { diff --git a/internal/imagemeta/metadata_test.go b/internal/imagemeta/metadata_test.go index b2f5189..776ea97 100644 --- a/internal/imagemeta/metadata_test.go +++ b/internal/imagemeta/metadata_test.go @@ -26,7 +26,7 @@ func TestEntryKeyOrdersNameEveryProperty(t *testing.T) { typ := reflect.TypeFor[Entry]() for i := range typ.NumField() { name, _, _ := strings.Cut(typ.Field(i).Tag.Get("json"), ",") - if name != "id" && !slices.Contains(configEntryKeys, name) { + if name != "id" && name != "-" && !slices.Contains(configEntryKeys, name) { t.Errorf("property %q is in no key order", name) } } diff --git a/internal/imagemeta/reference_oracle_test.go b/internal/imagemeta/reference_oracle_test.go index 404cc84..5b86ab0 100644 --- a/internal/imagemeta/reference_oracle_test.go +++ b/internal/imagemeta/reference_oracle_test.go @@ -41,6 +41,18 @@ func TestTSOracleImageMetadata(t *testing.T) { } return merged, nil }, + "mergeConfigurationContainerEnv": func(args []json.RawMessage) (any, error) { + var entries []Entry + if err := oracletest.Args(args, &entries); err != nil { + return nil, err + } + merged := MergeConfiguration(entries) + pairs := [][2]string{} + for _, k := range features.PropertyOrder(merged.ContainerEnv, merged.ContainerEnvOrder) { + pairs = append(pairs, [2]string{k, merged.ContainerEnv[k]}) + } + return pairs, nil + }, "getDevcontainerMetadataLabel": func(args []json.RawMessage) (any, error) { var entries []Entry if err := oracletest.Args(args, &entries); err != nil { diff --git a/internal/imagemeta/testdata/ts-oracle/imagemeta.json b/internal/imagemeta/testdata/ts-oracle/imagemeta.json index 046ec96..665d6e4 100644 --- a/internal/imagemeta/testdata/ts-oracle/imagemeta.json +++ b/internal/imagemeta/testdata/ts-oracle/imagemeta.json @@ -1,5 +1,5 @@ { - "comment": "imageMetadata.ts mergeConfiguration (with an empty config) and getDevcontainerMetadataLabel, commonUtils.ts getEntPasswdShellCommand, containerFeaturesConfiguration.ts generateContainerEnvs, getFeatureValueObject and escapeQuotesForShell, containerFeatures.ts getSafeId; ported in merge.go and extend.go.", + "comment": "imageMetadata.ts mergeConfiguration (with an empty config, and its containerEnv entries) and getDevcontainerMetadataLabel, commonUtils.ts getEntPasswdShellCommand, containerFeaturesConfiguration.ts generateContainerEnvs, getFeatureValueObject and escapeQuotesForShell, containerFeatures.ts getSafeId; ported in merge.go and extend.go.", "cases": [ { "name": "ts: remoteEnv from features and devcontainer.json", @@ -1552,6 +1552,58 @@ "portsAttributes": {} } }, + { + "name": "containerEnv keeps a key's first position", + "fn": "mergeConfigurationContainerEnv", + "args": [ + [ + { + "containerEnv": { + "ZB": "base", + "10": "base", + "AB": "base" + } + }, + { + "id": "f" + }, + { + "containerEnv": { + "ZED": "cfg", + "AB": "cfg", + "2": "cfg", + "ALPHA": "cfg" + } + } + ] + ], + "result": [ + [ + "2", + "cfg" + ], + [ + "10", + "base" + ], + [ + "ZB", + "base" + ], + [ + "AB", + "cfg" + ], + [ + "ZED", + "cfg" + ], + [ + "ALPHA", + "cfg" + ] + ] + }, { "name": "ts: label for Dockerfile", "fn": "getDevcontainerMetadataLabel", @@ -1811,6 +1863,34 @@ ], "result": "LABEL devcontainer.metadata=\"[ \\\n{\\\"postCreateCommand\\\":\\\"p\\\",\\\"waitFor\\\":\\\"onCreateCommand\\\",\\\"customizations\\\":{\\\"vscode\\\":{}},\\\"mounts\\\":[\\\"type=bind,src=/a,dst=/b\\\"],\\\"containerEnv\\\":{\\\"A\\\":\\\"1\\\"},\\\"containerUser\\\":\\\"c\\\",\\\"init\\\":true,\\\"privileged\\\":true,\\\"capAdd\\\":[\\\"NET_ADMIN\\\"],\\\"securityOpt\\\":[\\\"label=disable\\\"],\\\"remoteUser\\\":\\\"r\\\",\\\"userEnvProbe\\\":\\\"none\\\",\\\"remoteEnv\\\":{\\\"B\\\":\\\"2\\\"},\\\"overrideCommand\\\":false,\\\"portsAttributes\\\":{\\\"3000\\\":{\\\"label\\\":\\\"x\\\"}},\\\"otherPortsAttributes\\\":{\\\"onAutoForward\\\":\\\"ignore\\\"},\\\"forwardPorts\\\":[3000],\\\"shutdownAction\\\":\\\"none\\\",\\\"updateRemoteUserUID\\\":true,\\\"hostRequirements\\\":{\\\"cpus\\\":2}} \\\n]\"" }, + { + "name": "containerEnv in each entry's property order", + "fn": "getDevcontainerMetadataLabel", + "args": [ + [ + { + "containerEnv": { + "ZB": "1", + "10": "b", + "AB": "2", + "2": "a" + } + }, + { + "id": "f" + }, + { + "containerEnv": { + "ZED": "1", + "10": "x", + "ALPHA": "2", + "2": "y" + } + } + ] + ], + "result": "LABEL devcontainer.metadata=\"[ \\\n{\\\"containerEnv\\\":{\\\"2\\\":\\\"a\\\",\\\"10\\\":\\\"b\\\",\\\"ZB\\\":\\\"1\\\",\\\"AB\\\":\\\"2\\\"}}, \\\n{\\\"id\\\":\\\"f\\\"}, \\\n{\\\"containerEnv\\\":{\\\"2\\\":\\\"y\\\",\\\"10\\\":\\\"x\\\",\\\"ZED\\\":\\\"1\\\",\\\"ALPHA\\\":\\\"2\\\"}} \\\n]\"" + }, { "name": "getEntPasswd 'root'", "fn": "getEntPasswdShellCommand", diff --git a/tools/tsoracle/oracle.ts b/tools/tsoracle/oracle.ts index e4a0194..3358472 100644 --- a/tools/tsoracle/oracle.ts +++ b/tools/tsoracle/oracle.ts @@ -61,6 +61,8 @@ const fns: Record any> = { supportsBuildContexts: (df: string) => supportsBuildContexts(extractDockerfile(df)), // spec-node/imageMetadata.ts mergeConfiguration: (config: any, metadata: any[]) => mergeConfiguration(config, metadata), + // The containerEnv's key order is the compose environment's line order: entries keep it. + mergeConfigurationContainerEnv: (metadata: any[]) => Object.entries(mergeConfiguration({} as any, metadata).containerEnv || {}), getDevcontainerMetadataLabel: (raw: any[]) => getDevcontainerMetadataLabel({ config: raw, raw, substitute: (x: any) => x }), // spec-configuration/containerFeaturesConfiguration.ts generateContainerEnvs: (env: any, escapeDollar: boolean) => generateContainerEnvs(env ?? undefined, escapeDollar), From f2eda735982df186384cb166694fece8b52b3f4e Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Fri, 13 Nov 2026 03:05:25 -0300 Subject: [PATCH 2/2] refactor(imagemeta): leave the merged containerEnv order to PropertyOrder Every reader of MergedConfig.ContainerEnvOrder goes through features.PropertyOrder, which already keeps a key's first position, so deduplicating it in MergeConfiguration had no effect any test could see. Co-Authored-By: Claude Opus 5.5 --- internal/imagemeta/merge.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/imagemeta/merge.go b/internal/imagemeta/merge.go index 7189df4..9932298 100644 --- a/internal/imagemeta/merge.go +++ b/internal/imagemeta/merge.go @@ -134,7 +134,6 @@ func MergeConfiguration(entries []Entry) *MergedConfig { } } - m.ContainerEnvOrder = features.KeyOrder(m.ContainerEnvOrder) m.Mounts = dedupeMountsByTarget(m.Mounts) m.ForwardPorts = dedupeForwardPorts(m.ForwardPorts) m.HostRequirements = mergeHostRequirements(entries)