Skip to content

Commit b707e94

Browse files
aledbfclaude
andcommitted
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 <noreply@anthropic.com>
1 parent 4660f5f commit b707e94

20 files changed

Lines changed: 310 additions & 21 deletions

‎docs/DIVERGENCES.md‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -117,9 +117,10 @@ touches a compared surface, reflected in the parity matrix.
117117
order (`pickFeatureProperties` for a Feature, `pickConfigProperties` for the
118118
configuration) and `&`, `<`, `>` unescaped, as in TS. Within a property's object value
119119
(a mount object, `customizations`, `portsAttributes`, …) the keys are sorted, because a
120-
decoded Go map keeps no order, where TS keeps the order they were written in; an entry
121-
given in another order (a base image's label written by another tool) is likewise
122-
rewritten in pick order. The parsed metadata is the same; only the label's bytes differ
120+
decoded Go map keeps no order, where TS keeps the order they were written in;
121+
`containerEnv` alone keeps TS's order (the array-index keys ascending, then the others
122+
as written). An entry given in another order (a base image's label written by another
123+
tool) is likewise rewritten in pick order. The parsed metadata is the same; only the label's bytes differ
123124
(`internal/imagemeta/testdata/ts-oracle/imagemeta.json` records both cases).
124125
- **Command results write nested objects with their keys sorted.** TS writes each result
125126
with `JSON.stringify`, so properties come in insertion order. Go matches that order for

‎docs/parity/parity-matrix.yaml‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -199,6 +199,19 @@ initial_cases:
199199
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."
200200

201201
# --- Wave A: new cases (some uncovered divergences, now fixed) ---
202+
- id: features.resolve-dependencies-option-key-order-success
203+
lane: contract
204+
command: features
205+
priority: p1
206+
docker_required: false
207+
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"
208+
cleanup_cmd: "rm -rf /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}"
209+
ts_cmd: "features resolve-dependencies --workspace-folder ${WORKSPACE}"
210+
asserts: [exit_code, stdout_text]
211+
class: output-shape
212+
current_status: match
213+
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."
214+
202215
- id: features-publish.missing-required
203216
lane: contract
204217
command: features-publish
@@ -1135,6 +1148,20 @@ initial_cases:
11351148
current_status: match
11361149
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."
11371150

1151+
- id: build.metadata-label-container-env-order-success
1152+
lane: runtime
1153+
command: build
1154+
priority: p1
1155+
docker_required: true
1156+
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"
1157+
ts_cmd: "build --workspace-folder ${WORKSPACE} --image-name parity-${PARITY_CASE_ID}-${PARITY_SIDE}"
1158+
verify_cmd: "docker image inspect -f '{{index .Config.Labels \"devcontainer.metadata\"}}' ${IMAGE_NAME}"
1159+
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"
1160+
asserts: [exit_code, stdout_normalized]
1161+
class: build-runtime
1162+
current_status: match
1163+
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)."
1164+
11381165
- id: up.compose-feature-host-variables-raw-label-success
11391166
lane: runtime
11401167
command: up
@@ -1149,6 +1176,20 @@ initial_cases:
11491176
current_status: match
11501177
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."
11511178

1179+
- id: up.compose-container-env-order-success
1180+
lane: runtime
1181+
command: up
1182+
priority: p1
1183+
docker_required: true
1184+
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"
1185+
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}"
1186+
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})"
1187+
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"
1188+
asserts: [exit_code, stdout_normalized]
1189+
class: up-runtime
1190+
current_status: match
1191+
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."
1192+
11521193
- id: read-configuration.feature-host-variables-merged-success
11531194
lane: runtime
11541195
command: read-configuration
@@ -1396,6 +1437,20 @@ initial_cases:
13961437
current_status: match
13971438
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."
13981439

1440+
- id: read-configuration.features-configuration-value-key-order-success
1441+
lane: contract
1442+
command: read-configuration
1443+
priority: p1
1444+
docker_required: false
1445+
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"
1446+
cleanup_cmd: "rm -rf /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}"
1447+
ts_cmd: "read-configuration --workspace-folder ${WORKSPACE} --include-features-configuration"
1448+
key_order: ["featuresConfiguration.featureSets.0.features.0.value"]
1449+
asserts: [exit_code, stdout_normalized]
1450+
class: output-shape
1451+
current_status: match
1452+
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)."
1453+
13991454
- id: read-configuration.features-configuration-oci-feature-key-order-success
14001455
lane: runtime
14011456
command: read-configuration

