From 8c220fcbca0ea84781eea59e2537c38b7fa8ba3d Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Fri, 13 Nov 2026 17:22:34 -0300 Subject: [PATCH] fix(cli): keep JS property order in configuration output and docker run read-configuration, up and set-up wrote configuration's containerEnv, features and each Feature's options, and mergedConfiguration's containerEnv, with sorted keys. TS writes them as JSON.parse and mergeConfiguration built them: integer-like keys ascending, then the others in declaration order. up passed the merged containerEnv to docker run as -e flags in Go map order, which changed between runs; TS passes them in Object.keys order. Co-Authored-By: Claude Opus 5.5 --- docs/parity/parity-matrix.yaml | 49 ++++++++++++++++++++ internal/cli/read_configuration.go | 1 + internal/cli/read_configuration_held_test.go | 2 +- internal/cli/result_order.go | 27 +++++++++++ internal/cli/result_order_test.go | 23 +++++++++ internal/cli/up.go | 7 ++- 6 files changed, 106 insertions(+), 3 deletions(-) diff --git a/docs/parity/parity-matrix.yaml b/docs/parity/parity-matrix.yaml index 692cb4c..5963d05 100644 --- a/docs/parity/parity-matrix.yaml +++ b/docs/parity/parity-matrix.yaml @@ -1516,6 +1516,55 @@ initial_cases: 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.configuration-container-env-and-features-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; printf '%s' '{\"image\":\"alpine:3\",\"containerEnv\":{\"ZED\":\"1\",\"10\":\"2\",\"ALPHA\":\"3\",\"2\":\"4\"},\"features\":{\"zeta\":{\"zz\":\"1\",\"10\":\"t\",\"a\":\"b\",\"2\":\"s\"},\"10\":{},\"alpha\":{}}}' > $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}" + key_order: ["configuration.containerEnv", "configuration.features", "configuration.features.zeta"] + asserts: [exit_code, stdout_normalized] + class: output-shape + current_status: match + notes: "configuration is the devcontainer.json object as JSON.parse built it, so containerEnv, features and each Feature's options keep JS property order: the array-index keys 2 and 10 first, ascending, then the others as declared. Go sorted them (10, 2, ALPHA, ZED)." + + - id: read-configuration.merged-container-env-key-order-success + lane: runtime + command: read-configuration + priority: p1 + docker_required: true + setup_cmd: "WORKSPACE=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $WORKSPACE; mkdir -p $WORKSPACE/.devcontainer; printf '%s' '{\"image\":\"alpine:3\",\"containerEnv\":{\"ZED\":\"1\",\"10\":\"2\",\"ALPHA\":\"3\",\"2\":\"4\"}}' > $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-merged-configuration" + key_order: ["configuration.containerEnv", "mergedConfiguration.containerEnv"] + asserts: [exit_code, stdout_normalized] + class: output-shape + current_status: match + notes: "mergedConfiguration's containerEnv is mergeConfiguration's Object.assign over the metadata entries, so it keeps JS property order (2, 10, ZED, ALPHA). Go sorted it." + + - id: up.container-env-run-order-success + lane: runtime + command: up + priority: p1 + docker_required: true + setup_cmd: | + D=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE} + rm -rf $D && mkdir -p $D/ws/.devcontainer + printf '%s' '{"image":"alpine:3","containerEnv":{"ZED":"1","10":"2","ALPHA":"3","2":"4"}}' > $D/ws/.devcontainer/devcontainer.json + printf '%s\n' '#!/bin/sh' "echo \"\$*\" >> $D/docker.log" "exec $(command -v docker) \"\$@\"" > $D/docker + chmod +x $D/docker + echo WORKSPACE=$D/ws + echo DOCKER_WRAP=$D/docker + ts_cmd: "up --workspace-folder ${WORKSPACE} --docker-path ${DOCKER_WRAP} --skip-post-create --update-remote-user-uid-default never --id-label parity.case=${PARITY_CASE_ID}-${PARITY_SIDE} --remove-existing-container" + verify_cmd: "grep -o -E -- '-e (ZED|10|ALPHA|2)=[0-9]+' /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}/docker.log" + cleanup_cmd: "docker rm -f ${CONTAINER_ID} >/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: "The docker CLI is a wrapper that logs its arguments; verify_cmd lists the -e flags of docker run. TS passes the merged containerEnv in Object.keys order: the integer keys ascending (2, 10), then the others as declared (ZED, ALPHA). Go ranged over a map, so the order changed from run to run. The container's Config.Env is not compared: Docker reorders it." + - id: read-configuration.features-configuration-oci-feature-key-order-success lane: runtime command: read-configuration diff --git a/internal/cli/read_configuration.go b/internal/cli/read_configuration.go index d621967..93b3e14 100644 --- a/internal/cli/read_configuration.go +++ b/internal/cli/read_configuration.go @@ -444,6 +444,7 @@ func mergedConfigurationOutput(entries []imagemeta.Entry, containerCfg *containe if err := json.Unmarshal(mergedJSON, &mergedMap); err != nil { return nil, fmt.Errorf("read merged configuration: %w", err) } + orderProperty(mergedMap, "containerEnv", merged.ContainerEnvOrder) for k, v := range cfgMap { if !mergedOwnKeys[k] { mergedMap[k] = v diff --git a/internal/cli/read_configuration_held_test.go b/internal/cli/read_configuration_held_test.go index 9d2fabd..2c8be4e 100644 --- a/internal/cli/read_configuration_held_test.go +++ b/internal/cli/read_configuration_held_test.go @@ -373,7 +373,7 @@ func TestMergedConfigurationOutput(t *testing.T) { if env := got["remoteEnv"]; !reflect.DeepEqual(env, map[string]any{"P": "/container/bin"}) { t.Errorf("remoteEnv = %v, want ${containerEnv:PATH} resolved with the container's env", env) } - if env, _ := got["containerEnv"].(map[string]any); env["H"] != result.HostSub.Env["HOME"] { + if env, _ := got["containerEnv"].(orderedObject); env.fields["H"] != result.HostSub.Env["HOME"] { t.Errorf("containerEnv = %v, want the label's ${localEnv:HOME} substituted", got["containerEnv"]) } }) diff --git a/internal/cli/result_order.go b/internal/cli/result_order.go index cdd1a2d..d025d9f 100644 --- a/internal/cli/result_order.go +++ b/internal/cli/result_order.go @@ -7,6 +7,7 @@ import ( "slices" "github.com/devcontainers/cli/internal/config" + "github.com/devcontainers/cli/internal/features" ) type orderedObject struct { @@ -83,9 +84,35 @@ func mergedKeys(configurationKeys []string) []string { func orderConfigurations(result map[string]any, loaded *config.LoadResult) { cfgKeys := configurationKeys(loaded) if m, ok := result["configuration"].(map[string]any); ok { + orderConfigurationValues(m, loaded) result["configuration"] = orderedObject{m, cfgKeys} } if m, ok := result["mergedConfiguration"].(map[string]any); ok { + orderConfigurationValues(m, loaded) result["mergedConfiguration"] = orderedObject{m, mergedKeys(cfgKeys)} } } + +func orderConfigurationValues(m map[string]any, loaded *config.LoadResult) { + if loaded == nil || loaded.Config == nil { + return + } + cfg := loaded.Config + orderProperty(m, "containerEnv", cfg.ContainerEnvOrder) + feats, ok := m["features"].(map[string]any) + if !ok { + return + } + for id := range feats { + orderProperty(feats, id, cfg.FeatureOptionOrder[id]) + } + orderProperty(m, "features", cfg.FeatureOrder) +} + +// orderProperty writes the object at m[key] with its keys in +// features.PropertyOrder; a value already ordered, or not an object, stays. +func orderProperty(m map[string]any, key string, declared []string) { + if obj, ok := m[key].(map[string]any); ok { + m[key] = orderedObject{obj, features.PropertyOrder(obj, declared)} + } +} diff --git a/internal/cli/result_order_test.go b/internal/cli/result_order_test.go index 71f7ddc..e8a28fb 100644 --- a/internal/cli/result_order_test.go +++ b/internal/cli/result_order_test.go @@ -50,6 +50,29 @@ func TestReadConfigurationWritesTSKeyOrder(t *testing.T) { } } +func TestReadConfigurationWritesNestedObjectsInPropertyOrder(t *testing.T) { + startFakeEngine(t, nil) + stdout, err := readConfigurationStdout("--workspace-folder", writeConfigFixture(t, map[string]string{ + ".devcontainer/devcontainer.json": `{ + "image": "alpine:3", + "containerEnv": {"ZED": "1", "10": "2", "ALPHA": "3", "2": "4"}, + "features": {"zeta": {"zz": "1", "10": "t", "a": "b", "2": "s"}, "10": {}, "alpha": {}} + }`, + })) + if err != nil { + t.Fatal(err) + } + for path, want := range map[string][]string{ + "configuration.containerEnv": {"2", "10", "ZED", "ALPHA"}, + "configuration.features": {"10", "zeta", "alpha"}, + "configuration.features.zeta": {"2", "10", "zz", "a"}, + } { + if got, err := jsonKeyOrder(stdout, path); err != nil || !slices.Equal(got, want) { + t.Errorf("keys of %q = %v (%v), want %v", path, got, err, want) + } + } +} + func TestOrderConfigurationsWritesTSMergedOrder(t *testing.T) { loaded := &config.LoadResult{KeyOrder: []string{"name", "customizations", "image", "remoteUser", "postCreateCommand", "containerEnv"}} fields := func(keys ...string) map[string]any { diff --git a/internal/cli/up.go b/internal/cli/up.go index 65e48ac..08357ab 100644 --- a/internal/cli/up.go +++ b/internal/cli/up.go @@ -18,6 +18,7 @@ import ( "github.com/devcontainers/cli/internal/config" "github.com/devcontainers/cli/internal/docker" coreerrors "github.com/devcontainers/cli/internal/errors" + "github.com/devcontainers/cli/internal/features" "github.com/devcontainers/cli/internal/imagemeta" "github.com/devcontainers/cli/internal/lifecycle" "github.com/devcontainers/cli/internal/log" @@ -538,6 +539,7 @@ func addUpConfiguration(result map[string]any, opts *upOpts, fsPathRead bool, cf } cfgResult, _ := result["configuration"].(map[string]any) if mm, ok := mergedSub.(map[string]any); ok { + orderProperty(mm, "containerEnv", merged.ContainerEnvOrder) for k, v := range cfgResult { if !mergedOwnKeys[k] { mm[k] = v @@ -697,8 +699,9 @@ func (r *upRunner) runContainer(ctx context.Context, imageName string, cfg *conf containerUser := containerRunUser(merged.ContainerUser, imageInspect) var envVars []string - for k, v := range withCABundleEnv(merged.ContainerEnv, opts.caBundle) { - envVars = append(envVars, fmt.Sprintf("%s=%s", k, v)) + containerEnv := withCABundleEnv(merged.ContainerEnv, opts.caBundle) + for _, k := range features.PropertyOrder(containerEnv, merged.ContainerEnvOrder) { + envVars = append(envVars, k+"="+containerEnv[k]) } mounts, err := upWorkspaceMounts(loadResult.WorkspaceConfig, workspaceFolder, opts.workspaceMountConsistency)