‎internal/cli/build.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,7 @@ func runBuild(ctx context.Context, out Output, opts *buildOpts) error {
186186
defer engine.Close()
187187

188188
run := &buildRunner{log: logger, docker: dockerClient, engine: engine, opts: opts}
189-
if run.configLabel, err = configLabelEntry(loadResult.RawConfig, false); err != nil {
189+
if run.configLabel, err = configLabelEntry(loadResult.RawConfig, loadResult.Config.ContainerEnvOrder, false); err != nil {
190190
return writeErrorResult(out, err.Error())
191191
}
192192

‎internal/cli/compose_override.go‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,9 @@ import (
66
"fmt"
77
"os"
88
"regexp"
9-
"slices"
109
"strings"
1110

11+
"github.com/devcontainers/cli/internal/features"
1212
"github.com/devcontainers/cli/internal/imagemeta"
1313
)
1414

@@ -212,12 +212,7 @@ func generateComposeFeaturesOverride(in composeFeaturesOverride) string {
212212
}
213213
if len(merged.ContainerEnv) > 0 {
214214
b.WriteString("\n environment:")
215-
keys := make([]string, 0, len(merged.ContainerEnv))
216-
for k := range merged.ContainerEnv {
217-
keys = append(keys, k)
218-
}
219-
slices.Sort(keys)
220-
for _, k := range keys {
215+
for _, k := range features.PropertyOrder(merged.ContainerEnv, merged.ContainerEnvOrder) {
221216
v := strings.ReplaceAll(merged.ContainerEnv[k], "\n", `\n`)
222217
v = escapeComposeDollar(v)
223218
v = strings.ReplaceAll(v, "'", "''")

‎internal/cli/compose_override_test.go‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -215,3 +215,19 @@ func TestComposeFeaturesBuild(t *testing.T) {
215215
t.Errorf("label-only content = %q, want %q", label.DockerfileContent, want)
216216
}
217217
}
218+
219+
func TestGenerateComposeFeaturesOverrideWritesTheMergedEnvInPropertyOrder(t *testing.T) {
220+
entries := imagemeta.ReadMetadataFromLabels(map[string]string{
221+
imagemeta.MetadataLabel: `[{"containerEnv":{"ZB":"base","10":"base","AB":"base"}}]`,
222+
}, nil)
223+
entries = append(entries, imagemeta.Entry{
224+
ContainerEnv: map[string]string{"ZED": "cfg", "AB": "cfg", "2": "cfg", "ALPHA": "cfg"},
225+
ContainerEnvOrder: []string{"ZED", "AB", "2", "ALPHA"},
226+
})
227+
got := generateComposeFeaturesOverride(composeFeaturesOverride{Service: "app", Merged: imagemeta.MergeConfiguration(entries)})
228+
_, env, _ := strings.Cut(got, " environment:\n")
229+
want := " - '2=cfg'\n - '10=base'\n - 'ZB=base'\n - 'AB=cfg'\n - 'ZED=cfg'\n - 'ALPHA=cfg'\n"
230+
if env != want {
231+
t.Errorf("environment:\n%s\nwant:\n%s", env, want)
232+
}
233+
}

‎internal/cli/features_resolve_deps.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,9 @@ func realFeaturesResolveDepsCmd() *cobra.Command {
8181
}
8282
var opts any
8383
if len(fs.Features) > 0 {
84-
opts = fs.Features[0].Value
84+
if opts, err = features.JSValue(fs.Features[0].Value, fs.Features[0].ValueOrder); err != nil {
85+
return err
86+
}
8587
}
8688
entries = append(entries, installEntry{ID: id, Options: opts})
8789
}

‎internal/cli/metadata_label.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ var labelConfigProperties = []string{
1717
"otherPortsAttributes", "forwardPorts", "shutdownAction", "updateRemoteUserUID", "hostRequirements",
1818
}
1919

20-
func configLabelEntry(raw map[string]any, omitRemoteEnv bool) (imagemeta.Entry, error) {
20+
func configLabelEntry(raw map[string]any, envOrder []string, omitRemoteEnv bool) (imagemeta.Entry, error) {
2121
picked := map[string]any{}
2222
for _, k := range labelConfigProperties {
2323
if v, ok := raw[k]; ok && (!omitRemoteEnv || k != "remoteEnv") {
@@ -32,6 +32,7 @@ func configLabelEntry(raw map[string]any, omitRemoteEnv bool) (imagemeta.Entry,
3232
if err != nil {
3333
return imagemeta.Entry{}, fmt.Errorf("devcontainer.metadata entry of the configuration: %w", err)
3434
}
35+
entry.ContainerEnvOrder = envOrder
3536
return entry, nil
3637
}
3738

@@ -75,6 +76,9 @@ func hostMetadataSubstitution(hostSub config.HostSubContext, idLabels []string)
7576
if err != nil {
7677
return err
7778
}
79+
for i := range resolved {
80+
resolved[i].ContainerEnvOrder = (*entries)[i].ContainerEnvOrder
81+
}
7882
*entries = resolved
7983
return nil
8084
}

‎internal/cli/metadata_label_test.go‎

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ func TestConfigLabelEntryPicksTheRawConfig(t *testing.T) {
2525
}`), &raw); err != nil {
2626
t.Fatal(err)
2727
}
28-
entry, err := configLabelEntry(raw, false)
28+
entry, err := configLabelEntry(raw, nil, false)
2929
if err != nil {
3030
t.Fatal(err)
3131
}
@@ -34,14 +34,14 @@ func TestConfigLabelEntryPicksTheRawConfig(t *testing.T) {
3434
if string(got) != want {
3535
t.Errorf("entry = %s\nwant %s", got, want)
3636
}
37-
omitted, err := configLabelEntry(raw, true)
37+
omitted, err := configLabelEntry(raw, nil, true)
3838
if err != nil {
3939
t.Fatal(err)
4040
}
4141
if omitted.RemoteEnv != nil {
4242
t.Errorf("remoteEnv kept with omitRemoteEnv: %v", omitted.RemoteEnv)
4343
}
44-
if _, err := configLabelEntry(map[string]any{"init": "yes"}, false); err == nil {
44+
if _, err := configLabelEntry(map[string]any{"init": "yes"}, nil, false); err == nil {
4545
t.Error("an init that is no boolean: want an error")
4646
}
4747
}
@@ -164,3 +164,28 @@ func TestHostMetadataSubstitutionLeavesTheRawEntriesRaw(t *testing.T) {
164164
}
165165
}
166166
}
167+
168+
func TestMetadataEntriesKeepTheContainerEnvOrder(t *testing.T) {
169+
raw := map[string]any{"containerEnv": map[string]any{"ZED": "1", "10": "2", "ALPHA": "${localEnv:U}", "2": "4"}}
170+
order := []string{"ZED", "10", "ALPHA", "2"}
171+
entry, err := configLabelEntry(raw, order, false)
172+
if err != nil {
173+
t.Fatal(err)
174+
}
175+
want := `[{"containerEnv":{"2":"4","10":"2","ZED":"1","ALPHA":"${localEnv:U}"}}]`
176+
if got := containerMetadataLabel([]imagemeta.Entry{entry}); got != want {
177+
t.Errorf("label = %s\nwant %s", got, want)
178+
}
179+
entries := []imagemeta.Entry{entry}
180+
if err := hostMetadataSubstitution(config.HostSubContext{Env: map[string]string{"U": "me"}}, nil)(&entries); err != nil {
181+
t.Fatal(err)
182+
}
183+
want = `[{"containerEnv":{"2":"4","10":"2","ZED":"1","ALPHA":"me"}}]`
184+
if got := containerMetadataLabel(entries); got != want {
185+
t.Errorf("substituted label = %s\nwant %s", got, want)
186+
}
187+
cfg := &config.DevContainer{ContainerEnv: map[string]string{"ZED": "1", "ALPHA": "3"}, ContainerEnvOrder: []string{"ZED", "ALPHA"}}
188+
if got, want := containerMetadataLabel([]imagemeta.Entry{configMetadataEntry(cfg, false)}), `[{"containerEnv":{"ZED":"1","ALPHA":"3"}}]`; got != want {
189+
t.Errorf("config entry label = %s\nwant %s", got, want)
190+
}
191+
}

‎internal/cli/read_configuration.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -483,6 +483,7 @@ func configMetadataEntry(cfg *config.DevContainer, omitRemoteEnv bool) imagemeta
483483
ShutdownAction: cfg.ShutdownAction,
484484
UpdateRemoteUserUID: cfg.UpdateRemoteUserUID,
485485
ContainerEnv: cfg.ContainerEnv,
486+
ContainerEnvOrder: cfg.ContainerEnvOrder,
486487
CapAdd: cfg.CapAdd,
487488
SecurityOpt: cfg.SecurityOpt,
488489
Mounts: metadataMounts(cfg),

‎internal/cli/up.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -315,7 +315,7 @@ func runUp(ctx context.Context, out Output, opts *upOpts) error {
315315
}
316316
run.idLabels = idLabels
317317
run.substitute = hostMetadataSubstitution(loadResult.HostSub, idLabels)
318-
if run.configLabel, err = configLabelEntry(loadResult.RawConfig, opts.omitConfigRemoteEnvFromMeta); err != nil {
318+
if run.configLabel, err = configLabelEntry(loadResult.RawConfig, loadResult.Config.ContainerEnvOrder, opts.omitConfigRemoteEnvFromMeta); err != nil {
319319
return writeErrorResult(out, err.Error())
320320
}
321321

0 commit comments

Comments
 (0)