diff --git a/docs/migration.md b/docs/migration.md index 2b32a9e..0812e15 100644 --- a/docs/migration.md +++ b/docs/migration.md @@ -86,6 +86,36 @@ runs the official CLI is unaffected: matching container. - Features are staged under `/devcontainercli-/container-features/`, one folder per build, and that folder is kept as the official CLI keeps it. +- When the workspace is a Git worktree and the configuration sets both + `workspaceMount` and `workspaceFolder`, `up` no longer mounts the main + repository's Git common directory, and `read-configuration` no longer reports + `additionalMountString`, as in the official CLI v0.89.0. With only + `workspaceMount` set, the common directory is mounted under + `/workspaces/
/.git` rather than under the custom mount's + target. Earlier releases carried an unreleased upstream change (#1261) instead. + A configuration that needs Git inside the container this way should add the + common directory to `mounts` itself. +- A `${containerEnv}` reference without a variable name fails `up`, `set-up`, + `run-user-commands`, `exec` and `read-configuration` with exit 1 and + `'${containerEnv}' in can not be resolved because no environment + variable name is given.`, as the official CLI does. Earlier releases reported + success for most of them. Name the variable, as in `${containerEnv:PATH}`. +- `exec` reads the configuration `--config` or `--override-config` names, also + with `--container-id` and no `--workspace-folder`, and exits 1 when it is + missing (`Dev container config () not found.`) or cannot be loaded. + Earlier releases ignored those flags without a workspace folder and ran the + command despite a configuration error. +- `read-configuration` looks up the workspace's container (or the one + `--id-label` names) as the official CLI does, and exits 1 with nothing on + stdout when that lookup fails, including when no Docker engine is reachable. + Earlier releases ignored the failure and printed the configuration. Run it + where the Docker engine answers. +- A local Feature (`./x`) must live inside the workspace's `.devcontainer/` + folder, as in the official CLI. `build`, `up`, `read-configuration` and + `upgrade` now exit 1 with `ERR: Feature './x' could not be processed. ...` for + `../x`, an absolute path, or `./x` next to a `.devcontainer.json` at the + workspace root, where earlier releases resolved the folder next to the + configuration. Move the Feature's folder under `.devcontainer/`. - `build` on a Docker Compose configuration without `--image-name` reports `"imageName"` as the image's name, a string, as the official CLI does. Earlier releases wrapped it in an array. With `--image-name`, and for image and @@ -110,6 +140,39 @@ runs the official CLI is unaffected: `--frozen-lockfile` instead. - `upgrade` always rewrites the lockfile in its canonical form, even when the existing one differs only in formatting. +- `upgrade --log-level` accepts only `error`, `info`, `debug`, and `trace`. Any + other value exits 1 with `Invalid value "" for --log-level` instead of + running at the default level. +- `upgrade --config` naming a file that does not exist reports + `Dev container config () not found.` instead of the operating system's + read error. The exit code stays 1. +- `upgrade` no longer logs `Lockfile updated.` on stderr after writing the + lockfile, and starts its log with the version header once the configuration + loads, as the official CLI does. Check the exit code to tell whether the + lockfile was written. +- `upgrade` on a configuration without Features exits 1 with + `Failed to update lockfile` and writes no lockfile, with or without + `--dry-run`. Earlier releases wrote or printed `{"features":{}}` and exited 0. + Skip `upgrade` for configurations that use no Features. +- `upgrade` resolves Features the way `build` does. A Feature whose manifest or + content cannot be fetched fails the command (exit 1, + `ERR: Feature '' could not be processed. ...`) instead of being left out + of the lockfile. The lockfile also lists the Features that other Features + depend on, with each entry's `dependsOn`, so it matches the one `build` and the + official CLI write. Make every Feature reachable, with registry credentials + where needed, before running `upgrade`. +- `upgrade --feature --target-version ` rewrites the Feature's + key in `devcontainer.json` to `:`, even with + `--dry-run`, before generating the lockfile. Earlier releases validated the + two flags but otherwise ignored them. Run it on a clean working tree, or + without the flags, when the configuration must stay unchanged. +- `outdated --output-format json` indents its result only when stdout is a + terminal. Redirected or piped, as in a script, it is one unindented line + (`{"features":{...}}`) followed by a newline; earlier releases always indented it + with two spaces. A script that parses the JSON is unaffected; one that matches + lines or greps the indented text should parse the JSON instead (with `jq`, say). +- `outdated` on a configuration without Features prints its table header alone + (`Feature Current Wanted Latest`) instead of `No features configured.`. - Only a registry whose host is `localhost` is reached over plain HTTP. Earlier releases also used plain HTTP for `127.0.0.1`, which is now addressed over HTTPS like any other host. Refer to a local plain-HTTP registry as `localhost:`. diff --git a/docs/parity/parity-matrix.yaml b/docs/parity/parity-matrix.yaml index b242bad..06d7d83 100644 --- a/docs/parity/parity-matrix.yaml +++ b/docs/parity/parity-matrix.yaml @@ -310,6 +310,57 @@ initial_cases: current_status: match notes: "XOR from yargs .check(): --feature and --target-version must be used together. Only --feature => exit 1 'The '--target-version' and '--feature' flag must be used together.'. Go ignored both flags; now RunE applies the XOR with the same message." + - id: upgrade.err-invalid-log-level + lane: contract + command: upgrade + priority: p1 + docker_required: false + ts_cmd: "upgrade --workspace-folder src/test/configs/image-with-features --log-level warn --feature ghcr.io/devcontainers/features/node" + asserts: [exit_code, stderr_normalized] + class: format-validation + current_status: match + notes: "upgrade's --log-level is a yargs choice of error/info/debug/trace (featuresUpgradeOptions), checked before .check(): 'warn' => exit 1 with the invalid-choice error, not the --feature/--target-version one. Go accepted any value and ran." + + - id: upgrade.err-no-features + lane: contract + command: upgrade + priority: p1 + docker_required: false + setup_cmd: "WORKSPACE=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $WORKSPACE; mkdir -p $WORKSPACE; printf '%s' '{\"image\":\"alpine:3\"}' > $WORKSPACE/.devcontainer.json; echo WORKSPACE=$WORKSPACE" + verify_cmd: "test ! -e ${WORKSPACE}/.devcontainer-lock.json" + cleanup_cmd: "rm -rf /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}" + ts_cmd: "upgrade --workspace-folder ${WORKSPACE}" + asserts: [exit_code] + class: upgrade-runtime + current_status: match + notes: "Without Features generateFeaturesConfig returns undefined and upgrade throws ContainerError 'Failed to update lockfile' (upgradeCommand.ts): exit 1 and no lockfile written (verify_cmd). TS logs the inspected error object, whose last line is '}', so only the exit code is compared; TestUpgradeFailsWithoutFeatures holds the message. Go wrote {\"features\":{}} and exited 0." + + - id: outdated.json-not-a-terminal-success + lane: contract + command: outdated + priority: p1 + docker_required: false + setup_cmd: "WORKSPACE=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $WORKSPACE; mkdir -p $WORKSPACE/.devcontainer/feat; printf '%s' '{\"id\":\"feat\",\"version\":\"1.0.0\"}' > $WORKSPACE/.devcontainer/feat/devcontainer-feature.json; printf '%s' '{\"image\":\"alpine:3\",\"features\":{\"./feat\":{}}}' > $WORKSPACE/.devcontainer/devcontainer.json; echo WORKSPACE=$WORKSPACE" + cleanup_cmd: "rm -rf /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}" + ts_cmd: "outdated --workspace-folder ${WORKSPACE} --output-format json" + asserts: [exit_code, stdout_text] + class: outdated + current_status: match + notes: "outdated writes JSON.stringify(outdated, undefined, process.stdout.isTTY ? ' ' : undefined): with stdout not a terminal, as here, the result is one unindented line ({\"features\":{}}, a local Feature having no versions). Go used to indent it always." + + - id: outdated.text-without-features-success + lane: contract + command: outdated + 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\"}' > $WORKSPACE/.devcontainer/devcontainer.json; echo WORKSPACE=$WORKSPACE" + cleanup_cmd: "rm -rf /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}" + ts_cmd: "outdated --workspace-folder ${WORKSPACE}" + asserts: [exit_code, stdout_text] + class: outdated + current_status: match + notes: "Without Features, loadVersionInfo returns no Features and outdated writes its text table with no rows: the header alone ('Feature Current Wanted Latest'). Go used to print 'No features configured.'." + - id: outdated.malformed-lockfile-error lane: contract command: outdated @@ -805,6 +856,45 @@ initial_cases: current_status: match notes: "A bare Feature id ('myfeature') is a legacy id TS rejects ('Legacy feature myfeature not supported...'), exit 1 with no result. Go used to drop the Feature error silently and print a configuration without the Feature." + - id: read-configuration.local-feature-outside-devcontainer-error + 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 $WORKSPACE/feat; printf '%s' '{\"id\":\"feat\",\"version\":\"1.0.0\"}' > $WORKSPACE/feat/devcontainer-feature.json; printf '%s' '{\"image\":\"alpine:3\",\"features\":{\"../feat\":{}}}' > $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" + asserts: [exit_code, stdout_normalized] + class: features + current_status: match + notes: "A local Feature must lie in the workspace's .devcontainer folder (processFeatureIdentifier, containerFeaturesConfiguration.ts): '../feat' logs 'Local file path parse error. Resolved path must be a child of the .devcontainer/ folder.' and the Feature fails as one that could not be processed, exit 1 with no result. Go used to resolve the folder next to the configuration wherever it was and print a configuration with the Feature. stderr is not compared: Go also logs the thrown error at error level, which TS's read-configuration catch writes at debug level, a divergence common to every read-configuration failure." + + - id: build.local-feature-absolute-path-error + lane: runtime + command: build + priority: p1 + docker_required: true + setup_cmd: "WORKSPACE=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $WORKSPACE; mkdir -p $WORKSPACE/.devcontainer/feat; printf '%s' '{\"id\":\"feat\",\"version\":\"1.0.0\"}' > $WORKSPACE/.devcontainer/feat/devcontainer-feature.json; printf '#!/bin/sh\\necho installed\\n' > $WORKSPACE/.devcontainer/feat/install.sh; chmod +x $WORKSPACE/.devcontainer/feat/install.sh; printf '{\"image\":\"alpine:3\",\"features\":{\"%s/.devcontainer/feat\":{}}}' $WORKSPACE > $WORKSPACE/.devcontainer/devcontainer.json; echo WORKSPACE=$WORKSPACE" + cleanup_cmd: "rm -rf /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}" + ts_cmd: "build --workspace-folder ${WORKSPACE}" + asserts: [exit_code, stdout_normalized] + class: features + current_status: match + notes: "An absolute Feature id is a local Feature (getFeatureIdType: './', '../' or '/') that processFeatureIdentifier refuses, even for a folder inside .devcontainer: it logs 'An Absolute path to a local feature is not allowed.' and build reports {\"outcome\":\"error\",\"message\":\"ERR: Feature '' could not be processed. You may not have permission ...\",\"description\":\"An error occurred building the container.\"}, exit 1. Go used to take it for a legacy id and fail with the legacy Feature error." + + - id: build.local-feature-config-outside-devcontainer-error + lane: runtime + command: build + priority: p1 + docker_required: true + setup_cmd: "WORKSPACE=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $WORKSPACE; mkdir -p $WORKSPACE/feat; printf '%s' '{\"id\":\"feat\",\"version\":\"1.0.0\"}' > $WORKSPACE/feat/devcontainer-feature.json; printf '#!/bin/sh\\n' > $WORKSPACE/feat/install.sh; printf '%s' '{\"image\":\"alpine:3\",\"features\":{\"./feat\":{}}}' > $WORKSPACE/.devcontainer.json; echo WORKSPACE=$WORKSPACE" + cleanup_cmd: "rm -rf /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}" + ts_cmd: "build --workspace-folder ${WORKSPACE}" + asserts: [exit_code, stdout_normalized] + class: features + current_status: match + notes: "With the configuration at the workspace root (.devcontainer.json), './feat' resolves outside the .devcontainer folder, which processFeatureIdentifier refuses; build reports the plain Error buildDependencyGraph throws as its message: {\"outcome\":\"error\",\"message\":\"ERR: Feature './feat' could not be processed. You may not have permission ...\",\"description\":\"An error occurred building the container.\"}, exit 1. Go used to build the Feature." + - id: up.update-uid-remote-user-from-image-metadata-success lane: runtime command: up @@ -1141,6 +1231,72 @@ initial_cases: current_status: match notes: "The compose path checks the frozen lockfile too (TS generateFeaturesConfig -> writeLockfile, wrapped by dockerCompose.ts as 'An error occurred setting up the container.'). Go skipped it when no lockfile existed and started the service." + - id: read-configuration.features-configuration-local-feature-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 $WORKSPACE/.devcontainer/g; printf '%s' '{\"id\":\"f\",\"version\":\"1.0.0\",\"name\":\"F\",\"options\":{\"a\":{\"type\":\"string\",\"default\":\"z\"}},\"containerEnv\":{\"K\":\"v\"},\"customizations\":{\"vscode\":{\"extensions\":[\"e\"]}},\"installsAfter\":[\"./g\"]}' > $WORKSPACE/.devcontainer/f/devcontainer-feature.json; printf '%s\\n' '#!/bin/sh' 'true' > $WORKSPACE/.devcontainer/f/install.sh; printf '%s' '{\"version\":\"2.0.0\",\"id\":\"g\"}' > $WORKSPACE/.devcontainer/g/devcontainer-feature.json; printf '%s\\n' '#!/bin/sh' 'true' > $WORKSPACE/.devcontainer/g/install.sh; printf '%s' '{\"image\":\"alpine:3\",\"features\":{\"./f\":{\"a\":\"b\"},\"./g\":true}}' > $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", "featuresConfiguration.featureSets.1.features.0"] + asserts: [exit_code, stdout_normalized] + class: output-shape + current_status: match + notes: "A Feature entry is TS's {...featureJson, ...feature} (applyFeatureConfigToFeature): devcontainer-feature.json's properties in its order, then those processFeatureIdentifier and fetchFeatures set that it lacks ({id, name, value, included}, then cachePath and consecutiveId), so value follows the metadata's properties. g's metadata lists version before id and has no name, which then follows it. Go wrote the struct's order (value after consecutiveId, containerEnv and customizations last)." + + - id: read-configuration.features-configuration-local-feature-metadata-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/h; printf '%s' '{\"version\":\"1.0.0\",\"id\":\"h\",\"keywords\":[\"k\"],\"options\":{\"z\":{\"type\":\"string\",\"default\":\"1\"},\"a\":{\"default\":true,\"type\":\"boolean\"},\"e\":{}},\"x\":1,\"x\":2,\"10\":\"ten\",\"2\":\"two\",\"extensions\":[\"ext1\"],\"settings\":{\"s\":1,\"u\":1},\"customizations\":{\"vscode\":{\"extensions\":[\"e0\"],\"settings\":{\"t\":2,\"s\":3}}},\"num\":1.50,\"empty\":{}}' > $WORKSPACE/.devcontainer/h/devcontainer-feature.json; printf '%s\\n' '#!/bin/sh' 'true' > $WORKSPACE/.devcontainer/h/install.sh; printf '%s' '{\"image\":\"alpine:3\",\"features\":{\"./h\":{}}}' > $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", "featuresConfiguration.featureSets.0", "featuresConfiguration.featureSets.0.sourceInformation", "featuresConfiguration.featureSets.0.features.0", "featuresConfiguration.featureSets.0.features.0.options", "featuresConfiguration.featureSets.0.features.0.options.z", "featuresConfiguration.featureSets.0.features.0.options.a", "featuresConfiguration.featureSets.0.features.0.customizations.vscode", "featuresConfiguration.featureSets.0.features.0.customizations.vscode.settings"] + asserts: [exit_code, stdout_normalized, stdout_text] + class: output-shape + 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-oci-feature-key-order-success + lane: runtime + command: read-configuration + priority: p1 + network_required: true + setup_cmd: "WORKSPACE=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $WORKSPACE; mkdir -p $WORKSPACE/.devcontainer; printf '%s' '{\"image\":\"alpine:3\",\"features\":{\"ghcr.io/devcontainers/features/github-cli:1\":{}}}' > $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", "featuresConfiguration.featureSets.0.sourceInformation", "featuresConfiguration.featureSets.0.sourceInformation.manifest", "featuresConfiguration.featureSets.0.sourceInformation.manifest.config", "featuresConfiguration.featureSets.0.sourceInformation.manifest.annotations", "featuresConfiguration.featureSets.0.sourceInformation.featureRef", "featuresConfiguration.featureSets.0.features.0", "featuresConfiguration.featureSets.0.features.0.options", "featuresConfiguration.featureSets.0.features.0.options.version"] + asserts: [exit_code, stdout_normalized, stdout_text] + class: output-shape + current_status: match + notes: "An OCI Feature set as TS writes it: sourceInformation in tryGetOCIFeatureSet's order (type, manifest, manifestDigest, featureRef, userFeatureId, userFeatureIdWithoutVersion), the manifest as the registry served it (JSON.parse of its body), featureRef in getRef's order, then internalVersion before computedDigest (applyFeatureConfigToFeature). The Feature is the annotation's metadata spread over {id, included, value} and that over the layer's devcontainer-feature.json, so its options keep the published order. Go used to re-serialize the manifest and featureRef with sorted keys and write the Feature in the struct's order." + + - id: read-configuration.features-configuration-oci-feature-pinned-success + lane: runtime + command: read-configuration + priority: p1 + network_required: true + ts_cmd: "read-configuration --workspace-folder reference/src/test/container-features/configs/lockfile-frozen --include-features-configuration" + key_order: ["featuresConfiguration.featureSets.0.sourceInformation.featureRef", "featuresConfiguration.featureSets.0.features.0", "featuresConfiguration.featureSets.1.sourceInformation.featureRef"] + asserts: [exit_code, stdout_normalized] + class: output-shape + current_status: match + notes: "Both Features are pinned by the fixture's lockfile. featureRef is parsed from the reference as written (tryGetOCIFeatureSet), so it keeps tag and version 1 though the manifest is fetched by the lockfile's digest. color's published metadata names it new-color: the Feature's id, and so its consecutiveId and cachePath (new-color_0), are the metadata's (buildDependencyGraph spreads it over the Feature). Go used to report the pinned digest in featureRef, without the tag, and name the folder after the reference's id (color_0)." + + - id: read-configuration.features-configuration-oci-feature-digest-success + lane: runtime + command: read-configuration + priority: p1 + network_required: true + ts_cmd: "read-configuration --workspace-folder reference/src/test/container-features/configs/dockerfile-with-v2-oci-features --include-features-configuration" + key_order: ["featuresConfiguration.featureSets.0.sourceInformation.featureRef", "featuresConfiguration.featureSets.1.sourceInformation.featureRef", "featuresConfiguration.featureSets.2.sourceInformation.featureRef", "featuresConfiguration.featureSets.0.features.0", "featuresConfiguration.featureSets.1.features.0", "featuresConfiguration.featureSets.2.features.0"] + asserts: [exit_code, stdout_normalized] + class: output-shape + current_status: match + notes: "A digest reference's featureRef has digest and version, no tag (getRef), next to the legacy ids terraform and node mapped to their published Features." + - id: read-configuration.key-order-success lane: semantic command: read-configuration @@ -1235,6 +1391,19 @@ initial_cases: current_status: match notes: "TS makes a file URI of --config only: with --override-config, configFilePath is the configuration found in the workspace (getDevContainerConfigPathIn), with the vscode-fileHost scheme. Go wrote the file scheme whenever --override-config or --container-id was given." + - id: read-configuration.override-config-without-workspace-config-success + lane: semantic + 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\"}' > $WORKSPACE/.devcontainer/f/devcontainer-feature.json; printf '%s\\n' '#!/bin/sh' 'true' > $WORKSPACE/.devcontainer/f/install.sh; printf '%s' '{\"image\":\"alpine:3\",\"features\":{\"./f\":{}}}' > $WORKSPACE/override.json; echo WORKSPACE=$WORKSPACE" + ts_cmd: "read-configuration --workspace-folder ${WORKSPACE} --override-config ${WORKSPACE}/override.json --include-features-configuration" + cleanup_cmd: "rm -rf ${WORKSPACE}; true" + asserts: [exit_code, stdout_normalized] + class: config-parsing + current_status: match + notes: "A workspace without a devcontainer.json: TS names its default path, /.devcontainer/devcontainer.json, as configFilePath (getDefaultDevContainerConfigPath) and reads the override in its place, so the override's local Feature ./f resolves in .devcontainer. Go named the override as configFilePath and looked for ./f next to it, failing the command." + - id: up.override-config-scheme-success lane: runtime command: up @@ -1276,6 +1445,90 @@ initial_cases: current_status: match notes: "up mounts the additionalMountString read-configuration reports: TS spawnDevContainer passes it as a --mount after the workspace mount (singleContainer.ts), so the container has the worktree and its common dir." + - id: read-configuration.git-worktree-common-dir-custom-workspace-mount-and-folder-success + lane: semantic + command: read-configuration + priority: p1 + docker_required: true + setup_cmd: "BASE=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $BASE; mkdir -p $BASE; git init -q $BASE/repo && git -C $BASE/repo -c user.email=p@p -c user.name=p commit -q --allow-empty -m init && git -C $BASE/repo worktree add -q ../wt && printf '%s\\n' 'gitdir: ../repo/.git/worktrees/wt' > $BASE/wt/.git; mkdir -p $BASE/wt/.devcontainer; printf '%s' '{\"image\":\"alpine:3\",\"remoteUser\":\"root\",\"workspaceMount\":\"type=bind,source=${localWorkspaceFolder},target=/src/wt\",\"workspaceFolder\":\"/src/wt\"}' > $BASE/wt/.devcontainer/devcontainer.json; echo WORKSPACE=$BASE/wt; echo BASE=$BASE" + ts_cmd: "read-configuration --workspace-folder ${WORKSPACE} --mount-git-worktree-common-dir" + cleanup_cmd: "rm -rf ${BASE}; true" + key_order: ["workspace"] + asserts: [exit_code, stdout_normalized] + class: output-shape + current_status: match + notes: "TS getWorkspaceConfiguration (utils.ts) computes the common-dir mount together with the workspace mount, only when the config leaves workspaceFolder or workspaceMount unset: with both set, read-configuration reports no additionalMountString. Go added one relative to the custom mount's target." + + - id: read-configuration.git-worktree-common-dir-custom-workspace-mount-success + lane: semantic + command: read-configuration + priority: p1 + docker_required: true + setup_cmd: "BASE=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $BASE; mkdir -p $BASE; git init -q $BASE/repo && git -C $BASE/repo -c user.email=p@p -c user.name=p commit -q --allow-empty -m init && git -C $BASE/repo worktree add -q ../wt && printf '%s\\n' 'gitdir: ../repo/.git/worktrees/wt' > $BASE/wt/.git; mkdir -p $BASE/wt/.devcontainer; printf '%s' '{\"image\":\"alpine:3\",\"remoteUser\":\"root\",\"workspaceMount\":\"type=volume,source=parity-wt,target=/src/wt\"}' > $BASE/wt/.devcontainer/devcontainer.json; echo WORKSPACE=$BASE/wt; echo BASE=$BASE" + ts_cmd: "read-configuration --workspace-folder ${WORKSPACE} --mount-git-worktree-common-dir" + cleanup_cmd: "rm -rf ${BASE}; true" + key_order: ["workspace"] + asserts: [exit_code, stdout_normalized] + class: output-shape + current_status: match + notes: "A custom workspaceMount (here a volume) without workspaceFolder: TS getWorkspaceConfiguration (utils.ts) still computes the worktree layout, so workspaceFolder and the additionalMountString target derive from /workspaces/, not from the custom mount's target. Go resolved the common dir relative to the custom target." + + - id: up.git-worktree-common-dir-custom-workspace-mount-and-folder-success + lane: runtime + command: up + priority: p1 + docker_required: true + setup_cmd: "BASE=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $BASE; mkdir -p $BASE; git init -q $BASE/repo && git -C $BASE/repo -c user.email=p@p -c user.name=p commit -q --allow-empty -m init && git -C $BASE/repo worktree add -q ../wt && printf '%s\\n' 'gitdir: ../repo/.git/worktrees/wt' > $BASE/wt/.git; mkdir -p $BASE/wt/.devcontainer; printf '%s' '{\"image\":\"alpine:3\",\"remoteUser\":\"root\",\"workspaceMount\":\"type=bind,source=${localWorkspaceFolder},target=/src/wt\",\"workspaceFolder\":\"/src/wt\"}' > $BASE/wt/.devcontainer/devcontainer.json; echo WORKSPACE=$BASE/wt; echo BASE=$BASE" + ts_cmd: "up --workspace-folder ${WORKSPACE} --mount-git-worktree-common-dir --buildkit=never --id-label parity.case=${PARITY_CASE_ID}-${PARITY_SIDE}" + verify_cmd: "docker inspect -f '{{range .Mounts}}{{.Type}}:{{.Source}}:{{.Destination}} {{end}}' ${CONTAINER_ID}" + cleanup_cmd: "IDS=$(docker ps -aq --filter label=parity.case=${PARITY_CASE_ID}-${PARITY_SIDE}); [ -n \"$IDS\" ] && docker rm -f $IDS >/dev/null 2>&1; rm -rf ${BASE}; true" + asserts: [exit_code, stdout_normalized] + class: up-runtime + current_status: match + notes: "With workspaceMount and workspaceFolder both set, TS spawnDevContainer gets no additionalMountString (getWorkspaceConfiguration, utils.ts), so the container has only the custom workspace mount. Go also mounted the common dir." + + - id: read-configuration.git-worktree-common-dir-empty-workspace-mount-success + lane: semantic + command: read-configuration + priority: p1 + docker_required: true + setup_cmd: "BASE=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $BASE; mkdir -p $BASE; git init -q $BASE/repo && git -C $BASE/repo -c user.email=p@p -c user.name=p commit -q --allow-empty -m init && git -C $BASE/repo worktree add -q ../wt && printf '%s\\n' 'gitdir: ../repo/.git/worktrees/wt' > $BASE/wt/.git; mkdir -p $BASE/wt/.devcontainer; printf '%s' '{\"image\":\"alpine:3\",\"remoteUser\":\"root\",\"workspaceMount\":\"\"}' > $BASE/wt/.devcontainer/devcontainer.json; echo WORKSPACE=$BASE/wt; echo BASE=$BASE" + ts_cmd: "read-configuration --workspace-folder ${WORKSPACE} --mount-git-worktree-common-dir" + cleanup_cmd: "rm -rf ${BASE}; true" + key_order: ["workspace"] + asserts: [exit_code, stdout_normalized] + class: output-shape + current_status: match + notes: "An empty workspaceMount is set: TS readDevContainerConfigFile (configContainer.ts) tests `'workspaceMount' in config`, so read-configuration reports workspaceMount as \"\" beside the computed workspaceFolder and additionalMountString. Go replaced it with the computed bind mount." + + - id: read-configuration.git-worktree-common-dir-null-workspace-mount-and-folder-success + lane: semantic + command: read-configuration + priority: p1 + docker_required: true + setup_cmd: "BASE=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $BASE; mkdir -p $BASE; git init -q $BASE/repo && git -C $BASE/repo -c user.email=p@p -c user.name=p commit -q --allow-empty -m init && git -C $BASE/repo worktree add -q ../wt && printf '%s\\n' 'gitdir: ../repo/.git/worktrees/wt' > $BASE/wt/.git; mkdir -p $BASE/wt/.devcontainer; printf '%s' '{\"image\":\"alpine:3\",\"remoteUser\":\"root\",\"workspaceMount\":null,\"workspaceFolder\":\"/src/wt\"}' > $BASE/wt/.devcontainer/devcontainer.json; echo WORKSPACE=$BASE/wt; echo BASE=$BASE" + ts_cmd: "read-configuration --workspace-folder ${WORKSPACE} --mount-git-worktree-common-dir" + cleanup_cmd: "rm -rf ${BASE}; true" + key_order: ["workspace"] + asserts: [exit_code, stdout_normalized] + class: output-shape + current_status: match + notes: "A null workspaceMount with workspaceFolder: TS getWorkspaceConfiguration (utils.ts) counts the property as set, so it computes neither the workspace mount nor the common-dir mount, and read-configuration reports workspaceMount as null. Go computed both." + + - id: up.git-worktree-common-dir-empty-workspace-mount-success + lane: runtime + command: up + priority: p1 + docker_required: true + setup_cmd: "BASE=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $BASE; mkdir -p $BASE; git init -q $BASE/repo && git -C $BASE/repo -c user.email=p@p -c user.name=p commit -q --allow-empty -m init && git -C $BASE/repo worktree add -q ../wt && printf '%s\\n' 'gitdir: ../repo/.git/worktrees/wt' > $BASE/wt/.git; mkdir -p $BASE/wt/.devcontainer; printf '%s' '{\"image\":\"alpine:3\",\"remoteUser\":\"root\",\"workspaceMount\":\"\"}' > $BASE/wt/.devcontainer/devcontainer.json; echo WORKSPACE=$BASE/wt; echo BASE=$BASE" + ts_cmd: "up --workspace-folder ${WORKSPACE} --mount-git-worktree-common-dir --buildkit=never --id-label parity.case=${PARITY_CASE_ID}-${PARITY_SIDE}" + verify_cmd: "docker inspect -f '{{range .Mounts}}{{.Type}}:{{.Source}}:{{.Destination}} {{end}}' ${CONTAINER_ID}" + cleanup_cmd: "IDS=$(docker ps -aq --filter label=parity.case=${PARITY_CASE_ID}-${PARITY_SIDE}); [ -n \"$IDS\" ] && docker rm -f $IDS >/dev/null 2>&1; rm -rf ${BASE}; true" + asserts: [exit_code, stdout_normalized] + class: up-runtime + current_status: match + notes: "An empty workspaceMount mounts no workspace: TS spawnDevContainer (singleContainer.ts) passes no --mount for a falsy workspaceMount, so the container has only the worktree common dir. Go bind-mounted the workspace folder." + - id: build.lockfile-written-by-default-success lock: feature-content-image lane: runtime @@ -1432,6 +1685,47 @@ initial_cases: compare_hashes: true notes: "upgrade WITHOUT --dry-run persists the lockfile to disk (.devcontainer-lock.json next to .devcontainer.json, getLockfilePath) and does not emit JSON to stdout; the verify_cmd cats the produced lockfile. Each side works on an isolated tmp copy of the image-with-features fixture. Both resolve the features against ghcr.io to the same digests (sha256 scrubbed by normalization anyway)." + - id: upgrade.err-unresolvable-feature + lane: runtime + command: upgrade + priority: p1 + network_required: true + setup_cmd: "WORKSPACE=/tmp/parity-upgrade-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $WORKSPACE; mkdir -p $WORKSPACE; printf '%s' '{\"image\":\"alpine:3\",\"features\":{\"ghcr.io/devcontainers/features/node:1\":{},\"ghcr.io/devcontainers/features/doesnotexist-parity:1\":{}}}' > $WORKSPACE/.devcontainer.json; echo WORKSPACE=$WORKSPACE" + verify_cmd: "test ! -e ${WORKSPACE}/.devcontainer-lock.json" + cleanup_cmd: "rm -rf /tmp/parity-upgrade-${PARITY_CASE_ID}-${PARITY_SIDE}" + ts_cmd: "upgrade --workspace-folder ${WORKSPACE}" + asserts: [exit_code, stderr_normalized] + class: upgrade-runtime + current_status: match + notes: "upgrade resolves the Features through generateFeaturesConfig (readFeaturesConfig), so a Feature whose manifest cannot be fetched fails the command: exit 1 with 'ERR: Feature '' could not be processed. ...' and no lockfile written (verify_cmd). Go skipped the Feature, wrote a lockfile without it and exited 0." + + - id: upgrade.feature-dependencies-success + lane: runtime + command: upgrade + priority: p1 + network_required: true + setup_cmd: "WORKSPACE=/tmp/parity-upgrade-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $WORKSPACE; mkdir -p $WORKSPACE/.devcontainer/lf; printf '%s' '{\"image\":\"alpine:3\",\"features\":{\"./lf\":{},\"ghcr.io/devcontainers/features/git:1\":{}}}' > $WORKSPACE/.devcontainer/devcontainer.json; printf '%s' '{\"id\":\"lf\",\"version\":\"0.1.0\",\"dependsOn\":{\"ghcr.io/devcontainers/features/common-utils:2\":{}}}' > $WORKSPACE/.devcontainer/lf/devcontainer-feature.json; printf 'echo hi\\n' > $WORKSPACE/.devcontainer/lf/install.sh; echo WORKSPACE=$WORKSPACE" + cleanup_cmd: "rm -rf /tmp/parity-upgrade-${PARITY_CASE_ID}-${PARITY_SIDE}" + ts_cmd: "upgrade --workspace-folder ${WORKSPACE} --dry-run" + asserts: [exit_code, stdout_normalized] + class: upgrade-runtime + current_status: match + notes: "The lockfile holds every resolved OCI Feature, the dependencies a Feature's dependsOn pulls in among them (generateFeaturesConfig computes the dependency graph): a local Feature depending on common-utils:2 adds common-utils:2 to the lockfile, while the local Feature itself is left out. Go looked up only the configured Features and missed common-utils." + + - id: upgrade.feature-target-version-success + lane: runtime + command: upgrade + priority: p1 + network_required: true + setup_cmd: "WORKSPACE=/tmp/parity-upgrade-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $WORKSPACE; mkdir -p $WORKSPACE; cp -R reference/src/test/configs/image-with-features/. $WORKSPACE/; echo WORKSPACE=$WORKSPACE" + verify_cmd: "cat ${WORKSPACE}/.devcontainer.json" + cleanup_cmd: "rm -rf /tmp/parity-upgrade-${PARITY_CASE_ID}-${PARITY_SIDE}" + ts_cmd: "upgrade --workspace-folder ${WORKSPACE} --dry-run --feature ghcr.io/devcontainers/features/docker-in-docker:2.5 --target-version 1" + asserts: [exit_code, stdout_normalized] + class: upgrade-runtime + current_status: match + notes: "--feature/--target-version first rewrite the Feature's key in devcontainer.json to ':' (updateFeatureVersionInConfig), even with --dry-run, and the lockfile is generated from the configuration read again: docker-in-docker:2 becomes docker-in-docker:1 in the file (verify_cmd) and in the printed lockfile. Go validated the flags but never applied them." + - id: read-configuration.missing-required lane: contract command: read-configuration @@ -2068,6 +2362,102 @@ initial_cases: class: required-args current_status: match + - id: up.override-config-missing-error + lane: contract + command: up + priority: p1 + docker_required: false + ts_cmd: "up --workspace-folder src/test/configs/image --override-config path-that-does-not-exist.json" + asserts: [exit_code, stdout_normalized] + class: required-args + current_status: match + notes: "TS reads the override in place of the configuration (readDocument(overrideConfigFile ?? configFile)), so a missing override is the configuration not found, named by the configuration's path: 'Dev container config (/.devcontainer/devcontainer.json) not found.' Go failed with 'read override config …: no such file or directory'." + + - id: run-user-commands.override-config-missing-error + lane: contract + command: run-user-commands + priority: p1 + docker_required: false + ts_cmd: "run-user-commands --workspace-folder path-that-does-not-exist --override-config path-that-does-not-exist.json" + asserts: [exit_code, stdout_normalized] + class: required-args + current_status: match + notes: "A workspace without a devcontainer.json and a missing override: TS names the workspace's default config path (getDefaultDevContainerConfigPath) as not found. Go named the override in a read error." + + - id: read-configuration.id-label-lookup-failure + lane: contract + command: read-configuration + priority: p1 + docker_required: false + setup_cmd: "echo DOCKER_HOST=unix:///nonexistent/devcontainer-cli-parity.sock" + ts_cmd: "read-configuration --id-label parity.case=lookup --override-config src/test/configs/image/.devcontainer.json" + asserts: [exit_code, stdout_normalized] + class: container-runtime + current_status: match + notes: "A container lookup that fails fails read-configuration: TS findContainerAndIdLabels runs findDevContainer's `docker ps -q -a --filter label=…`, whose failure throws ('Command failed: docker ps …', logged), so the command exits 1 with nothing on stdout. Go ignored the failed lookup and printed the configuration." + + - id: read-configuration.workspace-lookup-failure + lane: contract + command: read-configuration + priority: p1 + docker_required: false + setup_cmd: "echo DOCKER_HOST=unix:///nonexistent/devcontainer-cli-parity.sock" + ts_cmd: "read-configuration --workspace-folder src/test/configs/image" + asserts: [exit_code, stdout_normalized] + class: container-runtime + current_status: match + notes: "The workspace's container lookup (devcontainer.local_folder and devcontainer.config_file labels) fails the command the same way when Docker cannot answer: exit 1, nothing on stdout." + + - id: up.host-variable-without-name-error + lane: contract + command: up + 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\":{\"A\":\"${localEnv}\"}}' > $WORKSPACE/.devcontainer/devcontainer.json; echo WORKSPACE=$WORKSPACE" + ts_cmd: "up --workspace-folder ${WORKSPACE}" + cleanup_cmd: "rm -rf ${WORKSPACE}; true" + asserts: [exit_code, stdout_normalized] + class: config-parsing + current_status: match + notes: "A ${localEnv} without a variable name is TS's ContainerError \"'${localEnv}' in devcontainer.json can not be resolved because no environment variable name is given.\" (variableSubstitution.ts), as both message and description. Go prefixed it with 'apply host substitutions: '." + + - id: set-up.host-variable-without-name-error + lane: contract + command: set-up + priority: p1 + docker_required: false + setup_cmd: "WORKSPACE=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $WORKSPACE; mkdir -p $WORKSPACE; printf '%s' '{\"image\":\"alpine:3\",\"containerEnv\":{\"A\":\"${localEnv}\"}}' > $WORKSPACE/devcontainer.json; echo WORKSPACE=$WORKSPACE" + ts_cmd: "set-up --container-id parity-no-such-container --config ${WORKSPACE}/devcontainer.json" + cleanup_cmd: "rm -rf ${WORKSPACE}; true" + asserts: [exit_code, stdout_normalized] + class: config-parsing + current_status: match + notes: "set-up reads --config before it looks for the container; the unresolvable variable is a ContainerError, so its description is its own text. Go described it as 'An error occurred running user commands in the container.' and prefixed the message with 'apply host substitutions: '." + + - id: run-user-commands.workspace-lookup-failure + lane: contract + command: run-user-commands + priority: p1 + docker_required: false + setup_cmd: "echo DOCKER_HOST=unix:///nonexistent/devcontainer-cli-parity.sock" + ts_cmd: "run-user-commands --workspace-folder src/test/configs/image" + asserts: [exit_code, stdout_normalized] + class: container-runtime + current_status: match + notes: "A container lookup that fails is TS findContainerAndIdLabels' error: 'Command failed: docker ps -q -a --filter label=devcontainer.local_folder=… --filter label=devcontainer.config_file=…' as the message, 'An error occurred running user commands in the container.' as the description. Go reported 'Dev container not found.' and looked up by the local folder alone." + + - id: run-user-commands.id-label-lookup-failure + lane: contract + command: run-user-commands + priority: p1 + docker_required: false + setup_cmd: "echo DOCKER_HOST=unix:///nonexistent/devcontainer-cli-parity.sock" + ts_cmd: "run-user-commands --workspace-folder src/test/configs/image --id-label parity.case=lookup" + asserts: [exit_code, stdout_normalized] + class: container-runtime + current_status: match + notes: "The --id-label lookup fails the same way, its message naming the id label filter." + - id: read-configuration.container-config-success lane: runtime command: read-configuration @@ -2080,6 +2470,58 @@ initial_cases: class: container-runtime current_status: match + - id: read-configuration.container-id-override-config-without-workspace-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; printf '%s' '{\"image\":\"alpine:3\",\"name\":\"${localWorkspaceFolder}|${containerWorkspaceFolder}|${containerEnv:FOO}\"}' > $WORKSPACE/override.json; CONTAINER_ID=$(docker run -d -e FOO=bar alpine:3.17 sleep inf); echo WORKSPACE=$WORKSPACE; echo CONTAINER_ID=$CONTAINER_ID" + ts_cmd: "read-configuration --container-id ${CONTAINER_ID} --override-config ${WORKSPACE}/override.json" + cleanup_cmd: "docker rm -f ${CONTAINER_ID} >/dev/null 2>&1; rm -rf ${WORKSPACE}; true" + asserts: [exit_code, stdout_normalized] + class: container-runtime + current_status: match + notes: "--container-id without --workspace-folder has no workspace, so TS's configuration is the override itself (configPath = overrideConfigFile, a file URI): it is printed with ${containerEnv:…} resolved from the container and the workspace variables left as written. Go loaded no configuration and printed an empty one." + + - id: read-configuration.container-id-config-workspace-without-folder-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; printf '%s' '{\"image\":\"alpine:3\",\"workspaceFolder\":\"/wf\",\"workspaceMount\":\"type=bind,source=/a,target=/wf\"}' > $WORKSPACE/devcontainer.json; CONTAINER_ID=$(docker run -d alpine:3.17 sleep inf); echo WORKSPACE=$WORKSPACE; echo CONTAINER_ID=$CONTAINER_ID" + ts_cmd: "read-configuration --container-id ${CONTAINER_ID} --config ${WORKSPACE}/devcontainer.json" + cleanup_cmd: "docker rm -f ${CONTAINER_ID} >/dev/null 2>&1; rm -rf ${WORKSPACE}; true" + asserts: [exit_code, stdout_normalized] + class: container-runtime + current_status: match + notes: "Without a workspace, TS getWorkspaceConfiguration keeps the configuration's own workspaceFolder and workspaceMount, and read-configuration prints them as the workspace. Go printed an empty workspace whenever --container-id came without --workspace-folder." + + - id: read-configuration.container-id-devcontainer-id-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; printf '%s' '{\"image\":\"alpine:3\",\"name\":\"${devcontainerId}\"}' > $WORKSPACE/override.json; CONTAINER_ID=$(docker run -d alpine:3.17 sleep inf); echo WORKSPACE=$WORKSPACE; echo CONTAINER_ID=$CONTAINER_ID" + ts_cmd: "read-configuration --container-id ${CONTAINER_ID} --override-config ${WORKSPACE}/override.json" + cleanup_cmd: "docker rm -f ${CONTAINER_ID} >/dev/null 2>&1; rm -rf ${WORKSPACE}; true" + asserts: [exit_code, stdout_normalized] + class: container-runtime + current_status: match + notes: "With a container, TS readConfiguration resolves ${devcontainerId} in the configuration from the container's id labels (beforeContainerSubstitute), here devcontainer.local_folder=undefined: no workspace folder, and a container without a config file label. Go left ${devcontainerId} as written." + + - id: read-configuration.container-id-undefined-id-labels-success + lane: runtime + command: read-configuration + priority: p1 + docker_required: true + setup_cmd: "CONTAINER_ID=$(docker run -d -l devcontainer.config_file=/parity/devcontainer.json -l 'devcontainer.metadata=[{\"remoteEnv\":{\"DID\":\"${devcontainerId}\"}}]' alpine:3.17 sleep inf); echo CONTAINER_ID=$CONTAINER_ID" + ts_cmd: "read-configuration --container-id ${CONTAINER_ID} --include-merged-configuration" + cleanup_cmd: "docker rm -f ${CONTAINER_ID} >/dev/null 2>&1; true" + asserts: [exit_code, stdout_normalized] + class: container-runtime + current_status: match + notes: "--container-id alone has neither a workspace folder nor a configuration, so TS findContainerAndIdLabels' labels are devcontainer.local_folder=undefined and, for a container with a config file label, devcontainer.config_file=undefined (template literals of undefined); the container metadata's ${devcontainerId} is computed from them. Go used empty values, a different id." + - id: read-configuration.container-metadata-success lane: runtime command: read-configuration @@ -2104,6 +2546,97 @@ initial_cases: class: container-runtime current_status: match + - id: exec.container-id-config-without-workspace-success + lane: runtime + command: exec + priority: p1 + docker_required: true + setup_cmd: "WORKSPACE=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $WORKSPACE; mkdir -p $WORKSPACE; printf '%s' '{\"image\":\"alpine:3\",\"remoteEnv\":{\"PARITY_A\":\"from-config\"}}' > $WORKSPACE/devcontainer.json; CONTAINER_ID=$(docker run -d alpine:3.17 sleep inf); echo WORKSPACE=$WORKSPACE; echo CONTAINER_ID=$CONTAINER_ID" + ts_cmd: "exec --container-id ${CONTAINER_ID} --config ${WORKSPACE}/devcontainer.json printenv PARITY_A" + cleanup_cmd: "docker rm -f ${CONTAINER_ID} >/dev/null 2>&1; rm -rf ${WORKSPACE}; true" + asserts: [exit_code, stdout_normalized] + class: container-runtime + current_status: match + notes: "TS exec reads --config without --workspace-folder (configPath = configFile), so the configuration's remoteEnv reaches the command. Go read a configuration only with --workspace-folder, and printenv found nothing (exit 1)." + + - id: exec.container-id-config-missing-error + lane: runtime + command: exec + priority: p1 + docker_required: true + setup_cmd: "CONTAINER_ID=$(docker run -d alpine:3.17 sleep inf); echo CONTAINER_ID=$CONTAINER_ID" + ts_cmd: "exec --container-id ${CONTAINER_ID} --config /tmp/parity-no-such-config/devcontainer.json true" + cleanup_cmd: "docker rm -f ${CONTAINER_ID} >/dev/null 2>&1; true" + asserts: [exit_code, stdout_normalized] + class: container-runtime + current_status: match + notes: "A --config that does not exist fails exec even with --container-id: TS throws 'Dev container config (…) not found.' before it looks at the container. Go ignored the load error with --container-id and ran the command." + + - id: read-configuration.container-env-without-name-error + 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; printf '%s' '{\"image\":\"alpine:3\",\"remoteEnv\":{\"A\":\"${containerEnv}\"}}' > $WORKSPACE/devcontainer.json; CONTAINER_ID=$(docker run -d alpine:3.17 sleep inf); echo WORKSPACE=$WORKSPACE; echo CONTAINER_ID=$CONTAINER_ID" + ts_cmd: "read-configuration --container-id ${CONTAINER_ID} --config ${WORKSPACE}/devcontainer.json" + cleanup_cmd: "docker rm -f ${CONTAINER_ID} >/dev/null 2>&1; rm -rf ${WORKSPACE}; true" + asserts: [exit_code, stdout_normalized] + class: container-runtime + current_status: match + notes: "A ${containerEnv} without a name is TS's ContainerError; read-configuration fails with it like any error: logged, exit 1, nothing on stdout. Go wrote an error envelope on stdout." + + - id: set-up.container-env-without-name-error + lane: runtime + command: set-up + priority: p1 + docker_required: true + setup_cmd: "WORKSPACE=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $WORKSPACE; mkdir -p $WORKSPACE; printf '%s' '{\"image\":\"alpine:3\",\"remoteEnv\":{\"A\":\"${containerEnv}\"}}' > $WORKSPACE/devcontainer.json; CONTAINER_ID=$(docker run -d alpine:3.17 sleep inf); echo WORKSPACE=$WORKSPACE; echo CONTAINER_ID=$CONTAINER_ID" + ts_cmd: "set-up --container-id ${CONTAINER_ID} --config ${WORKSPACE}/devcontainer.json --skip-post-create" + cleanup_cmd: "docker rm -f ${CONTAINER_ID} >/dev/null 2>&1; rm -rf ${WORKSPACE}; true" + asserts: [exit_code, stdout_normalized] + class: container-runtime + current_status: match + notes: "TS setupInContainer substitutes the container's environment into the configuration whether or not it is reported, so set-up fails with the ContainerError \"'${containerEnv}' in devcontainer.json can not be resolved …\" as message and description. Go reported success without --include-configuration, and with it prefixed the message with 'resolve configuration container variables: ' and named no file." + + - id: exec.container-env-without-name-error + lane: runtime + command: exec + priority: p1 + docker_required: true + setup_cmd: "WORKSPACE=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $WORKSPACE; mkdir -p $WORKSPACE; printf '%s' '{\"image\":\"alpine:3\",\"postCreateCommand\":\"echo ${containerEnv}\"}' > $WORKSPACE/devcontainer.json; CONTAINER_ID=$(docker run -d alpine:3.17 sleep inf); echo WORKSPACE=$WORKSPACE; echo CONTAINER_ID=$CONTAINER_ID" + ts_cmd: "exec --container-id ${CONTAINER_ID} --config ${WORKSPACE}/devcontainer.json true" + cleanup_cmd: "docker rm -f ${CONTAINER_ID} >/dev/null 2>&1; rm -rf ${WORKSPACE}; true" + asserts: [exit_code, stdout_normalized] + class: container-runtime + current_status: match + notes: "TS exec substitutes the container's environment into the whole merged configuration before it runs the command, so an unresolvable variable anywhere in it (here postCreateCommand) fails exec (exit 1). Go substituted remoteEnv alone and ran the command." + + - id: run-user-commands.container-env-without-name-error + lane: runtime + command: run-user-commands + priority: p1 + docker_required: true + setup_cmd: "WORKSPACE=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $WORKSPACE; mkdir -p $WORKSPACE; printf '%s' '{\"image\":\"alpine:3\",\"remoteEnv\":{\"A\":\"${containerEnv}\"}}' > $WORKSPACE/devcontainer.json; CONTAINER_ID=$(docker run -d alpine:3.17 sleep inf); echo WORKSPACE=$WORKSPACE; echo CONTAINER_ID=$CONTAINER_ID" + ts_cmd: "run-user-commands --container-id ${CONTAINER_ID} --config ${WORKSPACE}/devcontainer.json" + cleanup_cmd: "docker rm -f ${CONTAINER_ID} >/dev/null 2>&1; rm -rf ${WORKSPACE}; true" + asserts: [exit_code, stdout_normalized] + class: container-runtime + current_status: match + notes: "TS doRunUserCommands substitutes the container's environment into the configuration first and reports the ContainerError as message and description. Go ran the commands and reported success." + + - id: up.container-env-without-name-error + lane: runtime + command: up + 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\",\"remoteEnv\":{\"A\":\"${containerEnv}\"}}' > $WORKSPACE/.devcontainer/devcontainer.json; echo WORKSPACE=$WORKSPACE" + ts_cmd: "up --workspace-folder ${WORKSPACE} --buildkit=never --skip-post-create --id-label parity.case=${PARITY_CASE_ID}-${PARITY_SIDE}" + cleanup_cmd: "IDS=$(docker ps -aq --filter label=parity.case=${PARITY_CASE_ID}-${PARITY_SIDE}); [ -n \"$IDS\" ] && docker rm -f $IDS >/dev/null 2>&1; rm -rf ${WORKSPACE}; true" + asserts: [exit_code, stdout_normalized] + class: container-runtime + current_status: match + notes: "TS setupInContainer substitutes the container's environment into the configuration and the merged configuration once the container runs, even with --skip-post-create: up fails with the ContainerError and the container's id. Go reported success." + - id: exec.container-metadata-success lane: runtime command: exec @@ -3367,10 +3900,11 @@ initial_cases: priority: p1 network_required: true ts_cmd: "outdated --workspace-folder src/test/configs/image-with-features --output-format json" - asserts: [exit_code, stdout_normalized] + asserts: [exit_code, stdout_normalized, stdout_text] + key_order: ["features"] class: outdated-runtime current_status: match - notes: "outdated resolves current/wanted/latest (+ wantedMajor/latestMajor) per feature; key = full userFeatureId (with tag). Both sides query the same registry, so latest matches between them." + notes: "outdated resolves current/wanted/latest (+ wantedMajor/latestMajor) per feature; key = full userFeatureId (with tag). Both sides query the same registry, so latest matches between them. The Features follow the configuration's order (loadVersionInfo reorders resolved.features), which stdout_text and key_order pin; Go used to sort them by id." - id: upgrade.image-with-features-success lane: runtime diff --git a/internal/cli/build.go b/internal/cli/build.go index 358a1ca..f1eb261 100644 --- a/internal/cli/build.go +++ b/internal/cli/build.go @@ -16,6 +16,7 @@ import ( "github.com/devcontainers/cli/internal/features" "github.com/devcontainers/cli/internal/imagemeta" "github.com/devcontainers/cli/internal/log" + "github.com/devcontainers/cli/internal/stringify" "github.com/spf13/cobra" ) @@ -760,7 +761,7 @@ func buildOptionsFromConfig(cfg *config.DevContainer) []string { // writeSuccessJSON writes a command's result with its keys in order (see // orderedObject). func writeSuccessJSON(out Output, data map[string]any, order ...string) error { - b, err := stringifyJSON(orderedObject{data, order}, "") + b, err := stringify.JSON(orderedObject{data, order}, "") if err != nil { return fmt.Errorf("marshal result: %w", err) } @@ -787,7 +788,7 @@ func writeValidationError(out Output, message string) error { } func writeErrorJSON(out Output, errOutput coreerrors.ErrorOutput) error { - b, _ := stringifyJSON(errOutput, "") + b, _ := stringify.JSON(errOutput, "") fmt.Fprintln(out.Stdout(), string(b)) return &coreerrors.ExitCodeError{Code: 1, Err: errors.New(errOutput.Description)} } @@ -863,6 +864,7 @@ func (r *buildRunner) buildCompose(ctx context.Context, cfg *config.DevContainer docker: dockerClient, engine: engine, opts: &upOpts{ + workspaceFolder: opts.workspaceFolder, cacheFrom: opts.cacheFrom, buildNoCache: opts.noCache, skipFeatureAutoMapping: opts.skipFeatureAutoMapping, @@ -966,6 +968,7 @@ func (r *buildRunner) featureBuildOptions(cfg *config.DevContainer, hostSub *con HostSub: hostSub, SkipPersistCustoms: opts.skipPersistCustoms, FeaturesBasePath: filepath.Dir(cfg.ConfigFilePath), + WorkspaceFolder: opts.workspaceFolder, AdditionalMetadata: []imagemeta.Entry{configToMetadataEntry(cfg)}, LabelMetadata: []imagemeta.Entry{r.configLabel}, } diff --git a/internal/cli/cli_parity_test.go b/internal/cli/cli_parity_test.go index 3fddf05..0add28a 100644 --- a/internal/cli/cli_parity_test.go +++ b/internal/cli/cli_parity_test.go @@ -34,6 +34,9 @@ func readConfigurationResult(t *testing.T, fixtureName string) map[string]any { if _, err := os.Stat(fixtureDir); os.IsNotExist(err) { t.Skip("fixture not found: " + fixtureName) } + // read-configuration looks for the workspace's container, so it needs an + // engine to answer; this one has none. + startFakeEngine(t, nil) old := os.Stdout r, w, _ := os.Pipe() diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index 81baa46..8c76107 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -95,6 +95,8 @@ func TestReadConfigurationFixtures(t *testing.T) { } func TestReadConfiguration_AllFixtures(t *testing.T) { + // read-configuration looks for the container; this engine has none. + startFakeEngine(t, nil) fixturesDir := filepath.Join("..", "..", "src", "test", "configs") if _, err := os.Stat(fixturesDir); os.IsNotExist(err) { t.Skip("fixtures not found") diff --git a/internal/cli/collection_commands.go b/internal/cli/collection_commands.go index ba31efd..0ed8ef2 100644 --- a/internal/cli/collection_commands.go +++ b/internal/cli/collection_commands.go @@ -19,6 +19,7 @@ import ( "github.com/devcontainers/cli/internal/jsonc" "github.com/devcontainers/cli/internal/log" "github.com/devcontainers/cli/internal/oci" + "github.com/devcontainers/cli/internal/stringify" "github.com/iancoleman/orderedmap" "github.com/spf13/cobra" ) @@ -450,7 +451,7 @@ func publishCollectionWith(ctx context.Context, out Output, reg oci.Registry, ta } logger.Write("Published collection metadata.", log.LevelInfo) - data, _ := stringifyJSON(result, "") + data, _ := stringify.JSON(result, "") fmt.Fprintln(out.Stdout(), string(data)) return nil } diff --git a/internal/cli/compose_up.go b/internal/cli/compose_up.go index d7bdd34..e1bcc04 100644 --- a/internal/cli/compose_up.go +++ b/internal/cli/compose_up.go @@ -311,7 +311,7 @@ func (r *upRunner) composeFeatureSets(ctx context.Context, cfg *config.DevContai return nil, "", err } } - res, err := fetchFeatureSetsWithOrder(ctx, r.log, cfg.Features, filepath.Dir(cfg.ConfigFilePath), opts.skipFeatureAutoMapping, lockfile, cfg.OverrideFeatureInstallOrder) + res, err := fetchFeatureSetsWithOrder(ctx, r.log, cfg.Features, localFeatureRoots{filepath.Dir(cfg.ConfigFilePath), opts.workspaceFolder}, opts.skipFeatureAutoMapping, lockfile, cfg.OverrideFeatureInstallOrder) if err != nil { return nil, "", err } diff --git a/internal/cli/compose_up_held_test.go b/internal/cli/compose_up_held_test.go index f05ebb5..5bed224 100644 --- a/internal/cli/compose_up_held_test.go +++ b/internal/cli/compose_up_held_test.go @@ -295,7 +295,7 @@ func newComposeHarness(t *testing.T) *composeHarness { log: logger, docker: &docker.Client{DockerPath: "docker", Log: logger, Runner: run}, engine: &docker.EngineClient{API: engine, Log: logger}, - opts: &upOpts{userDataFolder: persisted, updateRemoteUserUIDDefault: "off", gpuAvailability: "none"}, + opts: &upOpts{workspaceFolder: ws, userDataFolder: persisted, updateRemoteUserUIDDefault: "off", gpuAvailability: "none"}, // The fixtures' metadata names no variables. substitute: func(*[]imagemeta.Entry) error { return nil }, }, diff --git a/internal/cli/container_lookup.go b/internal/cli/container_lookup.go index 0f90af4..98daf55 100644 --- a/internal/cli/container_lookup.go +++ b/internal/cli/container_lookup.go @@ -3,6 +3,7 @@ package cli import ( "context" "fmt" + "strings" ) // containerLister is the narrow seam resolveContainerID needs (satisfied by @@ -11,32 +12,77 @@ type containerLister interface { ListContainers(ctx context.Context, all bool, labelFilters []string) ([]string, error) } +// containerListError is a failed container lookup, worded as TS's: its +// findDevContainer runs `docker ps -q -a --filter label=…` and the failure is +// "Command failed: " (commonUtils.ts). The engine's error is +// kept for errors.Is/As. +type containerListError struct { + labels []string + err error +} + +func (e *containerListError) Error() string { + var b strings.Builder + b.WriteString("Command failed: docker ps -q -a") + for _, l := range e.labels { + b.WriteString(" --filter label=" + l) + } + return b.String() +} + +func (e *containerListError) Unwrap() error { return e.err } + +// listContainers is ListContainers with a failure as a containerListError. +func listContainers(ctx context.Context, engine containerLister, labels []string) ([]string, error) { + ids, err := engine.ListContainers(ctx, true, labels) + if err != nil { + return nil, &containerListError{labels: labels, err: err} + } + return ids, nil +} + // resolveContainerID finds a workspace's dev container. Explicit idLabels win. // Otherwise it prefers the [local_folder, config_file] label filter so that // multiple devcontainer.json configs in one project (e.g. .devcontainer/frontend // and .devcontainer/backend, each its own container) resolve to the RIGHT one, // and falls back to local_folder alone for a container created without a // config_file label (older runs / --id-label-only) — mirroring the TS CLI's -// old/new id-label scheme. Returns "" when nothing matches. -func resolveContainerID(ctx context.Context, engine containerLister, workspaceFolder, configFile string, idLabels []string) string { +// old/new id-label scheme. Returns "" when nothing matches, and the first +// lookup that fails as a containerListError, as TS's findContainerAndIdLabels +// throws. +func resolveContainerID(ctx context.Context, engine containerLister, workspaceFolder, configFile string, idLabels []string) (string, error) { if len(idLabels) > 0 { - if ids, _ := engine.ListContainers(ctx, true, idLabels); len(ids) > 0 { - return ids[0] + ids, err := listContainers(ctx, engine, idLabels) + if err != nil || len(ids) == 0 { + return "", err } - return "" + return ids[0], nil } if workspaceFolder == "" { - return "" + return "", nil } local := fmt.Sprintf("devcontainer.local_folder=%s", resolvePath(workspaceFolder)) if configFile != "" { labels := []string{local, fmt.Sprintf("devcontainer.config_file=%s", configFile)} - if ids, _ := engine.ListContainers(ctx, true, labels); len(ids) > 0 { - return ids[0] + ids, err := listContainers(ctx, engine, labels) + if err != nil { + return "", err + } + if len(ids) > 0 { + return ids[0], nil } } - if ids, _ := engine.ListContainers(ctx, true, []string{local}); len(ids) > 0 { - return ids[0] + ids, err := listContainers(ctx, engine, []string{local}) + if err != nil || len(ids) == 0 { + return "", err } - return "" + return ids[0], nil +} + +// failingLister is the containerLister without a Docker engine: every lookup +// fails with the engine's error. +type failingLister struct{ err error } + +func (f failingLister) ListContainers(context.Context, bool, []string) ([]string, error) { + return nil, f.err } diff --git a/internal/cli/container_lookup_test.go b/internal/cli/container_lookup_test.go index 55c2d6a..61fb3f2 100644 --- a/internal/cli/container_lookup_test.go +++ b/internal/cli/container_lookup_test.go @@ -2,6 +2,7 @@ package cli import ( "context" + "errors" "strings" "testing" ) @@ -28,10 +29,10 @@ func TestResolveContainerID(t *testing.T) { key(local, "devcontainer.config_file="+cfgB): {"idB"}, key(local): {"idA", "idB"}, // both share local_folder }} - if got := resolveContainerID(context.Background(), f, ws, cfgB, nil); got != "idB" { + if got, err := resolveContainerID(context.Background(), f, ws, cfgB, nil); err != nil || got != "idB" { t.Errorf("cfgB -> %q, want idB", got) } - if got := resolveContainerID(context.Background(), f, ws, cfgA, nil); got != "idA" { + if got, err := resolveContainerID(context.Background(), f, ws, cfgA, nil); err != nil || got != "idA" { t.Errorf("cfgA -> %q, want idA", got) } }) @@ -40,7 +41,7 @@ func TestResolveContainerID(t *testing.T) { f := &fakeLister{byFilter: map[string][]string{ key(local): {"legacy"}, // no config_file-filtered entry }} - if got := resolveContainerID(context.Background(), f, ws, cfgA, nil); got != "legacy" { + if got, err := resolveContainerID(context.Background(), f, ws, cfgA, nil); err != nil || got != "legacy" { t.Errorf("-> %q, want legacy (fallback)", got) } }) @@ -50,14 +51,51 @@ func TestResolveContainerID(t *testing.T) { key("x=y"): {"bylabel"}, key(local): {"byfolder"}, }} - if got := resolveContainerID(context.Background(), f, ws, cfgA, []string{"x=y"}); got != "bylabel" { + if got, err := resolveContainerID(context.Background(), f, ws, cfgA, []string{"x=y"}); err != nil || got != "bylabel" { t.Errorf("-> %q, want bylabel", got) } }) t.Run("no match", func(t *testing.T) { - if got := resolveContainerID(context.Background(), &fakeLister{}, ws, cfgA, nil); got != "" { + if got, err := resolveContainerID(context.Background(), &fakeLister{}, ws, cfgA, nil); err != nil || got != "" { t.Errorf("-> %q, want empty", got) } }) + + t.Run("a failed lookup is TS's failed docker ps", func(t *testing.T) { + boom := errors.New("boom") + failing := failingLister{boom} + for name, tc := range map[string]struct { + ws, cfg string + labels []string + want string + }{ + "id labels": {ws, cfgA, []string{"x=y", "a=b"}, "Command failed: docker ps -q -a --filter label=x=y --filter label=a=b"}, + "folder and config": {ws, cfgA, nil, "Command failed: docker ps -q -a --filter label=" + local + " --filter label=devcontainer.config_file=" + cfgA}, + "folder without config": {ws, "", nil, "Command failed: docker ps -q -a --filter label=" + local}, + } { + got, err := resolveContainerID(context.Background(), failing, tc.ws, tc.cfg, tc.labels) + if got != "" || err == nil || err.Error() != tc.want || !errors.Is(err, boom) { + t.Errorf("%s: %q, %v; want %q wrapping the engine's error", name, got, err, tc.want) + } + } + // The fallback lookup failing after a config-file miss fails too. + half := &halfLister{fail: key(local), err: boom} + if _, err := resolveContainerID(context.Background(), half, ws, cfgA, nil); err == nil || err.Error() != "Command failed: docker ps -q -a --filter label="+local { + t.Errorf("fallback lookup: err = %v", err) + } + }) +} + +// halfLister finds nothing, and fails the lookup by the fail filter. +type halfLister struct { + fail string + err error +} + +func (h *halfLister) ListContainers(_ context.Context, _ bool, labels []string) ([]string, error) { + if strings.Join(labels, "|") == h.fail { + return nil, h.err + } + return nil, nil } diff --git a/internal/cli/e2e_test.go b/internal/cli/e2e_test.go index f2b2636..0362c16 100644 --- a/internal/cli/e2e_test.go +++ b/internal/cli/e2e_test.go @@ -98,6 +98,8 @@ func TestE2E_BuildUpExec_Dockerfile(t *testing.T) { } func TestE2E_ReadConfiguration_AllVariants(t *testing.T) { + // read-configuration looks for the container; this engine has none. + startFakeEngine(t, nil) fixtures := []struct { name string checkFn func(t *testing.T, cfg map[string]any) diff --git a/internal/cli/env.go b/internal/cli/env.go index 026dec7..74ff903 100644 --- a/internal/cli/env.go +++ b/internal/cli/env.go @@ -1,16 +1,38 @@ package cli import ( + "encoding/json" "os" "github.com/devcontainers/cli/internal/config" ) -func resolveContainerVariables(containerEnv map[string]string, value any) (any, error) { - return config.NewVariableResolver().AfterContainer(config.SubstitutionContext{ - Platform: "linux", - ContainerEnv: containerEnv, - }, value) +// resolveContainerVariables is TS containerSubstitute: ${containerEnv:…} +// resolved from the container's environment. A ${containerEnv} without a name +// is TS's ContainerError, which names configFile (the configuration's file, +// "" without one). +func resolveContainerVariables(containerEnv map[string]string, configFile string, value any) (any, error) { + // The container's platform is not win32, so variable names are matched as + // written (the zero Platform). + ctx := config.SubstitutionContext{ContainerEnv: containerEnv} + ctx.ConfigFilePath = configFile + return config.NewVariableResolver().AfterContainer(ctx, value) +} + +// checkContainerSubstitution substitutes the container's environment into +// each value (the configuration, the merged configuration) for the failure +// alone, as TS's commands run containerSubstitute over them before they use +// the container: the first error, a ContainerError, is returned. +func checkContainerSubstitution(containerEnv map[string]string, configFile string, values ...any) error { + for _, v := range values { + data, _ := json.Marshal(v) + var generic any + json.Unmarshal(data, &generic) + if _, err := resolveContainerVariables(containerEnv, configFile, generic); err != nil { + return err + } + } + return nil } // osEnvMap returns the current process environment as a map. diff --git a/internal/cli/exec.go b/internal/cli/exec.go index e5f5e61..e4a7fe8 100644 --- a/internal/cli/exec.go +++ b/internal/cli/exec.go @@ -4,8 +4,10 @@ import ( "cmp" "context" "encoding/json" + "errors" "fmt" "io" + "io/fs" "maps" "os" "os/exec" @@ -149,15 +151,13 @@ func runExec(ctx context.Context, opts *execOpts, cmdArgs []string) error { Writer: logDst, }) - // 1. If workspace-folder provided without container-id, validate config exists early - var loadResult *config.LoadResult - if opts.workspaceFolder != "" { - cwd, _ := os.Getwd() - var loadErr error - loadResult, loadErr = config.LoadDevContainerConfig(resolvePath(opts.workspaceFolder), execConfigPath(opts.configPath, cwd), "") - if err := execConfigError(loadErr, opts.containerID); err != nil { - return err - } + // 1. The configuration, read before the container is looked for, as TS + // exec does: the workspace's, --config's or --override-config's, with or + // without --container-id. One that is named but cannot be read fails exec. + cwd, _ := os.Getwd() + loadResult, loadErr := execLoadConfig(opts, cwd) + if loadErr != nil { + return loadErr } engine, err := docker.NewEngineClient(logger) @@ -173,7 +173,10 @@ func runExec(ctx context.Context, opts *execOpts, cmdArgs []string) error { if loadResult != nil { configFile = loadResult.Config.ConfigFilePath } - containerID = findContainerByOpts(ctx, engine, opts, configFile, logger) + var lookupErr error + if containerID, lookupErr = findContainerByOpts(ctx, engine, opts, configFile, logger); lookupErr != nil { + return lookupErr + } } if containerID == "" { @@ -204,6 +207,16 @@ func runExec(ctx context.Context, opts *execOpts, cmdArgs []string) error { return err } merged := imagemeta.MergeConfiguration(entries) + // TS exec substitutes the container's environment into the merged + // configuration before it probes the user environment; a variable that + // cannot be resolved fails exec, naming the configuration's file. + var rawConfig any + if loadResult != nil { + rawConfig = loadResult.Raw + } + if err := checkContainerSubstitution(containerEnv, configFile, rawConfig, merged); err != nil { + return err + } // The user is the merged configuration's remoteUser - the configuration's, // else the image metadata's - else the container's, else root; the // environment is probed as that user (TS createContainerProperties with @@ -249,14 +262,24 @@ func execConfigPath(configPath, cwd string) string { return configPath } -// execConfigError is the configuration error that stops exec: with -// --container-id the container is known, and a configuration that does not -// load only loses its settings. -func execConfigError(loadErr error, containerID string) error { - if loadErr != nil && containerID == "" { - return loadErr +// execLoadConfig is TS exec's configuration: none without --workspace-folder, +// --config or --override-config, else the one they name, which must exist +// ("Dev container config (…) not found." names --config, else the workspace's +// default path), even with --container-id. +func execLoadConfig(opts *execOpts, cwd string) (*config.LoadResult, error) { + if opts.workspaceFolder == "" && opts.configPath == "" && opts.overrideConfig == "" { + return nil, nil + } + ws := "" + if opts.workspaceFolder != "" { + ws = resolvePath(opts.workspaceFolder) } - return nil + configPath := execConfigPath(opts.configPath, cwd) + loaded, err := config.LoadDevContainerConfig(ws, configPath, execConfigPath(opts.overrideConfig, cwd)) + if errors.Is(err, fs.ErrNotExist) { + return nil, &coreerrors.ContainerError{Description: fmt.Sprintf("Dev container config (%s) not found.", configPath)} + } + return loaded, err } // containerRunning reports whether an inspected container is running. @@ -299,10 +322,14 @@ func execRemoteEnv(probed map[string]string, metadata map[string]*string, cfg *c } } } + configFile := "" + if cfg != nil { + configFile = cfg.ConfigFilePath + } for k, v := range resolved { - substituted, err := resolveContainerVariables(containerEnv, v) + substituted, err := resolveContainerVariables(containerEnv, configFile, v) if err != nil { - return nil, fmt.Errorf("resolve container variables: %w", err) + return nil, err } if s, ok := substituted.(string); ok { resolved[k] = s @@ -398,13 +425,15 @@ func (w *rawLogWriter) Write(p []byte) (int, error) { return len(p), nil } -func findContainerByOpts(ctx context.Context, engine *docker.EngineClient, opts *execOpts, configFile string, logger log.Logger) string { +func findContainerByOpts(ctx context.Context, engine *docker.EngineClient, opts *execOpts, configFile string, logger log.Logger) (string, error) { // [local_folder, config_file] disambiguates when a project has multiple // devcontainer.json configs; falls back to local_folder for legacy containers. - if id := resolveContainerID(ctx, engine, opts.workspaceFolder, configFile, opts.idLabels); id != "" { - return id + // A failed lookup fails exec, as TS's findContainerAndIdLabels throws. + id, err := resolveContainerID(ctx, engine, opts.workspaceFolder, configFile, opts.idLabels) + if err != nil || id != "" { + return id, err } - return findComposeContainer(ctx, engine, opts, logger) + return findComposeContainer(ctx, engine, opts, logger), nil } func findComposeContainer(ctx context.Context, engine *docker.EngineClient, opts *execOpts, logger log.Logger) string { diff --git a/internal/cli/exec_held_test.go b/internal/cli/exec_held_test.go index 8c5ad48..f782883 100644 --- a/internal/cli/exec_held_test.go +++ b/internal/cli/exec_held_test.go @@ -10,6 +10,7 @@ import ( "testing" "github.com/devcontainers/cli/internal/config" + coreerrors "github.com/devcontainers/cli/internal/errors" "github.com/devcontainers/cli/internal/lifecycle" "github.com/devcontainers/cli/internal/log" "github.com/moby/moby/api/types/container" @@ -69,16 +70,37 @@ func TestExecConfigPath(t *testing.T) { } } -func TestExecConfigError(t *testing.T) { - loadErr := errors.New("no config") - if err := execConfigError(loadErr, ""); err != loadErr { - t.Errorf("without a container id: err = %v, want the load error", err) +// TS exec reads the configuration the flags name, with or without +// --container-id, and fails when it cannot: --config without +// --workspace-folder is read, and a missing one is not found. +func TestExecLoadConfig(t *testing.T) { + dir := t.TempDir() + cfg := filepath.Join(dir, "c.json") + if err := os.WriteFile(cfg, []byte(`{"image":"img","remoteUser":"u"}`), 0o644); err != nil { + t.Fatal(err) + } + override := filepath.Join(dir, "o.json") + if err := os.WriteFile(override, []byte(`{"image":"img","remoteUser":"o"}`), 0o644); err != nil { + t.Fatal(err) + } + if got, err := execLoadConfig(&execOpts{containerID: "c"}, dir); got != nil || err != nil { + t.Errorf("nothing named = %v, %v; want no configuration", got, err) + } + if got, err := execLoadConfig(&execOpts{containerID: "c", configPath: "c.json"}, dir); err != nil || got.Config.RemoteUser != "u" || got.Config.ConfigFilePath != cfg { + t.Errorf("--config with --container-id = %+v, %v; want c.json read", got, err) } - if err := execConfigError(loadErr, "c"); err != nil { - t.Errorf("with a container id: err = %v, want none", err) + if got, err := execLoadConfig(&execOpts{containerID: "c", overrideConfig: "o.json"}, dir); err != nil || got.Config.RemoteUser != "o" { + t.Errorf("--override-config = %+v, %v; want o.json read", got, err) } - if err := execConfigError(nil, ""); err != nil { - t.Errorf("loaded: err = %v", err) + missing := filepath.Join(dir, "missing.json") + want := "Dev container config (" + missing + ") not found." + if _, err := execLoadConfig(&execOpts{containerID: "c", configPath: "missing.json"}, dir); err == nil || err.Error() != want { + t.Errorf("missing --config: err = %v, want %q", err, want) + } + ws := t.TempDir() + want = "Dev container config (" + filepath.Join(ws, ".devcontainer", "devcontainer.json") + ") not found." + if _, err := execLoadConfig(&execOpts{containerID: "c", workspaceFolder: ws}, dir); err == nil || err.Error() != want { + t.Errorf("workspace without a configuration: err = %v, want %q", err, want) } } @@ -134,6 +156,32 @@ func TestExecRemoteEnv(t *testing.T) { if _, err := execRemoteEnv(nil, map[string]*string{"X": str("${containerEnv}")}, nil, nil); err == nil { t.Error("an unresolvable ${containerEnv}: want an error") } + // TS's ContainerError text, naming the configuration's file. + withFile := &config.DevContainer{ConfigFilePath: "/ws/c.json", RemoteEnv: map[string]*string{"X": str("${containerEnv}")}} + wantErr := "'${containerEnv}' in c.json can not be resolved because no environment variable name is given." + if _, err := execRemoteEnv(nil, nil, withFile, nil); err == nil || err.Error() != wantErr { + t.Errorf("unresolvable in the configuration: err = %v, want %q", err, wantErr) + } +} + +// TS runs containerSubstitute over the whole configuration and merged +// configuration: an unnamed ${containerEnv} anywhere is its ContainerError, +// named ones resolve. +func TestCheckContainerSubstitution(t *testing.T) { + env := map[string]string{"P": "/bin"} + ok := map[string]any{"postCreateCommand": "echo ${containerEnv:P}"} + bad := map[string]any{"postCreateCommand": []any{"echo", "${containerEnv}"}} + if err := checkContainerSubstitution(env, "/ws/c.json", ok, nil); err != nil { + t.Errorf("resolvable: err = %v", err) + } + want := "'${containerEnv}' in c.json can not be resolved because no environment variable name is given." + err := checkContainerSubstitution(env, "/ws/c.json", ok, bad) + if ce, isCE := coreerrors.AsContainerError(err); !isCE || ce.Description != want { + t.Errorf("unresolvable in the second value: err = %v, want the ContainerError %q", err, want) + } + if err := checkContainerSubstitution(env, "", bad); err == nil || err.Error() != "'${containerEnv}' can not be resolved because no environment variable name is given." { + t.Errorf("without a configuration file: err = %v", err) + } } func TestExecDockerArgs(t *testing.T) { diff --git a/internal/cli/exec_repo_held_test.go b/internal/cli/exec_repo_held_test.go index 111be88..32657c1 100644 --- a/internal/cli/exec_repo_held_test.go +++ b/internal/cli/exec_repo_held_test.go @@ -47,11 +47,23 @@ func TestFindContainerByOptsPrefersTheLabeledContainer(t *testing.T) { "labeled": {"devcontainer.local_folder": ws}, }) opts := &execOpts{workspaceFolder: ws} - if got := findContainerByOpts(t.Context(), fakeEngineClient(t), opts, "", quietLogger()); got != "labeled" { + if got, err := findContainerByOpts(t.Context(), fakeEngineClient(t), opts, "", quietLogger()); err != nil || got != "labeled" { t.Errorf("container = %q, want labeled", got) } } +// A container lookup that fails fails exec with TS's failed `docker ps`, +// rather than looking further or reporting no container. +func TestFindContainerByOptsFailedLookup(t *testing.T) { + ws := composeWorkspace(t) + t.Setenv("DOCKER_HOST", "unix:///nonexistent/devcontainer-cli-test.sock") + opts := &execOpts{workspaceFolder: ws, idLabels: []string{"a=b"}} + got, err := findContainerByOpts(t.Context(), fakeEngineClient(t), opts, "", quietLogger()) + if want := "Command failed: docker ps -q -a --filter label=a=b"; got != "" || err == nil || err.Error() != want { + t.Errorf("container, err = %q, %v; want %q", got, err, want) + } +} + // A Compose workspace's container without devcontainer labels is found by the // project (the workspace folder's name, or COMPOSE_PROJECT_NAME) and service. func TestFindContainerByOptsFindsTheComposeService(t *testing.T) { @@ -63,11 +75,11 @@ func TestFindContainerByOptsFindsTheComposeService(t *testing.T) { }) engine := fakeEngineClient(t) opts := &execOpts{workspaceFolder: ws} - if got := findContainerByOpts(t.Context(), engine, opts, "", quietLogger()); got != "app" { + if got, err := findContainerByOpts(t.Context(), engine, opts, "", quietLogger()); err != nil || got != "app" { t.Errorf("container = %q, want app", got) } t.Setenv("COMPOSE_PROJECT_NAME", "Custom") - if got := findContainerByOpts(t.Context(), engine, opts, "", quietLogger()); got != "custom" { + if got, err := findContainerByOpts(t.Context(), engine, opts, "", quietLogger()); err != nil || got != "custom" { t.Errorf("container with COMPOSE_PROJECT_NAME = %q, want custom", got) } } diff --git a/internal/cli/feature_deps.go b/internal/cli/feature_deps.go index 7cbd289..3cbdc2a 100644 --- a/internal/cli/feature_deps.go +++ b/internal/cli/feature_deps.go @@ -26,7 +26,7 @@ func renderDependencyMermaid(client oci.Registry, logger log.Logger, roots []str Options: map[string]any{}, }) } - graph, err := features.BuildDependencyGraph(logger, newMetadataProcessFeature(client, logger, "", nil), userFeatures, nil, nil) + graph, err := features.BuildDependencyGraph(logger, newMetadataProcessFeature(client, logger, localFeatureRoots{}, nil), userFeatures, nil, nil) if err != nil { logger.Write(fmt.Sprintf("Could not build dependency graph: %v", err), log.LevelTrace) return "flowchart\n" @@ -39,18 +39,17 @@ func renderDependencyMermaid(client oci.Registry, logger log.Logger, roots []str // dependsOn, installsAfter). It does not stage install content, so it is cheap // enough for the resolve-dependencies and mermaid consumers. OCI metadata is read // annotation-first with a blob fallback, matching the TS getOCIFeatureMetadata. -func newMetadataProcessFeature(client oci.Registry, logger log.Logger, basePath string, lockfile *features.Lockfile) features.ProcessFeature { +func newMetadataProcessFeature(client oci.Registry, logger log.Logger, local localFeatureRoots, lockfile *features.Lockfile) features.ProcessFeature { return func(node *features.FNode) (*features.Set, error) { id := node.UserFeatureID srcType := features.ClassifyID(id) switch srcType { case features.SourceLocalPath: - resolvedPath := id - if !filepath.IsAbs(resolvedPath) { - resolvedPath = filepath.Join(basePath, resolvedPath) + resolvedPath, ok, err := local.resolve(logger, id) + if !ok { + return nil, err } - resolvedPath = filepath.Clean(resolvedPath) meta, err := readLocalFeatureMetadata(resolvedPath) if err != nil { return nil, fmt.Errorf("ERR: Feature '%s' could not be processed. %w", id, err) diff --git a/internal/cli/feature_deps_held_test.go b/internal/cli/feature_deps_held_test.go index faa61d4..1001455 100644 --- a/internal/cli/feature_deps_held_test.go +++ b/internal/cli/feature_deps_held_test.go @@ -1,7 +1,9 @@ package cli import ( + "bytes" "net/http" + "path/filepath" "reflect" "strings" "testing" @@ -11,43 +13,70 @@ import ( "github.com/devcontainers/cli/internal/oci" ) -func resolveMetadata(t *testing.T, reg oci.Registry, basePath string, lockfile *features.Lockfile, id string, opts any) (*features.Set, error) { +func resolveMetadata(t *testing.T, reg oci.Registry, local localFeatureRoots, lockfile *features.Lockfile, id string, opts any) (*features.Set, error) { t.Helper() - return newMetadataProcessFeature(reg, log.Null, basePath, lockfile)(&features.FNode{UserFeatureID: id, Options: opts}) + return newMetadataProcessFeature(reg, log.Null, local, lockfile)(&features.FNode{UserFeatureID: id, Options: opts}) } // A local Feature's metadata is its devcontainer-feature.json, its id the // user's reference when the file declares none, and its value the options. func TestMetadataProcessFeatureLocal(t *testing.T) { - base := t.TempDir() + roots := devcontainerRoots(t) + base := roots.configDir writeFeature(t, base, "named", `{"id":"declared","dependsOn":{"./other":{}}}`) writeFeature(t, base, "anonymous", `{"version":"1.0.0"}`) opts := map[string]any{"flag": true} - set, err := resolveMetadata(t, nil, base, nil, "./named", opts) + set, err := resolveMetadata(t, nil, roots, nil, "./named", opts) if err != nil { t.Fatal(err) } if f := set.Features[0]; f.ID != "declared" || !reflect.DeepEqual(f.Value, opts) || f.DependsOn["./other"] == nil { t.Errorf("feature = %+v, want id declared, the options and the dependsOn", f) } - set, err = resolveMetadata(t, nil, base, nil, "./anonymous", nil) + set, err = resolveMetadata(t, nil, roots, nil, "./anonymous", nil) if err != nil { t.Fatal(err) } if got := set.Features[0].ID; got != "./anonymous" { t.Errorf("id = %q, want the reference ./anonymous", got) } - if _, err := resolveMetadata(t, nil, base, nil, "./missing", nil); err == nil || !strings.HasPrefix(err.Error(), "ERR: Feature './missing' could not be processed.") { + if _, err := resolveMetadata(t, nil, roots, nil, "./missing", nil); err == nil || !strings.HasPrefix(err.Error(), "ERR: Feature './missing' could not be processed.") { t.Errorf("missing local Feature: err = %v", err) } } +// resolve-dependencies refuses a local Feature outside the workspace's +// .devcontainer folder as TS processFeatureIdentifier does: the reason is +// logged and no Feature set comes back, which the graph fails; without a +// configuration (features info's graph) a local Feature is refused too. +func TestMetadataProcessFeatureRefusesLocalFeaturesOutsideDevcontainer(t *testing.T) { + roots := devcontainerRoots(t) + writeFeature(t, roots.workspaceRoot, "feat", `{"version":"1.0.0"}`) + for _, tc := range []struct { + roots localFeatureRoots + logged string + }{ + {roots, "Local file path parse error. Resolved path must be a child of the .devcontainer/ folder. Parsed: " + filepath.Join(roots.workspaceRoot, "feat")}, + {localFeatureRoots{}, "A local feature requires a configuration path."}, + } { + var logs bytes.Buffer + logger := log.New(log.Options{Level: log.LevelInfo, Format: "text", Writer: &logs}) + set, err := newMetadataProcessFeature(nil, logger, tc.roots, nil)(&features.FNode{UserFeatureID: "../feat"}) + if set != nil || err != nil { + t.Errorf("set, err = %+v, %v; want neither", set, err) + } + if !strings.Contains(logs.String(), tc.logged+"\n") { + t.Errorf("logs = %q, want %q", logs.String(), tc.logged) + } + } +} + // A direct tarball's metadata is the devcontainer-feature.json inside it. func TestMetadataProcessFeatureTarball(t *testing.T) { opts := map[string]any{"v": "1"} uri := serveBytes(t, featureTgzBytes(t, map[string]string{"devcontainer-feature.json": `{"id":"tb","installsAfter":["x"]}`}), http.StatusOK) + "/f.tgz" - set, err := resolveMetadata(t, nil, "", nil, uri, opts) + set, err := resolveMetadata(t, nil, localFeatureRoots{}, nil, uri, opts) if err != nil { t.Fatal(err) } @@ -55,11 +84,11 @@ func TestMetadataProcessFeatureTarball(t *testing.T) { t.Errorf("feature = %+v", f) } missing := serveBytes(t, nil, http.StatusNotFound) + "/f.tgz" - if _, err := resolveMetadata(t, nil, "", nil, missing, nil); err == nil || !strings.HasPrefix(err.Error(), "ERR: Feature '"+missing+"' could not be processed.") { + if _, err := resolveMetadata(t, nil, localFeatureRoots{}, nil, missing, nil); err == nil || !strings.HasPrefix(err.Error(), "ERR: Feature '"+missing+"' could not be processed.") { t.Errorf("unreachable tarball: err = %v", err) } notTar := serveBytes(t, []byte("not a tarball"), http.StatusOK) + "/f.tgz" - if _, err := resolveMetadata(t, nil, "", nil, notTar, nil); err == nil { + if _, err := resolveMetadata(t, nil, localFeatureRoots{}, nil, notTar, nil); err == nil { t.Error("a tarball that does not extract must fail") } } @@ -85,7 +114,7 @@ func TestMetadataProcessFeatureOCI(t *testing.T) { t.Run(name, func(t *testing.T) { reg := newOCIFeatureRegistry() manifest := reg.add(t, "ghcr.io/owner/coll/hello:1", layer, tc.annotation) - set, err := resolveMetadata(t, reg, "", nil, id, opts) + set, err := resolveMetadata(t, reg, localFeatureRoots{}, nil, id, opts) if err != nil { t.Fatal(err) } @@ -105,7 +134,7 @@ func TestMetadataProcessFeatureOCI(t *testing.T) { m := reg.add(t, "ghcr.io/owner/coll/other:1", layer, nil) reg.manifests["ghcr.io/owner/coll/broken:1"] = &oci.ManifestContainer{Manifest: &oci.Manifest{Layers: []oci.Layer{{Digest: "sha256:" + strings.Repeat("e", 64)}, m.Manifest.Layers[0]}}} for _, ref := range []string{"ghcr.io/owner/coll/hello:1", "ghcr.io/owner/coll/broken:1"} { - set, err := resolveMetadata(t, reg, "", nil, ref, nil) + set, err := resolveMetadata(t, reg, localFeatureRoots{}, nil, ref, nil) if err != nil { t.Fatal(err) } @@ -120,7 +149,7 @@ func TestMetadataProcessFeatureOCI(t *testing.T) { reg := newOCIFeatureRegistry() reg.add(t, "ghcr.io/owner/coll/hello@"+digest, layer, nil) lf := &features.Lockfile{Features: map[string]features.LockfileEntry{id: {Integrity: digest}}} - if _, err := resolveMetadata(t, reg, "", lf, id, nil); err != nil { + if _, err := resolveMetadata(t, reg, localFeatureRoots{}, lf, id, nil); err != nil { t.Fatal(err) } if want := []string{"ghcr.io/owner/coll/hello@" + digest}; !reflect.DeepEqual(reg.fetched, want) { @@ -144,7 +173,7 @@ func TestMetadataProcessFeatureOCI(t *testing.T) { if tc.setup != nil { tc.setup(reg) } - if _, err := resolveMetadata(t, reg, "", nil, tc.id, nil); err == nil || !strings.Contains(err.Error(), tc.want) { + if _, err := resolveMetadata(t, reg, localFeatureRoots{}, nil, tc.id, nil); err == nil || !strings.Contains(err.Error(), tc.want) { t.Fatalf("err = %v, want %q", err, tc.want) } }) diff --git a/internal/cli/feature_install.go b/internal/cli/feature_install.go index f7646d3..8b09244 100644 --- a/internal/cli/feature_install.go +++ b/internal/cli/feature_install.go @@ -2,11 +2,13 @@ package cli import ( "archive/tar" + "cmp" "compress/gzip" "context" "crypto/sha256" "encoding/hex" "encoding/json" + "errors" "fmt" "io" "maps" @@ -55,6 +57,9 @@ type FeatureBuildOptions struct { SkipFeatureAutoMapping bool SkipPersistCustoms bool FeaturesBasePath string + // WorkspaceFolder is the workspace root local Features must lie under + // (in its .devcontainer folder); "" stands for the working directory. + WorkspaceFolder string // AdditionalMetadata follows the Features' entries in the metadata the // Features' users are found in (TS findContainerUsers), variables // substituted; LabelMetadata follows them in the image's @@ -90,8 +95,34 @@ type fetchFeatureResult struct { // fetchFeatureSets fetches features, without a lockfile or an install-order // override, and returns them in install order. -func fetchFeatureSets(ctx context.Context, logger log.Logger, featuresCfg map[string]any, featuresBasePath string, skipAutoMapping bool) (*fetchFeatureResult, error) { - return fetchFeatureSetsWithOrder(ctx, logger, featuresCfg, featuresBasePath, skipAutoMapping, nil, nil) +func fetchFeatureSets(ctx context.Context, logger log.Logger, featuresCfg map[string]any, local localFeatureRoots, skipAutoMapping bool) (*fetchFeatureResult, error) { + return fetchFeatureSetsWithOrder(ctx, logger, featuresCfg, local, skipAutoMapping, nil, nil) +} + +// localFeatureRoots are what a local Feature reference (./x) resolves +// against: the folder of the configuration that lists it ("" when there is no +// configuration) and the workspace root (TS params.cwd: the workspace folder, +// or the working directory without one), whose .devcontainer folder the +// Feature must lie in. +type localFeatureRoots struct { + configDir string + workspaceRoot string +} + +// resolve returns a local Feature's folder. A reference TS +// processFeatureIdentifier refuses is logged as the error TS writes and +// reported as no Feature set (nil, nil), which the dependency graph fails as +// a Feature that could not be processed, as TS does. +func (r localFeatureRoots) resolve(logger log.Logger, id string) (string, bool, error) { + path, err := features.LocalFeaturePath(id, r.configDir, r.workspaceRoot) + if invalid, ok := errors.AsType[*features.InvalidLocalFeatureError](err); ok { + logger.Write(invalid.Msg, log.LevelError) + return "", false, nil + } + if err != nil { + return "", false, err + } + return path, true, nil } // createFeaturesFolder creates the folder the Features are staged in, TS @@ -109,7 +140,7 @@ func createFeaturesFolder() (string, error) { // overrideFeatureInstallOrder and returning the FeatureSets in install // order. Each returned Set's content is staged in the features folder at its // consecutiveId, `_`, as TS fetchFeatures does. -func fetchFeatureSetsWithOrder(ctx context.Context, logger log.Logger, featuresCfg map[string]any, featuresBasePath string, skipAutoMapping bool, lockfile *features.Lockfile, overrideOrder []string) (*fetchFeatureResult, error) { +func fetchFeatureSetsWithOrder(ctx context.Context, logger log.Logger, featuresCfg map[string]any, local localFeatureRoots, skipAutoMapping bool, lockfile *features.Lockfile, overrideOrder []string) (*fetchFeatureResult, error) { if len(featuresCfg) == 0 { return nil, nil } @@ -142,7 +173,7 @@ func fetchFeatureSetsWithOrder(ctx context.Context, logger log.Logger, featuresC } processFeature := func(node *features.FNode) (*features.Set, error) { - return processInstallFeature(logger, ociClient, node, featuresBasePath, skipAutoMapping, lockfile, nextStageDir) + return processInstallFeature(logger, ociClient, node, local, skipAutoMapping, lockfile, nextStageDir) } graph, err := features.BuildDependencyGraph(logger, processFeature, userFeatures, overrideOrder, lockfile) @@ -209,6 +240,8 @@ func featureFromMetadata(m features.Feature, id string, opts any, cachePath stri Customizations: m.Customizations, Included: true, CachePath: cachePath, + Metadata: m.Metadata, + SourceKeys: features.LocalFeatureKeys, } } @@ -220,7 +253,7 @@ func processInstallFeature( logger log.Logger, ociClient oci.Registry, node *features.FNode, - featuresBasePath string, + local localFeatureRoots, skipAutoMapping bool, lockfile *features.Lockfile, nextStageDir func() string, @@ -250,11 +283,10 @@ func processInstallFeature( switch srcType { case features.SourceLocalPath: - resolvedPath := id - if !filepath.IsAbs(resolvedPath) { - resolvedPath = filepath.Join(featuresBasePath, resolvedPath) + resolvedPath, ok, err := local.resolve(logger, id) + if !ok { + return nil, err } - resolvedPath = filepath.Clean(resolvedPath) info, statErr := os.Stat(resolvedPath) if statErr != nil || !info.IsDir() { @@ -341,6 +373,9 @@ func processInstallFeature( return nil, fmt.Errorf("ERR: Feature '%s' could not be processed. %w", id, err) } + // featureRef is the reference as written, even when the lockfile pins + // the manifest (TS tryGetOCIFeatureSet parses the user's id). + userRef := ref // Pin to the digest recorded in the lockfile (reproducible resolution), // matching the TS CLI which fetches the manifest by the lockfile integrity. if lockfile != nil { @@ -378,14 +413,17 @@ func processInstallFeature( } logger.Write(fmt.Sprintf("Extracted feature %s (%d bytes)", ref.ID, len(blobData)), log.LevelTrace) - // Blob-fallback metadata: the extracted devcontainer-feature.json. + // Blob-fallback metadata: the extracted devcontainer-feature.json, JSONC + // as TS's jsonc.parse reads it. var m features.Feature if metaData, readErr := os.ReadFile(filepath.Join(featureDir, "devcontainer-feature.json")); readErr == nil { - json.Unmarshal(metaData, &m) + jsonc.Unmarshal(metaData, &m) } + // TS's Feature takes its id from the metadata it spreads over + // {id: , ...}: the layer's here, the annotation's below. feat := features.Feature{ - ID: ref.ID, + ID: cmp.Or(m.ID, ref.ID), Version: m.Version, Name: m.Name, Description: m.Description, @@ -407,13 +445,15 @@ func processInstallFeature( Customizations: m.Customizations, Included: true, CachePath: featureDir, + Metadata: m.Metadata, + File: m.Metadata, + SourceKeys: features.OCIFeatureKeys, } if feat.Version == "" { feat.Version = ref.Tag } userIDNoVer := featureIDWithoutVersion(id) - owner, _, _ := strings.Cut(ref.Namespace, "/") set := &features.Set{ SourceInfo: &features.OCISource{ @@ -422,16 +462,19 @@ func processInstallFeature( ID: ref.ID, Resource: ref.Resource, Tag: ref.Tag, ManifestDigest: manifest.ContentDigest, UserID: id, UserFeatureIdWithoutVersion: userIDNoVer, - FeatureRef: map[string]string{ - "id": ref.ID, "namespace": ref.Namespace, - "owner": owner, - "path": ref.Namespace + "/" + ref.ID, - "registry": ref.Registry, - "scheme": ref.Scheme(), - "resource": ref.Resource, - "tag": ref.Tag, "version": ref.Tag, + FeatureRef: &features.FeatureRef{ + Scheme: userRef.Scheme(), + ID: userRef.ID, + Owner: userRef.Owner, + Namespace: userRef.Namespace, + Registry: userRef.Registry, + Resource: userRef.Resource, + Path: userRef.Path, + Version: userRef.Version, + Tag: userRef.Tag, + Digest: userRef.Digest, }, - Manifest: manifest.Manifest, + Manifest: features.ManifestJSON(manifest.ManifestBytes, manifest.Manifest), }, Features: []features.Feature{feat}, ComputedDigest: manifest.ContentDigest, @@ -455,6 +498,10 @@ func processInstallFeature( set.Features[0].Entrypoint = am.Entrypoint set.Features[0].Mounts = am.Mounts set.Features[0].Customizations = am.Customizations + // TS reads the dependencies from the annotation and + // spreads it over the Feature (buildDependencyGraph). + set.Features[0].Metadata = am.Metadata + set.Features[0].ID = cmp.Or(am.ID, ref.ID) } } } @@ -659,7 +706,7 @@ func extendImageWithFeatures( return nil, err } } - result, err := fetchFeatureSetsWithOrder(ctx, logger, featuresCfg, fbOpts.FeaturesBasePath, fbOpts.SkipFeatureAutoMapping, lockfile, fbOpts.OverrideFeatureInstallOrder) + result, err := fetchFeatureSetsWithOrder(ctx, logger, featuresCfg, localFeatureRoots{fbOpts.FeaturesBasePath, fbOpts.WorkspaceFolder}, fbOpts.SkipFeatureAutoMapping, lockfile, fbOpts.OverrideFeatureInstallOrder) if err != nil { return nil, err } diff --git a/internal/cli/feature_install_held_test.go b/internal/cli/feature_install_held_test.go index a0e953d..7f12abc 100644 --- a/internal/cli/feature_install_held_test.go +++ b/internal/cli/feature_install_held_test.go @@ -121,7 +121,7 @@ func stageDirs(t *testing.T) func() string { func installFeature(t *testing.T, reg oci.Registry, id string, opts any, lockfile *features.Lockfile) (*features.Set, error) { t.Helper() - return processInstallFeature(log.Null, reg, &features.FNode{UserFeatureID: id, Options: opts}, t.TempDir(), false, lockfile, stageDirs(t)) + return processInstallFeature(log.Null, reg, &features.FNode{UserFeatureID: id, Options: opts}, localFeatureRoots{}, false, lockfile, stageDirs(t)) } // An OCI Feature's identity, version and staged content come from the @@ -180,9 +180,8 @@ func TestProcessInstallFeatureOCI(t *testing.T) { if !ok { t.Fatalf("source = %T", set.SourceInfo) } - ref := src.FeatureRef.(map[string]string) - if ref["owner"] != "owner" || ref["path"] != "owner/coll/hello" || ref["tag"] != "1" { - t.Errorf("featureRef = %v", ref) + if ref := src.FeatureRef; ref == nil || ref.Owner != "owner" || ref.Path != "owner/coll/hello" || ref.Tag != "1" || ref.Version != "1" { + t.Errorf("featureRef = %+v", ref) } if src.UserFeatureIdWithoutVersion != "ghcr.io/owner/coll/hello" || set.ComputedDigest != manifest.ContentDigest { t.Errorf("userFeatureIdWithoutVersion = %q, computedDigest = %q", src.UserFeatureIdWithoutVersion, set.ComputedDigest) @@ -290,7 +289,7 @@ func TestProcessInstallFeatureOCIErrors(t *testing.T) { if stage == nil { stage = stageDirs(t) } - _, err := processInstallFeature(log.Null, reg, &features.FNode{UserFeatureID: featureID}, t.TempDir(), false, nil, stage) + _, err := processInstallFeature(log.Null, reg, &features.FNode{UserFeatureID: featureID}, localFeatureRoots{}, false, nil, stage) if err == nil || !strings.Contains(err.Error(), tc.wantErr) { t.Fatalf("err = %v, want %q", err, tc.wantErr) } @@ -375,7 +374,7 @@ func TestProcessInstallFeatureTarballErrors(t *testing.T) { }) t.Run("staging folder not writable", func(t *testing.T) { uri := serveBytes(t, blob, http.StatusOK) + "/f.tgz" - _, err := processInstallFeature(log.Null, nil, &features.FNode{UserFeatureID: uri}, t.TempDir(), false, nil, func() string { return filepath.Join(blocker, "stage") }) + _, err := processInstallFeature(log.Null, nil, &features.FNode{UserFeatureID: uri}, localFeatureRoots{}, false, nil, func() string { return filepath.Join(blocker, "stage") }) if err == nil || !strings.Contains(err.Error(), "write feature tarball") { t.Fatalf("err = %v", err) } @@ -400,7 +399,7 @@ func TestTarballFeatureIDUsesTheURLPath(t *testing.T) { // always names the same. func TestFetchFeatureSetsReportsTheFirstSortedFailure(t *testing.T) { for range 5 { - _, err := fetchFeatureSets(t.Context(), log.Null, map[string]any{"./b-missing": true, "./a-missing": true, "./c-missing": true}, t.TempDir(), false) + _, err := fetchFeatureSets(t.Context(), log.Null, map[string]any{"./b-missing": true, "./a-missing": true, "./c-missing": true}, devcontainerRoots(t), false) if err == nil || !strings.Contains(err.Error(), "'./a-missing'") { t.Fatalf("err = %v, want the failure of ./a-missing", err) } @@ -579,11 +578,11 @@ type extendResult struct { func (e extendRun) run(t *testing.T) extendResult { t.Helper() - base := t.TempDir() - writeLocalFeature(t, base, "hello", nil) + roots := devcontainerRoots(t) + writeLocalFeature(t, roots.configDir, "hello", nil) r := &fakeDockerRunner{reply: e.reply} opts := e.fbOpts - opts.FeaturesBasePath = base + opts.FeaturesBasePath, opts.WorkspaceFolder = roots.configDir, roots.workspaceRoot cfg := e.features if cfg == nil { cfg = map[string]any{"./hello": map[string]any{}} @@ -849,11 +848,11 @@ func TestExtendImageWithFeaturesBridgesRegistryAuth(t *testing.T) { t.Setenv("DOCKER_CONFIG", ambient) t.Setenv("DEVCONTAINERS_OCI_AUTH", "registry.example.com|user|secret") - base := t.TempDir() - writeLocalFeature(t, base, "hello", nil) + roots := devcontainerRoots(t) + writeLocalFeature(t, roots.configDir, "hello", nil) client := &docker.Client{DockerPath: script, Env: []string{"FAKE_DOCKER_LOG=" + logPath}, Log: log.Null} _, err := extendImageWithFeatures(t.Context(), log.Null, client, &docker.EngineClient{API: &fakeDockerAPI{}}, "base:latest", - map[string]any{"./hello": true}, false, []string{"registry.example.com/app:1"}, &FeatureBuildOptions{FeaturesBasePath: base}) + map[string]any{"./hello": true}, false, []string{"registry.example.com/app:1"}, &FeatureBuildOptions{FeaturesBasePath: roots.configDir, WorkspaceFolder: roots.workspaceRoot}) if err != nil { t.Fatal(err) } @@ -927,3 +926,68 @@ func TestRealignFeatureDirsSkipsUnstagedSets(t *testing.T) { t.Errorf("unstaged Feature = %+v, want it untouched", f) } } + +// An OCI Feature set is written as TS's featuresConfiguration has it: the +// sourceInformation properties in tryGetOCIFeatureSet's order, the manifest +// as the registry served it, featureRef parsed from the reference as written +// even when the lockfile pins the manifest, and the Feature built from the +// annotation's metadata spread over the layer's devcontainer-feature.json. +func TestProcessInstallFeatureOCISetJSON(t *testing.T) { + const id = "ghcr.io/owner/coll/hello:1" + digest := "sha256:" + strings.Repeat("cd", 32) + annotation := `{"version":"1.2.0","id":"hello","options":{"b":{"type":"string","default":""},"a":{"default":true,"type":"boolean"}}}` + reg := newOCIFeatureRegistry() + m := reg.add(t, "ghcr.io/owner/coll/hello@"+digest, map[string]string{"devcontainer-feature.json": "// layer\n{\"name\":\"Hello\",\"id\":\"hello\",\"version\":\"1.0.0\",\"keywords\":[\"k\"],}"}, &annotation) + m.ManifestBytes = []byte(`{"schemaVersion":2,"config":{"mediaType":"application/vnd.devcontainers","digest":"sha256:c","size":2},"layers":[]}`) + lf := &features.Lockfile{Features: map[string]features.LockfileEntry{id: {Integrity: digest}}} + set, err := installFeature(t, reg, id, map[string]any{}, lf) + if err != nil { + t.Fatal(err) + } + set.Features[0].CachePath, set.Features[0].ConsecutiveId = "/c", "hello_0" + got, err := json.Marshal(set) + if err != nil { + t.Fatal(err) + } + want := `{"sourceInformation":{"type":"oci",` + + `"manifest":{"schemaVersion":2,"config":{"mediaType":"application/vnd.devcontainers","digest":"sha256:c","size":2},"layers":[]},` + + `"manifestDigest":"` + m.ContentDigest + `",` + + `"featureRef":{"scheme":"https","id":"hello","owner":"owner","namespace":"owner/coll","registry":"ghcr.io","resource":"ghcr.io/owner/coll/hello","path":"owner/coll/hello","version":"1","tag":"1"},` + + `"userFeatureId":"` + id + `","userFeatureIdWithoutVersion":"ghcr.io/owner/coll/hello"},` + + `"features":[{"name":"Hello","id":"hello","version":"1.2.0","keywords":["k"],"included":true,"value":{},` + + `"options":{"b":{"type":"string","default":""},"a":{"default":true,"type":"boolean"}},"cachePath":"/c","consecutiveId":"hello_0"}],` + + `"internalVersion":"2","computedDigest":"` + m.ContentDigest + `"}` + if string(got) != want { + t.Errorf("set JSON:\ngot %s\nwant %s", got, want) + } +} + +// An OCI Feature's id is its metadata's, as TS spreads the metadata over +// {id: , ...}: the annotation's when the manifest has one, +// else the layer's, else the reference's. consecutiveId follows it. +func TestProcessInstallFeatureOCIIDIsTheMetadatas(t *testing.T) { + const id = "ghcr.io/owner/coll/hello:1" + renamed, anonymous := `{"id":"new-hello"}`, `{"version":"1.0.0"}` + for name, tc := range map[string]struct { + layer string + annotation *string + want string + }{ + "annotation": {`{"id":"layer-hello"}`, &renamed, "new-hello"}, + "annotation without id": {`{"id":"layer-hello"}`, &anonymous, "hello"}, + "layer": {`{"id":"layer-hello"}`, nil, "layer-hello"}, + "neither": {`{"version":"1.0.0"}`, nil, "hello"}, + } { + t.Run(name, func(t *testing.T) { + reg := newOCIFeatureRegistry() + reg.add(t, id, map[string]string{"devcontainer-feature.json": tc.layer}, tc.annotation) + set, err := installFeature(t, reg, id, nil, nil) + if err != nil { + t.Fatal(err) + } + if got := set.Features[0].ID; got != tc.want { + t.Errorf("id = %q, want %q", got, tc.want) + } + }) + } +} diff --git a/internal/cli/feature_install_nodocker_held_test.go b/internal/cli/feature_install_nodocker_held_test.go index a4c2630..af527b3 100644 --- a/internal/cli/feature_install_nodocker_held_test.go +++ b/internal/cli/feature_install_nodocker_held_test.go @@ -18,7 +18,8 @@ import ( // it is written into the extended image, and the build still reports the // image's names. func TestExtendImageWithFeaturesResolvesFeatureHostVars(t *testing.T) { - base := t.TempDir() + roots := devcontainerRoots(t) + base := roots.configDir dir := filepath.Join(base, "envy") if err := os.MkdirAll(dir, 0o755); err != nil { t.Fatal(err) @@ -33,6 +34,7 @@ func TestExtendImageWithFeaturesResolvesFeatureHostVars(t *testing.T) { names, err := extendImageWithFeatures(t.Context(), log.Null, fakeDockerClient(r), &docker.EngineClient{API: &fakeDockerAPI{version: "25.0.0"}}, "base:latest", map[string]any{"./envy": true}, false, []string{"out:1"}, &FeatureBuildOptions{ FeaturesBasePath: base, + WorkspaceFolder: roots.workspaceRoot, HostSub: &config.HostSubContext{Platform: "linux", Env: map[string]string{"HOST_VALUE": "resolved-on-host"}}, }) if err != nil { diff --git a/internal/cli/feature_install_test.go b/internal/cli/feature_install_test.go index 66df1b1..8f19669 100644 --- a/internal/cli/feature_install_test.go +++ b/internal/cli/feature_install_test.go @@ -1,17 +1,33 @@ package cli import ( + "bytes" "encoding/json" + "fmt" "os" "path/filepath" "strings" "testing" + coreerrors "github.com/devcontainers/cli/internal/errors" "github.com/devcontainers/cli/internal/features" "github.com/devcontainers/cli/internal/imagemeta" "github.com/devcontainers/cli/internal/log" ) +// devcontainerRoots creates a workspace with an empty .devcontainer folder and +// returns the roots of a configuration in that folder, where local Features +// must lie. +func devcontainerRoots(t *testing.T) localFeatureRoots { + t.Helper() + ws := t.TempDir() + dir := filepath.Join(ws, ".devcontainer") + if err := os.Mkdir(dir, 0o755); err != nil { + t.Fatal(err) + } + return localFeatureRoots{configDir: dir, workspaceRoot: ws} +} + func TestFetchFeatureSets(t *testing.T) { tests := []struct { name string @@ -134,9 +150,9 @@ func TestFetchFeatureSets(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - baseDir := t.TempDir() - tt.setup(t, baseDir) - result, err := fetchFeatureSets(t.Context(), log.Null, tt.entries, baseDir, false) + roots := devcontainerRoots(t) + tt.setup(t, roots.configDir) + result, err := fetchFeatureSets(t.Context(), log.Null, tt.entries, roots, false) if result != nil && result.TmpDir != "" { defer os.RemoveAll(result.TmpDir) } @@ -213,7 +229,8 @@ func TestFeatureMetadataEntry_UsesOCIUserID(t *testing.T) { // devcontainer-feature.json id, else the folder name; its name, when the file // has none, is the user's reference. func TestFetchFeatureSets_StagingLayout(t *testing.T) { - baseDir := t.TempDir() + roots := devcontainerRoots(t) + baseDir := roots.configDir writeLocalFeature(t, baseDir, "dir-a", map[string]any{"./dir-b": true}) dirB := filepath.Join(baseDir, "dir-b") if err := os.MkdirAll(dirB, 0o755); err != nil { @@ -225,7 +242,7 @@ func TestFetchFeatureSets_StagingLayout(t *testing.T) { if err := os.WriteFile(filepath.Join(dirB, "install.sh"), []byte("#!/bin/sh\n"), 0o755); err != nil { t.Fatal(err) } - result, err := fetchFeatureSets(t.Context(), log.Null, map[string]any{"./dir-a": true}, baseDir, false) + result, err := fetchFeatureSets(t.Context(), log.Null, map[string]any{"./dir-a": true}, roots, false) if err != nil { t.Fatal(err) } @@ -390,3 +407,56 @@ func TestFeatureMetadataEntryCarriesEveryLifecycleHook(t *testing.T) { t.Errorf("merged updateContentCommands = %#v, want the Feature's command", merged.UpdateContentCommands) } } + +// A local Feature outside the workspace's .devcontainer folder, or given by an +// absolute path, is refused as TS processFeatureIdentifier refuses it: the +// reason is logged as an error and the Feature fails as one that could not be +// processed, the plain Error build and up report as their message. +func TestFetchFeatureSetsRefusesLocalFeaturesOutsideDevcontainer(t *testing.T) { + roots := devcontainerRoots(t) + writeLocalFeature(t, roots.workspaceRoot, "feat", nil) + writeLocalFeature(t, roots.configDir, "inside", nil) + for _, tc := range []struct{ id, logged string }{ + {"../feat", "Local file path parse error. Resolved path must be a child of the .devcontainer/ folder. Parsed: " + filepath.Join(roots.workspaceRoot, "feat")}, + {filepath.Join(roots.configDir, "inside"), "An Absolute path to a local feature is not allowed."}, + } { + t.Run(tc.id, func(t *testing.T) { + var logs bytes.Buffer + logger := log.New(log.Options{Level: log.LevelInfo, Format: "text", Writer: &logs}) + result, err := fetchFeatureSets(t.Context(), logger, map[string]any{tc.id: true}, roots, false) + if result != nil { + t.Fatalf("result = %+v, want none", result) + } + msg := "ERR: Feature '" + tc.id + "' could not be processed. You may not have permission to access this Feature, or may not be logged in. If the issue persists, report this to the Feature author." + if err == nil || err.Error() != msg { + t.Fatalf("err = %v, want %q", err, msg) + } + if !strings.Contains(logs.String(), tc.logged+"\n") { + t.Errorf("logs = %q, want %q", logs.String(), tc.logged) + } + // up wraps it; both report the message TS's plain Error has. + for _, e := range []error{err, fmt.Errorf("install features: %w", err)} { + want := coreerrors.ErrorOutput{Outcome: "error", Message: msg, Description: "An error occurred building the container."} + if got := commandErrorOutput(e, "An error occurred building the container"); got != want { + t.Errorf("commandErrorOutput = %+v, want %+v", got, want) + } + } + }) + } +} + +// read-configuration resolves local Features against its workspace folder: +// a configuration at the workspace root (.devcontainer.json) has its Features +// outside .devcontainer, and the command fails with no result. +func TestReadConfigurationRefusesALocalFeatureOutsideDevcontainer(t *testing.T) { + startFakeEngine(t, nil) + ws := writeConfigFixture(t, map[string]string{ + ".devcontainer.json": `{"image":"alpine:3","features":{"./feat":{}}}`, + "feat/devcontainer-feature.json": `{"id":"feat","version":"1.0.0"}`, + }) + stdout, err := readConfigurationStdout("--workspace-folder", ws, "--include-merged-configuration") + if err == nil || !strings.HasPrefix(err.Error(), "ERR: Feature './feat' could not be processed.") || stdout != "" { + t.Fatalf("stdout = %q, err = %v; want the Feature's failure and no result", stdout, err) + } + wantExitCode(t, err, 1) +} diff --git a/internal/cli/feature_tarball_test.go b/internal/cli/feature_tarball_test.go index 870bf5b..1ab6087 100644 --- a/internal/cli/feature_tarball_test.go +++ b/internal/cli/feature_tarball_test.go @@ -150,7 +150,7 @@ func TestDirectTarballDigestCheckedAgainstTheLockfile(t *testing.T) { digest := "sha256:" + hex.EncodeToString(sum[:]) resolve := func(lf *features.Lockfile) error { - _, err := newMetadataProcessFeature(nil, log.Null, "", lf)(&features.FNode{UserFeatureID: uri}) + _, err := newMetadataProcessFeature(nil, log.Null, localFeatureRoots{}, lf)(&features.FNode{UserFeatureID: uri}) return err } wrong := &features.Lockfile{Features: map[string]features.LockfileEntry{uri: {Integrity: "sha256:" + strings.Repeat("0", 64)}}} diff --git a/internal/cli/features_info.go b/internal/cli/features_info.go index 2f9f2e2..582ae11 100644 --- a/internal/cli/features_info.go +++ b/internal/cli/features_info.go @@ -10,6 +10,7 @@ import ( "github.com/devcontainers/cli/internal/log" "github.com/devcontainers/cli/internal/oci" + "github.com/devcontainers/cli/internal/stringify" "github.com/spf13/cobra" ) @@ -85,7 +86,7 @@ func runFeaturesInfo(ctx context.Context, out Output, mode, featureID, logLevel, if json.Indent(&buf, manifest.ManifestBytes, "", " ") == nil { fmt.Fprintln(out.Stdout(), buf.String()) } else { - data, _ := stringifyJSON(manifest.Manifest, " ") + data, _ := stringify.JSON(manifest.Manifest, " ") fmt.Fprintln(out.Stdout(), string(data)) } fmt.Fprintln(out.Stdout()) @@ -126,7 +127,7 @@ func runFeaturesInfo(ctx context.Context, out Output, mode, featureID, logLevel, } if outputFormat == "json" { - data, _ := stringifyJSON(jsonOutput, " ") + data, _ := stringify.JSON(jsonOutput, " ") fmt.Fprintln(out.Stdout(), string(data)) } diff --git a/internal/cli/features_info_held_test.go b/internal/cli/features_info_held_test.go index 520ce80..9c56bc1 100644 --- a/internal/cli/features_info_held_test.go +++ b/internal/cli/features_info_held_test.go @@ -20,11 +20,12 @@ type readRegistry struct { host string tags map[string][]string // repository -> tags manifests map[string][]byte // repository -> manifest served for any reference + blobs map[string][]byte // digest -> blob, from any repository } func newReadRegistry(t *testing.T) *readRegistry { t.Helper() - r := &readRegistry{tags: map[string][]string{}, manifests: map[string][]byte{}} + r := &readRegistry{tags: map[string][]string{}, manifests: map[string][]byte{}, blobs: map[string][]byte{}} srv := httptest.NewServer(http.HandlerFunc(r.serve)) t.Cleanup(srv.Close) // httptest listens on 127.0.0.1, which the OCI client (as the reference) @@ -58,6 +59,13 @@ func (r *readRegistry) serve(w http.ResponseWriter, req *http.Request) { return } } + if i := strings.Index(path, "/blobs/"); i >= 0 { + if body, ok := r.blobs[path[i+len("/blobs/"):]]; ok { + w.Header().Set("Content-Length", fmt.Sprint(len(body))) + _, _ = w.Write(body) + return + } + } if path == "" { w.WriteHeader(http.StatusOK) return diff --git a/internal/cli/features_resolve_deps.go b/internal/cli/features_resolve_deps.go index 9e55eb4..87d1a62 100644 --- a/internal/cli/features_resolve_deps.go +++ b/internal/cli/features_resolve_deps.go @@ -10,6 +10,7 @@ import ( coreerrors "github.com/devcontainers/cli/internal/errors" "github.com/devcontainers/cli/internal/features" "github.com/devcontainers/cli/internal/log" + "github.com/devcontainers/cli/internal/stringify" "github.com/spf13/cobra" ) @@ -68,7 +69,7 @@ func realFeaturesResolveDepsCmd() *cobra.Command { // diagram and the install order from it via a single builder. The // previous edge-less node list ignored transitive dependencies in // the install order. - processFeature := newMetadataProcessFeature(ociClient, logger, basePath, lockfile) + processFeature := newMetadataProcessFeature(ociClient, logger, localFeatureRoots{basePath, ws}, lockfile) graph, err := features.BuildDependencyGraph(logger, processFeature, userFeatures, cfg.OverrideFeatureInstallOrder, lockfile) if err != nil { return err @@ -102,7 +103,7 @@ func realFeaturesResolveDepsCmd() *cobra.Command { // install-order JSON. fmt.Fprintln(out.Stdout(), features.GenerateMermaidDiagram(graph)) - data, _ := stringifyJSON(map[string]any{"installOrder": entries}, " ") + data, _ := stringify.JSON(map[string]any{"installOrder": entries}, " ") fmt.Fprintln(out.Stdout(), string(data)) return nil }, diff --git a/internal/cli/metadata_label.go b/internal/cli/metadata_label.go index 1ddf683..b4bff2d 100644 --- a/internal/cli/metadata_label.go +++ b/internal/cli/metadata_label.go @@ -1,6 +1,7 @@ package cli import ( + "cmp" "encoding/json" "fmt" @@ -112,13 +113,15 @@ func imageMetadataFromContainer(labels map[string]string, cfg *config.DevContain // found without the CLI's own lookup: the --id-label values, else the // workspace folder's and the config file's labels, the config file's only when // the container has one (an older container has the workspace folder's alone). +// A missing workspace folder or config file is written as TS's template +// literal writes undefined. func containerIDLabels(provided []string, containerLabels map[string]string, workspaceFolder, configFile string) []string { if len(provided) > 0 { return provided } - labels := []string{"devcontainer.local_folder=" + workspaceFolder} + labels := []string{"devcontainer.local_folder=" + cmp.Or(workspaceFolder, "undefined")} if containerLabels["devcontainer.config_file"] != "" { - labels = append(labels, "devcontainer.config_file="+configFile) + labels = append(labels, "devcontainer.config_file="+cmp.Or(configFile, "undefined")) } return labels } diff --git a/internal/cli/metadata_label_test.go b/internal/cli/metadata_label_test.go index ebf3199..daa2070 100644 --- a/internal/cli/metadata_label_test.go +++ b/internal/cli/metadata_label_test.go @@ -133,6 +133,11 @@ func TestContainerIDLabels(t *testing.T) { if got := containerIDLabels(nil, map[string]string{}, "/ws", "/ws/c.json"); !reflect.DeepEqual(got, []string{"devcontainer.local_folder=/ws"}) { t.Errorf("older container = %v", got) } + // Without a workspace folder or a config file, TS's template literal + // writes undefined (--container-id alone). + if got := containerIDLabels(nil, withConfig, "", ""); !reflect.DeepEqual(got, []string{"devcontainer.local_folder=undefined", "devcontainer.config_file=undefined"}) { + t.Errorf("no workspace folder or config file = %v", got) + } } // The label's entries get the host substitutions, and ${devcontainerId} only diff --git a/internal/cli/ociauth_test.go b/internal/cli/ociauth_test.go index b24aa60..f8b49b1 100644 --- a/internal/cli/ociauth_test.go +++ b/internal/cli/ociauth_test.go @@ -17,6 +17,8 @@ import ( // --oci-auth-hardening, entries must be '=' pairs of // bare authorities, and the error message is the one users see (exit code 1). func TestOCIAuthGlobalFlagValidation(t *testing.T) { + // read-configuration looks for the container; this engine has none. + startFakeEngine(t, nil) tests := []struct { name string args []string @@ -125,6 +127,8 @@ func TestExecAcceptsGlobalOCIAuthFlags(t *testing.T) { // reference CLI emits ociAuthDiagnostics on every read-configuration, even when // no registry was contacted. func TestReadConfigurationEmitsOCIAuthDiagnostics(t *testing.T) { + // read-configuration looks for the container; this engine has none. + startFakeEngine(t, nil) ws := t.TempDir() if err := os.MkdirAll(filepath.Join(ws, ".devcontainer"), 0o755); err != nil { t.Fatalf("mkdir: %v", err) diff --git a/internal/cli/outdated.go b/internal/cli/outdated.go index 7470122..cf091f0 100644 --- a/internal/cli/outdated.go +++ b/internal/cli/outdated.go @@ -6,7 +6,10 @@ import ( "encoding/json" "errors" "fmt" + "io" + "io/fs" "os" + "path/filepath" "regexp" "slices" "sort" @@ -19,7 +22,9 @@ import ( "github.com/devcontainers/cli/internal/jsonc" "github.com/devcontainers/cli/internal/log" "github.com/devcontainers/cli/internal/oci" + "github.com/devcontainers/cli/internal/stringify" "github.com/spf13/cobra" + "golang.org/x/term" ) // bareVersion is a bare semver-ish target: x / x.y / x.y.z @@ -139,6 +144,26 @@ func resolvePublishedVersions(ociClient oci.Registry, ref *oci.Ref) []*semver.Ve return versions } +// outdatedHeader is the header row of outdated's text table. +var outdatedHeader = []string{"Feature", "Current", "Wanted", "Latest"} + +// writeOutdatedJSON writes outdated's JSON result as TS does: +// JSON.stringify(outdated, undefined, process.stdout.isTTY ? ' ' : undefined), +// indented only when stdout is a terminal. +func writeOutdatedJSON(w io.Writer, payload map[string]any) { + data, _ := stringify.JSON(payload, outdatedIndent(w)) + fmt.Fprintln(w, string(data)) +} + +// outdatedIndent is the indent of outdated's JSON on w: two spaces when w is +// a terminal (Node's isTTY, an isatty check), none otherwise. +func outdatedIndent(w io.Writer) string { + if f, ok := w.(*os.File); ok && term.IsTerminal(int(f.Fd())) { + return " " + } + return "" +} + // majorOf returns the major version of a semver string, or "" if unparseable. func majorOf(v string) string { if v == "" { @@ -178,9 +203,10 @@ func runOutdated(ctx context.Context, out Output, opts outdatedOpts) error { cfg := loadResult.Config if len(cfg.Features) == 0 { if opts.outputFormat == "json" { - fmt.Fprintln(out.Stdout(), `{"features":{}}`) + writeOutdatedJSON(out.Stdout(), map[string]any{"features": map[string]any{}}) } else { - fmt.Fprintln(out.Stdout(), "No features configured.") + // TS writes its table with no rows: the header alone. + fmt.Fprintln(out.Stdout(), textTable([][]string{outdatedHeader})) } return nil } @@ -234,16 +260,21 @@ func runOutdated(ctx context.Context, out Output, opts outdatedOpts) error { } if opts.outputFormat == "json" { - payload := map[string]any{"features": result} - data, _ := stringifyJSON(payload, " ") - fmt.Fprintln(out.Stdout(), string(data)) + // TS reorders resolved.features to the order the configuration lists + // the Features in (loadVersionInfo), which JSON.stringify keeps. + fields := make(map[string]any, len(result)) + for id, e := range result { + fields[id] = e + } + ordered := orderedObject{fields: fields, order: orderedFeatureKeys(cfg.ConfigFilePath, cfg.Features)} + writeOutdatedJSON(out.Stdout(), map[string]any{"features": ordered}) } else { // Text table. Rows follow the order features were declared in the config // (TS reorders resolved.features back to config order), and the Feature // column shows the full user id without its version tag/digest. Formatting // mirrors npm's text-table (min column widths, 2-space separator, left // aligned, trailing whitespace stripped per row). - rows := [][]string{{"Feature", "Current", "Wanted", "Latest"}} + rows := [][]string{outdatedHeader} for _, id := range orderedFeatureKeys(cfg.ConfigFilePath, cfg.Features) { e, ok := result[id] if !ok { @@ -390,6 +421,10 @@ func textTable(rows [][]string) string { return strings.Join(lines, "\n") } +// upgradeLogLevels are the choices TS featuresUpgradeOptions declares for +// --log-level, which, unlike the other commands', include error. +var upgradeLogLevels = []string{"error", "info", "debug", "trace"} + func newUpgradeCmd() *cobra.Command { var ( workspaceFolder string @@ -408,6 +443,10 @@ func newUpgradeCmd() *cobra.Command { Use: "upgrade", Short: "Upgrade lockfile", RunE: func(cmd *cobra.Command, args []string) error { + // --log-level is a yargs choice, validated before .check() runs. + if err := validateEnum("log-level", logLevel, upgradeLogLevels); err != nil { + return err + } // --feature/--target-version are the dependabot-oriented flags: they // must be supplied together, and --target-version must be a bare // x / x.y / x.y.z version. TS validates this in yargs .check() before @@ -436,33 +475,62 @@ func newUpgradeCmd() *cobra.Command { } defer closeLog() - logger := log.New(log.Options{ - Level: log.ParseLevel(logLevel), - Format: "text", - Writer: logDst, - }) - loadResult, err := config.LoadDevContainerConfig(ws, cp, "") + if errors.Is(err, fs.ErrNotExist) { + // A --config file that does not exist is TS getConfig's "not found". + return &coreerrors.ContainerError{Description: fmt.Sprintf("Dev container config (%s) not found.", cp)} + } if err != nil { return err } + // TS writes its log header with the first event, which every run + // that gets past loading the configuration logs (at trace level + // at least), and none that fails to load it. + logger := log.New(log.Options{ + Version: cliVersion(), + Level: log.ParseLevel(logLevel), + Format: "text", + Writer: logDst, + }) + cfg := loadResult.Config + // --feature/--target-version first pin the Feature's version in + // devcontainer.json, with or without --dry-run, and the lockfile + // is then generated from the configuration read again. + if feature != "" { + logger.Write(fmt.Sprintf("Updating '%s' to '%s' in devcontainer.json", feature, targetVersion), log.LevelInfo) + if err := updateFeatureVersionInConfig(logger, cfg, feature, targetVersion); err != nil { + return err + } + if loadResult, err = config.LoadDevContainerConfig(ws, cp, ""); err != nil { + return err + } + cfg = loadResult.Config + } + // The lockfile sits next to the configuration, whose path a loaded // configuration always carries. lockfilePath := cfg.ConfigFilePath - // Generate new lockfile from current features config - // This is a simplified version — the full implementation - // would resolve all features via OCI and compute digests. - ociClient := newOCIClient(cmd.Context(), logger) - - featureSets := resolveFeatureSets(cfg, ociClient, logger) - lf := features.GenerateLockfile(&features.Config{FeatureSets: featureSets}, nil) + // The Features are resolved as build resolves them, without the + // lockfile (TS readFeaturesConfig: generateFeaturesConfig with + // noLockfile): dependencies included, and a Feature that cannot be + // resolved fails the command. A configuration without Features has + // no Features config, which TS reports as a failure rather than + // writing an empty lockfile. + res, err := fetchFeatureSetsWithOrder(cmd.Context(), logger, cfg.Features, localFeatureRoots{filepath.Dir(lockfilePath), ws}, false, nil, nil) + if err != nil { + return err + } + if res == nil { + return &coreerrors.ContainerError{Description: "Failed to update lockfile"} + } + lf := features.GenerateLockfile(&features.Config{FeatureSets: res.FeatureSets}, nil) if dryRun { - data, _ := stringifyJSON(lf, " ") + data, _ := stringify.JSON(lf, " ") fmt.Fprintln(outputFor(cmd).Stdout(), string(data)) return nil } @@ -470,15 +538,10 @@ func newUpgradeCmd() *cobra.Command { // TS truncates any existing lockfile before writing it // (upgradeCommand.ts), so the new one is always written in its // canonical form, even over one that differs only cosmetically. - if err := os.WriteFile(features.LockfilePath(lockfilePath), nil, 0o644); err != nil { + if err := os.WriteFile(features.LockfilePath(lockfilePath), nil, 0o666); err != nil { return err } - if err := features.WriteLockfile(lockfilePath, lf, features.WriteOptions{}); err != nil { - return err - } - - logger.Write("Lockfile updated.", log.LevelInfo) - return nil + return features.WriteLockfile(lockfilePath, lf, features.WriteOptions{}) }, } @@ -500,61 +563,49 @@ func newUpgradeCmd() *cobra.Command { return cmd } -func resolveFeatureSets(cfg *config.DevContainer, ociClient oci.Registry, logger log.Logger) []*features.Set { +// updateFeatureVersionInConfig pins the configured Feature that is feature, +// whatever its version, to targetVersion in the configuration file, as TS +// updateFeatureVersionInConfig does: the first Feature key, in the order the +// file lists them, whose id without its version is feature's is replaced, +// read as a regular expression, everywhere in the file's text by +// ":". +func updateFeatureVersionInConfig(logger log.Logger, cfg *config.DevContainer, feature, targetVersion string) error { + configPath := cfg.ConfigFilePath if cfg.Features == nil { + logger.Write(fmt.Sprintf("No Features found in '%s'.", configPath), log.LevelDebug) return nil } + data, err := os.ReadFile(configPath) + if err != nil { + return err + } + previous := string(data) + updated := previous - var sets []*features.Set - for id, opts := range cfg.Features { - resolvedID, _ := features.ResolveID(id, false) - ref, err := oci.ParseRef(resolvedID) - if err != nil { + target := getFeatureIdWithoutVersion(feature) + for _, userFeatureID := range orderedFeatureKeys(configPath, cfg.Features) { + if getFeatureIdWithoutVersion(userFeatureID) != target { continue } - - manifest, err := ociClient.FetchManifest(ref, "") + re, err := regexp.Compile(userFeatureID) if err != nil { - continue - } - - // The lockfile records the concrete feature version, not the tag. Prefer the - // manifest's dev.containers.metadata annotation; if absent (some features - // don't publish it), resolve the tag to its newest published version — both - // match the version TS stores. - version := ref.Tag - if manifest.Manifest != nil { - if meta := manifest.Manifest.Annotations["dev.containers.metadata"]; meta != "" { - var m struct { - Version string `json:"version"` - } - if json.Unmarshal([]byte(meta), &m) == nil && m.Version != "" { - version = m.Version - } - } - } - if version == ref.Tag { - if v := highestSatisfyingTag(resolvePublishedVersions(ociClient, ref), ref.Tag); v != "" { - version = v - } + return err } + // TS String.replace reads '$' patterns in the replacement, but a + // replacement holding '$' comes from a key holding '$', which as a + // pattern only matches at the very end of the text, where a JSON + // document has its closing brace: it is replaced literally. + updated = re.ReplaceAllLiteralString(updated, target+":"+targetVersion) + break + } - set := &features.Set{ - SourceInfo: &features.OCISource{ - Registry: ref.Registry, - Namespace: ref.Namespace, - ID: ref.ID, - Resource: ref.Resource, - Tag: ref.Tag, - ManifestDigest: manifest.ContentDigest, - UserID: id, - }, - Features: []features.Feature{{ID: ref.ID, Version: version, Value: opts}}, - ComputedDigest: manifest.ContentDigest, - } - sets = append(sets, set) + logger.Write(updated, log.LevelTrace) + if updated == previous { + logger.Write(fmt.Sprintf("No changes to config file: %s\n", configPath), log.LevelTrace) + return nil } - return sets + logger.Write(fmt.Sprintf("Updating config file: '%s'", configPath), log.LevelInfo) + return os.WriteFile(configPath, []byte(updated), 0o644) } // errorStack is how the reference logs an error it catches (err.stack, frames diff --git a/internal/cli/outdated_held_test.go b/internal/cli/outdated_held_test.go index 6c12302..916a5fb 100644 --- a/internal/cli/outdated_held_test.go +++ b/internal/cli/outdated_held_test.go @@ -51,6 +51,10 @@ func TestOutdatedReportsVersions(t *testing.T) { if err != nil { t.Fatal(err) } + // stdout is no terminal: TS's JSON.stringify writes it on one line. + if strings.Count(got, "\n") != 1 || strings.Contains(got, " ") { + t.Errorf("json output = %q, want one unindented line", got) + } var doc struct { Features map[string]outdatedEntry `json:"features"` } @@ -62,6 +66,11 @@ func TestOutdatedReportsVersions(t *testing.T) { h + "/ns/single:3": {Current: "3.1.0", Wanted: "3.1.0", WantedMajor: "3", Latest: "3.1.0", LatestMajor: "3"}, h + "/ns/blank:1": {Current: "1.0.0", Wanted: "1.0.0", WantedMajor: "1", Latest: "1.0.0", LatestMajor: "1"}, } + // The Features follow the configuration's order, as TS loadVersionInfo + // reorders them, not the ids' sorted order. + if keys, err := jsonKeyOrder(got, "features"); err != nil || !slices.Equal(keys, []string{h + "/ns/multi:1", h + "/ns/single:3", h + "/ns/blank:1"}) { + t.Errorf("features order = %v, %v; want the configuration's", keys, err) + } if len(doc.Features) != len(want) { t.Errorf("features = %v, want %v", doc.Features, want) } @@ -81,14 +90,14 @@ func TestOutdatedReportsVersions(t *testing.T) { } } -// With no Features there is nothing to look up: an empty result in JSON, a -// sentence in text. +// With no Features there is nothing to look up: an empty result in JSON, and +// in text the table TS writes with no rows, its header alone. func TestOutdatedWithoutFeatures(t *testing.T) { ws := writeOutdatedWorkspace(t, `{}`, "") if got, err := runOutdatedFor(t, outdatedOpts{workspaceFolder: ws, outputFormat: "json"}); err != nil || got != "{\"features\":{}}\n" { t.Errorf("json: err = %v, stdout = %q", err, got) } - if got, err := runOutdatedFor(t, outdatedOpts{workspaceFolder: ws, outputFormat: "text"}); err != nil || got != "No features configured.\n" { + if got, err := runOutdatedFor(t, outdatedOpts{workspaceFolder: ws, outputFormat: "text"}); err != nil || got != "Feature Current Wanted Latest\n" { t.Errorf("text: err = %v, stdout = %q", err, got) } } diff --git a/internal/cli/outdated_repo_held_test.go b/internal/cli/outdated_repo_held_test.go index 02768f9..8b05ffc 100644 --- a/internal/cli/outdated_repo_held_test.go +++ b/internal/cli/outdated_repo_held_test.go @@ -9,9 +9,13 @@ import ( "io" "os" "path/filepath" + "reflect" + "strconv" "strings" + "syscall" "testing" + coreerrors "github.com/devcontainers/cli/internal/errors" "github.com/devcontainers/cli/internal/features" "github.com/spf13/cobra" ) @@ -33,44 +37,31 @@ func executeCmd(t *testing.T, cmd *cobra.Command, args ...string) (string, error func TestOutdatedCmdWorkspaceFolderDefaultsToCwd(t *testing.T) { ws := writeOutdatedWorkspace(t, `{}`, "") t.Chdir(t.TempDir()) - if got, err := executeCmd(t, newOutdatedCmd(), "--workspace-folder", ws); err != nil || got != "No features configured.\n" { + if got, err := executeCmd(t, newOutdatedCmd(), "--workspace-folder", ws); err != nil || got != "Feature Current Wanted Latest\n" { t.Errorf("--workspace-folder: err = %v, stdout = %q", err, got) } t.Chdir(ws) - if got, err := executeCmd(t, newOutdatedCmd()); err != nil || got != "No features configured.\n" { + if got, err := executeCmd(t, newOutdatedCmd()); err != nil || got != "Feature Current Wanted Latest\n" { t.Errorf("cwd: err = %v, stdout = %q", err, got) } } -// manifestWithAnnotations is a Feature manifest carrying the given annotations -// object (JSON), or none when it is empty. -func manifestWithAnnotations(annotations string) []byte { - m := `{"schemaVersion":2,"mediaType":"application/vnd.oci.image.manifest.v1+json",` + - `"config":{"mediaType":"application/vnd.devcontainers","digest":"sha256:44136fa355b3678a1146ad16f7e8649e94fb4fc21fe77e8310c060f61caaff8a","size":2},` + - `"layers":[{"mediaType":"application/vnd.devcontainers.layer.v1+tar","digest":"sha256:44136fa355b3678a1146ad16f7e8649e94fb4fc21fe77e8310c060f61caaff8a","size":2}]` - if annotations != "" { - m += `,"annotations":` + annotations - } - return []byte(m + `}`) -} - -// upgradeRegistry serves Features whose lockfile version comes from each of -// the places upgrade looks: the manifest's metadata, else the newest published -// version the tag wants, else the tag itself. -func upgradeRegistry(t *testing.T) (*readRegistry, string) { +// addFeature publishes repo as a Feature whose devcontainer-feature.json is +// meta (JSON): its manifest carries meta as its metadata annotation and its +// layer is the Feature's tarball. +func (r *readRegistry) addFeature(t *testing.T, repo, meta string) { t.Helper() - reg := newReadRegistry(t) - reg.manifests["ns/annotated"] = featureManifest("annotated") - reg.tags["ns/annotated"] = []string{"1.0.0", "1.5.0"} - reg.manifests["ns/plain"] = manifestWithAnnotations("") - reg.tags["ns/plain"] = []string{"1.0.0", "1.5.0", "2.0.0"} - reg.manifests["ns/noversion"] = manifestWithAnnotations(`{"dev.containers.metadata":"{\"id\":\"noversion\"}"}`) - reg.tags["ns/noversion"] = []string{"1.2.0"} - reg.manifests["ns/untagged"] = manifestWithAnnotations("") - h := reg.host - features := `{"` + h + `/ns/annotated:1":{}, "` + h + `/ns/plain:1":{}, "` + h + `/ns/noversion:1":{}, "` + - h + `/ns/untagged:1":{}, "` + h + `/ns/missing:1":{}, "./local":{}}` - return reg, features + tgz := featureTgzBytes(t, map[string]string{"devcontainer-feature.json": meta, "install.sh": "#!/bin/sh\n"}) + digest := sha256Digest(tgz) + r.blobs[digest] = tgz + annotations, err := json.Marshal(map[string]string{"dev.containers.metadata": meta}) + if err != nil { + t.Fatal(err) + } + r.manifests[repo] = []byte(`{"schemaVersion":2,"mediaType":"application/vnd.oci.image.manifest.v1+json",` + + `"config":{"mediaType":"application/vnd.devcontainers","digest":"sha256:44136fa355b3678a1146ad16f7e8649e94fb4fc21fe77e8310c060f61caaff8a","size":2},` + + `"layers":[{"mediaType":"application/vnd.devcontainers.layer.v1+tar","digest":"` + digest + `","size":` + strconv.Itoa(len(tgz)) + `}],` + + `"annotations":` + string(annotations) + `}`) } func parseLockfile(t *testing.T, data string) features.Lockfile { @@ -82,47 +73,66 @@ func parseLockfile(t *testing.T, data string) features.Lockfile { return lf } -// upgrade --dry-run prints the lockfile for every OCI Feature whose manifest -// resolves, pinned to its manifest digest, at the version TS records; local -// Features and ones without a manifest are left out. +// upgrade --dry-run resolves the Features as build does (TS readFeaturesConfig) +// and prints their lockfile: each OCI Feature at the version its +// devcontainer-feature.json records, pinned to its manifest digest, the +// Features it depends on included, local Features left out. func TestUpgradeDryRunPrintsLockfile(t *testing.T) { - reg, feats := upgradeRegistry(t) - ws := writeOutdatedWorkspace(t, feats, "") + reg := newReadRegistry(t) + h := reg.host + reg.addFeature(t, "ns/tool", `{"id":"tool","version":"1.2.3","dependsOn":{"`+h+`/ns/dep:2":{}}}`) + reg.addFeature(t, "ns/dep", `{"id":"dep","version":"2.0.1"}`) + ws := writeOutdatedWorkspace(t, `{"`+h+`/ns/tool:1":{}, "./local":{}}`, "") + writeFile(t, filepath.Join(ws, ".devcontainer", "local", "devcontainer-feature.json"), `{"id":"local","version":"0.1.0"}`, 0o644) got, err := executeCmd(t, newUpgradeCmd(), "--workspace-folder", ws, "--dry-run") if err != nil { t.Fatal(err) } - lf := parseLockfile(t, got) - h := reg.host - want := map[string]string{ - h + "/ns/annotated:1": "1.0.0", - h + "/ns/plain:1": "1.5.0", - h + "/ns/noversion:1": "1.2.0", - h + "/ns/untagged:1": "1", - } - if len(lf.Features) != len(want) { - t.Errorf("lockfile features = %v, want %v", lf.Features, want) - } - for id, version := range want { - if e := lf.Features[id]; e.Version != version { - t.Errorf("%s version = %q, want %q", id, e.Version, version) - } + digest := func(repo string) string { + sum := sha256.Sum256(reg.manifests[repo]) + return "sha256:" + hex.EncodeToString(sum[:]) + } + want := map[string]features.LockfileEntry{ + h + "/ns/tool:1": {Version: "1.2.3", Resolved: h + "/ns/tool@" + digest("ns/tool"), Integrity: digest("ns/tool"), DependsOn: []string{h + "/ns/dep:2"}}, + h + "/ns/dep:2": {Version: "2.0.1", Resolved: h + "/ns/dep@" + digest("ns/dep"), Integrity: digest("ns/dep")}, } - sum := sha256.Sum256(reg.manifests["ns/annotated"]) - digest := "sha256:" + hex.EncodeToString(sum[:]) - if e := lf.Features[h+"/ns/annotated:1"]; e.Integrity != digest || e.Resolved != h+"/ns/annotated@"+digest { - t.Errorf("annotated entry = %+v, want it pinned to %s", e, digest) + if lf := parseLockfile(t, got); !reflect.DeepEqual(lf.Features, want) { + t.Errorf("lockfile features = %+v, want %+v", lf.Features, want) } if _, err := os.Stat(filepath.Join(ws, ".devcontainer", "devcontainer-lock.json")); !os.IsNotExist(err) { t.Errorf("--dry-run wrote a lockfile: %v", err) } } +// A Feature that cannot be resolved fails upgrade, as it fails TS +// generateFeaturesConfig, rather than being left out of the lockfile: nothing +// is printed or written. +func TestUpgradeFailsOnAnUnresolvableFeature(t *testing.T) { + reg := newReadRegistry(t) + reg.addFeature(t, "ns/tool", `{"id":"tool","version":"1.0.0"}`) + missing := reg.host + "/ns/missing:1" + want := "ERR: Feature '" + missing + "' could not be processed." + for _, dryRun := range []bool{false, true} { + ws := writeOutdatedWorkspace(t, `{"`+reg.host+`/ns/tool:1":{}, "`+missing+`":{}}`, "") + args := []string{"--workspace-folder", ws} + if dryRun { + args = append(args, "--dry-run") + } + got, err := executeCmd(t, newUpgradeCmd(), args...) + if err == nil || !strings.HasPrefix(err.Error(), want) || got != "" { + t.Errorf("dry-run %t: err = %v, stdout = %q, want %q", dryRun, err, got, want) + } + if _, err := os.Stat(filepath.Join(ws, ".devcontainer", "devcontainer-lock.json")); !os.IsNotExist(err) { + t.Errorf("dry-run %t: wrote a lockfile: %v", dryRun, err) + } + } +} + // Without --dry-run the lockfile is written next to the configuration // --config names; one that cannot be written fails the command. func TestUpgradeWritesLockfileNextToConfig(t *testing.T) { reg := newReadRegistry(t) - reg.manifests["ns/tool"] = featureManifest("tool") + reg.addFeature(t, "ns/tool", `{"id":"tool","version":"1.0.0"}`) cfg := filepath.Join(t.TempDir(), "custom.json") writeFile(t, cfg, `{"image":"alpine","features":{"`+reg.host+`/ns/tool:1":{}}}`, 0o644) ws := t.TempDir() @@ -152,7 +162,7 @@ func TestUpgradeWritesLockfileNextToConfig(t *testing.T) { // where build and up would leave it alone. func TestUpgradeRewritesACosmeticallyDifferentLockfile(t *testing.T) { reg := newReadRegistry(t) - reg.manifests["ns/tool"] = featureManifest("tool") + reg.addFeature(t, "ns/tool", `{"id":"tool","version":"1.0.0"}`) cfg := filepath.Join(t.TempDir(), "devcontainer.json") writeFile(t, cfg, `{"image":"alpine","features":{"`+reg.host+`/ns/tool:1":{}}}`, 0o644) ws := t.TempDir() @@ -184,7 +194,7 @@ func TestUpgradeFailsOnALockfileItCannotReadBack(t *testing.T) { t.Skip("root reads a write-only file") } reg := newReadRegistry(t) - reg.manifests["ns/tool"] = featureManifest("tool") + reg.addFeature(t, "ns/tool", `{"id":"tool","version":"1.0.0"}`) cfg := filepath.Join(t.TempDir(), "devcontainer.json") writeFile(t, cfg, `{"image":"alpine","features":{"`+reg.host+`/ns/tool:1":{}}}`, 0o644) writeFile(t, features.LockfilePath(cfg), "{}", 0o200) @@ -196,13 +206,15 @@ func TestUpgradeFailsOnALockfileItCannotReadBack(t *testing.T) { // upgrade reads the workspace given by --workspace-folder, and the current // directory without it. func TestUpgradeWorkspaceFolderDefaultsToCwd(t *testing.T) { - ws := writeOutdatedWorkspace(t, `{}`, "") + reg := newReadRegistry(t) + reg.addFeature(t, "ns/tool", `{"id":"tool","version":"1.0.0"}`) + ws := writeOutdatedWorkspace(t, `{"`+reg.host+`/ns/tool:1":{}}`, "") t.Chdir(t.TempDir()) - if got, err := executeCmd(t, newUpgradeCmd(), "--workspace-folder", ws, "--dry-run"); err != nil || parseLockfile(t, got).Features == nil { + if got, err := executeCmd(t, newUpgradeCmd(), "--workspace-folder", ws, "--dry-run"); err != nil || len(parseLockfile(t, got).Features) != 1 { t.Errorf("--workspace-folder: err = %v, stdout = %q", err, got) } t.Chdir(ws) - if got, err := executeCmd(t, newUpgradeCmd(), "--dry-run"); err != nil || parseLockfile(t, got).Features == nil { + if got, err := executeCmd(t, newUpgradeCmd(), "--dry-run"); err != nil || len(parseLockfile(t, got).Features) != 1 { t.Errorf("cwd: err = %v, stdout = %q", err, got) } } @@ -229,8 +241,30 @@ func TestUpgradeRejectsBadInvocations(t *testing.T) { t.Errorf("%q: err = %v, want %q", c.args, err, c.want) } } - if _, err := executeCmd(t, newUpgradeCmd(), "--workspace-folder", ws, "--dry-run", "--feature", "ghcr.io/o/f", "--target-version", "2.1"); err != nil { - t.Errorf("--feature with a bare --target-version: %v", err) + if _, err := executeCmd(t, newUpgradeCmd(), "--workspace-folder", ws, "--dry-run", "--feature", "ghcr.io/o/f", "--target-version", "2.1"); err == nil || err.Error() != "Failed to update lockfile" { + t.Errorf("--feature with a bare --target-version: err = %v, want it past the flag checks", err) + } +} + +// A configuration without Features has no Features config to lock, which TS +// upgrade fails on ("Failed to update lockfile") rather than writing an empty +// lockfile, with or without --dry-run. +func TestUpgradeFailsWithoutFeatures(t *testing.T) { + for _, feats := range []string{`{}`, `null`} { + for _, dryRun := range []bool{false, true} { + ws := writeOutdatedWorkspace(t, feats, "") + args := []string{"--workspace-folder", ws} + if dryRun { + args = append(args, "--dry-run") + } + got, err := executeCmd(t, newUpgradeCmd(), args...) + if _, ok := coreerrors.AsContainerError(err); !ok || err.Error() != "Failed to update lockfile" || got != "" { + t.Errorf("features %s, dry-run %t: err = %v, stdout = %q", feats, dryRun, err, got) + } + if _, err := os.Stat(filepath.Join(ws, ".devcontainer", "devcontainer-lock.json")); !os.IsNotExist(err) { + t.Errorf("features %s, dry-run %t: wrote a lockfile: %v", feats, dryRun, err) + } + } } } @@ -261,3 +295,190 @@ func TestGetFeatureIdWithoutVersionRightAfterSlash(t *testing.T) { } } } + +// --log-level is one of TS upgrade's choices, error among them, and yargs +// rejects any other before .check() looks at --feature/--target-version. +func TestUpgradeLogLevelChoices(t *testing.T) { + ws := writeOutdatedWorkspace(t, `{}`, "") + want := `Invalid value "warn" for --log-level. Choose from: error, info, debug, trace` + if _, err := executeCmd(t, newUpgradeCmd(), "--workspace-folder", ws, "--log-level", "warn", "--feature", "ghcr.io/o/f"); err == nil || err.Error() != want { + t.Errorf("err = %v, want %q", err, want) + } + if _, err := executeCmd(t, newUpgradeCmd(), "--workspace-folder", ws, "--log-level", "error", "--feature", "ghcr.io/o/f"); err == nil || !strings.Contains(err.Error(), "must be used together") { + t.Errorf("--log-level error: err = %v, want the --feature/--target-version check", err) + } +} + +// A --config file that does not exist is TS getConfig's "not found" error, +// naming the resolved path. +func TestUpgradeMissingConfigIsNotFound(t *testing.T) { + missing := filepath.Join(t.TempDir(), "nope.json") + want := "Dev container config (" + missing + ") not found." + if _, err := executeCmd(t, newUpgradeCmd(), "--workspace-folder", t.TempDir(), "--config", missing); err == nil || err.Error() != want { + t.Errorf("err = %v, want %q", err, want) + } +} + +// upgrade logs TS's header once the configuration loads, none when it does +// not, and nothing of its own after writing the lockfile. +func TestUpgradeLogsAsTheReference(t *testing.T) { + reg := newReadRegistry(t) + reg.addFeature(t, "ns/tool", `{"id":"tool","version":"1.0.0"}`) + cfg := filepath.Join(t.TempDir(), "devcontainer.json") + writeFile(t, cfg, `{"image":"alpine","features":{"`+reg.host+`/ns/tool:1":{}}}`, 0o644) + logFile := filepath.Join(t.TempDir(), "upgrade.log") + if _, err := executeCmd(t, newUpgradeCmd(), "--workspace-folder", t.TempDir(), "--config", cfg, "--log-file", logFile); err != nil { + t.Fatal(err) + } + lines := strings.Split(strings.TrimSpace(readFile(t, logFile)), "\n") + if !strings.Contains(lines[0], "@devcontainers/cli ") || strings.Contains(readFile(t, logFile), "Lockfile updated.") { + t.Errorf("log = %q, want the header first and no \"Lockfile updated.\"", lines) + } + + missing := filepath.Join(t.TempDir(), "nope.json") + if _, err := executeCmd(t, newUpgradeCmd(), "--workspace-folder", t.TempDir(), "--config", missing, "--log-file", logFile); err == nil { + t.Fatal("a missing --config: want an error") + } + if got := readFile(t, logFile); got != "" { + t.Errorf("log of a configuration that does not load = %q, want none", got) + } +} + +// --feature/--target-version pin the Feature in devcontainer.json before the +// lockfile is generated, as TS updateFeatureVersionInConfig does, also with +// --dry-run: the first key, in file order, naming the Feature whatever its +// version is replaced, as a pattern, everywhere in the file's text. +func TestUpgradePinsTheFeatureVersionInConfig(t *testing.T) { + reg := newReadRegistry(t) + h := reg.host + reg.addFeature(t, "ns/tool", `{"id":"tool","version":"2.0.0"}`) + reg.addFeature(t, "ns/other", `{"id":"other","version":"1.0.0"}`) + ws := t.TempDir() + cfg := filepath.Join(ws, ".devcontainer", "devcontainer.json") + writeFile(t, cfg, "{\n\t// pinned: "+h+"/ns/tool:1\n\t\"image\": \"alpine\",\n\t\"features\": {\n\t\t\""+h+"/ns/other:1\": {},\n\t\t\""+h+"/ns/tool:1\": {}\n\t}\n}\n", 0o644) + logFile := filepath.Join(t.TempDir(), "upgrade.log") + got, err := executeCmd(t, newUpgradeCmd(), "--workspace-folder", ws, "--dry-run", "--feature", h+"/ns/tool:1.5", "--target-version", "2", "--log-file", logFile) + if err != nil { + t.Fatal(err) + } + wantCfg := "{\n\t// pinned: " + h + "/ns/tool:2\n\t\"image\": \"alpine\",\n\t\"features\": {\n\t\t\"" + h + "/ns/other:1\": {},\n\t\t\"" + h + "/ns/tool:2\": {}\n\t}\n}\n" + if got := readFile(t, cfg); got != wantCfg { + t.Errorf("config = %q, want %q", got, wantCfg) + } + if e, ok := parseLockfile(t, got).Features[h+"/ns/tool:2"]; !ok || e.Version != "2.0.0" { + t.Errorf("lockfile = %s, want it generated from the pinned configuration", got) + } + log := readFile(t, logFile) + for _, want := range []string{ + "Updating '" + h + "/ns/tool:1.5' to '2' in devcontainer.json", + "Updating config file: '" + cfg + "'", + } { + if !strings.Contains(log, want) { + t.Errorf("log = %q, want it to hold %q", log, want) + } + } + if strings.Contains(log, "\"image\": \"alpine\"") { + t.Errorf("log = %q, want the updated text only at trace level", log) + } +} + +// Of two keys naming the Feature only the first in file order is replaced; +// the key is read as a regular expression, so a '.' in it matches any +// character, and a key that is not a valid one fails the command. +func TestUpgradeFeatureKeyIsAPattern(t *testing.T) { + reg := newReadRegistry(t) + h := reg.host + reg.addFeature(t, "ns/tool", `{"id":"tool","version":"2.0.0"}`) + ws := t.TempDir() + cfg := filepath.Join(ws, ".devcontainer", "devcontainer.json") + writeFile(t, cfg, `{"image":"alpine","x":"`+h+`/ns/tool:1X0","features":{"`+h+`/ns/tool:1.0":{},"`+h+`/ns/tool:3":{}}}`, 0o644) + if _, err := executeCmd(t, newUpgradeCmd(), "--workspace-folder", ws, "--dry-run", "--feature", h+"/ns/tool", "--target-version", "2"); err != nil { + t.Fatal(err) + } + if got, want := readFile(t, cfg), `{"image":"alpine","x":"`+h+`/ns/tool:2","features":{"`+h+`/ns/tool:2":{},"`+h+`/ns/tool:3":{}}}`; got != want { + t.Errorf("config = %q, want %q", got, want) + } + + bad := `{"image":"alpine","features":{"` + h + `/ns/to(ol:1":{}}}` + writeFile(t, cfg, bad, 0o644) + if _, err := executeCmd(t, newUpgradeCmd(), "--workspace-folder", ws, "--dry-run", "--feature", h+"/ns/to(ol", "--target-version", "2"); err == nil || !strings.Contains(err.Error(), "missing closing )") { + t.Errorf("an invalid pattern: err = %v", err) + } + if got := readFile(t, cfg); got != bad { + t.Errorf("config = %q, want it untouched", got) + } +} + +// A configuration with no Feature to pin is left as it is: without Features +// TS logs so at debug level, and with Features none of which is the one +// asked for, the unchanged text at trace level. +func TestUpgradeLeavesAConfigWithoutTheFeature(t *testing.T) { + reg := newReadRegistry(t) + h := reg.host + reg.addFeature(t, "ns/tool", `{"id":"tool","version":"1.0.0"}`) + cases := []struct { + features, level, wantLog string + wantErr bool + }{ + {`null`, "debug", "No Features found in '", true}, + {`{"` + h + `/ns/tool:1":{}}`, "trace", "No changes to config file: ", false}, + } + for _, c := range cases { + ws := writeOutdatedWorkspace(t, c.features, "") + cfg := filepath.Join(ws, ".devcontainer", "devcontainer.json") + before := readFile(t, cfg) + logFile := filepath.Join(t.TempDir(), "upgrade.log") + _, err := executeCmd(t, newUpgradeCmd(), "--workspace-folder", ws, "--dry-run", "--feature", h+"/ns/other", "--target-version", "2", "--log-level", c.level, "--log-file", logFile) + if (err != nil) != c.wantErr { + t.Errorf("features %s: err = %v, want error %t", c.features, err, c.wantErr) + } + if got := readFile(t, cfg); got != before { + t.Errorf("features %s: config = %q, want it untouched", c.features, got) + } + if log := readFile(t, logFile); !strings.Contains(log, c.wantLog+cfg) || strings.Contains(log, "Updating config file") { + t.Errorf("features %s: log = %q, want %q", c.features, log, c.wantLog+cfg) + } + } +} + +// A local Feature outside the .devcontainer folder fails upgrade as TS +// processFeatureIdentifier fails it, with or without --dry-run, rather than +// being written to the lockfile. +func TestUpgradeRefusesALocalFeatureOutsideDevcontainer(t *testing.T) { + for _, dryRun := range []bool{false, true} { + ws := writeOutdatedWorkspace(t, `{"../f":{}}`, "") + writeFile(t, filepath.Join(ws, "f", "devcontainer-feature.json"), `{"id":"f","version":"1.0.0"}`, 0o644) + args := []string{"--workspace-folder", ws} + if dryRun { + args = append(args, "--dry-run") + } + got, err := executeCmd(t, newUpgradeCmd(), args...) + if err == nil || !strings.HasPrefix(err.Error(), "ERR: Feature '../f' could not be processed.") || got != "" { + t.Errorf("dry-run %t: err = %v, stdout = %q", dryRun, err, got) + } + if _, err := os.Stat(filepath.Join(ws, ".devcontainer", "devcontainer-lock.json")); !os.IsNotExist(err) { + t.Errorf("dry-run %t: wrote a lockfile: %v", dryRun, err) + } + } +} + +// A lockfile upgrade creates gets Node's default mode, 0666 minus the umask, +// as TS writeLocalFile(lockfilePath, "") gives it before the lockfile is +// written. +func TestUpgradeCreatesTheLockfileWithNodesMode(t *testing.T) { + old := syscall.Umask(0o002) + t.Cleanup(func() { syscall.Umask(old) }) + reg := newReadRegistry(t) + reg.addFeature(t, "ns/tool", `{"id":"tool","version":"1.0.0"}`) + ws := writeOutdatedWorkspace(t, `{"`+reg.host+`/ns/tool:1":{}}`, "") + if _, err := executeCmd(t, newUpgradeCmd(), "--workspace-folder", ws); err != nil { + t.Fatal(err) + } + info, err := os.Stat(filepath.Join(ws, ".devcontainer", "devcontainer-lock.json")) + if err != nil { + t.Fatal(err) + } + if got := info.Mode().Perm(); got != 0o664 { + t.Errorf("lockfile mode = %v, want 0664", got) + } +} diff --git a/internal/cli/outdated_tty_linux_test.go b/internal/cli/outdated_tty_linux_test.go new file mode 100644 index 0000000..5c86122 --- /dev/null +++ b/internal/cli/outdated_tty_linux_test.go @@ -0,0 +1,51 @@ +//go:build linux + +package cli + +import ( + "io" + "os" + "path/filepath" + "testing" +) + +type terminalStdout struct{ terminal *os.File } + +func (o terminalStdout) Stdout() io.Writer { return o.terminal } +func (terminalStdout) Stderr() io.Writer { return io.Discard } + +// outdated indents its JSON only on a terminal, as TS's +// JSON.stringify(outdated, undefined, process.stdout.isTTY ? ' ' : undefined); +// a character device that is no terminal (/dev/null) or a regular file gets +// none. +func TestOutdatedIndentsOnlyOnATerminal(t *testing.T) { + master, terminal := openPTY(t) + ws := writeOutdatedWorkspace(t, `{}`, "") + if err := runOutdated(t.Context(), terminalStdout{terminal}, outdatedOpts{workspaceFolder: ws, outputFormat: "json", logLevel: "info"}); err != nil { + t.Fatal(err) + } + want := "{\r\n \"features\": {}\r\n}\r\n" // the terminal turns \n into \r\n + // With the terminal side closed, the master reads what was written and + // then fails (EIO) instead of waiting for more. + terminal.Close() + got, _ := io.ReadAll(master) + if string(got) != want { + t.Errorf("terminal output = %q, want %q", got, want) + } + + devNull, err := os.OpenFile(os.DevNull, os.O_WRONLY, 0) + if err != nil { + t.Fatal(err) + } + defer devNull.Close() + regular, err := os.Create(filepath.Join(t.TempDir(), "out")) + if err != nil { + t.Fatal(err) + } + defer regular.Close() + for _, w := range []*os.File{devNull, regular} { + if got := outdatedIndent(w); got != "" { + t.Errorf("%s: indent = %q, want none", w.Name(), got) + } + } +} diff --git a/internal/cli/parity_key_order_test.go b/internal/cli/parity_key_order_test.go new file mode 100644 index 0000000..1910d29 --- /dev/null +++ b/internal/cli/parity_key_order_test.go @@ -0,0 +1,20 @@ +package cli + +import ( + "slices" + "testing" +) + +// A key_order path reaches into arrays by index, so a case can pin the key +// order of an element (featuresConfiguration.featureSets.0.features.0). +func TestJSONKeyOrderIndexesArrays(t *testing.T) { + doc := `{"a":[{"x":1},{"z":1,"y":2}]}` + if keys, err := jsonKeyOrder(doc, "a.1"); err != nil || !slices.Equal(keys, []string{"z", "y"}) { + t.Errorf("a.1: keys = %v, %v; want [z y]", keys, err) + } + for _, path := range []string{"a.2", "a.-1", "a.0.x.0"} { + if _, err := jsonKeyOrder(doc, path); err == nil { + t.Errorf("%s: no error, want one", path) + } + } +} diff --git a/internal/cli/parity_matrix_test.go b/internal/cli/parity_matrix_test.go index 07b65c3..1e7e82e 100644 --- a/internal/cli/parity_matrix_test.go +++ b/internal/cli/parity_matrix_test.go @@ -1055,11 +1055,22 @@ func normalizeRequired(raw string) string { // jsonKeyOrder is the keys of the JSON object at path in the document data, in // the order data lists them: "" is the document itself, "a.b" the object at -// property b of property a. +// property b of property a, and "a.0" the first element of the array a. func jsonKeyOrder(data, path string) ([]string, error) { raw := json.RawMessage(data) if path != "" { for name := range strings.SplitSeq(path, ".") { + if i, err := strconv.Atoi(name); err == nil { + var arr []json.RawMessage + if err := json.Unmarshal(raw, &arr); err != nil { + return nil, fmt.Errorf("%s: %w", path, err) + } + if i < 0 || i >= len(arr) { + return nil, fmt.Errorf("%s: no element %d", path, i) + } + raw = arr[i] + continue + } var obj map[string]json.RawMessage if err := json.Unmarshal(raw, &obj); err != nil { return nil, fmt.Errorf("%s: %w", path, err) diff --git a/internal/cli/read_configuration.go b/internal/cli/read_configuration.go index 5f4eca5..6f01f5a 100644 --- a/internal/cli/read_configuration.go +++ b/internal/cli/read_configuration.go @@ -1,6 +1,7 @@ package cli import ( + "cmp" "context" "encoding/json" "errors" @@ -13,6 +14,7 @@ import ( coreerrors "github.com/devcontainers/cli/internal/errors" "github.com/devcontainers/cli/internal/imagemeta" "github.com/devcontainers/cli/internal/log" + "github.com/devcontainers/cli/internal/stringify" "github.com/moby/moby/api/types/container" "github.com/spf13/cobra" ) @@ -121,7 +123,7 @@ func runReadConfiguration(ctx context.Context, out Output, opts *readConfigOpts) workspaceFolder, configPath, overridePath := readConfigurationPaths(opts, cwd) var result *config.LoadResult - if workspaceFolder != "" || configPath != "" { + if workspaceFolder != "" || configPath != "" || overridePath != "" { var loadErr error if result, loadErr = loadReadConfiguration(workspaceFolder, configPath, overridePath, opts); loadErr != nil { return loadErr @@ -134,21 +136,38 @@ func runReadConfiguration(ctx context.Context, out Output, opts *readConfigOpts) lgr := log.New(log.Options{Version: cliVersion(), Level: log.ParseLevel(opts.logLevel), Format: opts.logFormat, Writer: logDst}) engine, engineErr := docker.NewEngineClient(lgr) if engineErr != nil { - // Non-fatal: container lookup features won't work but config can still be read. + // Logged here; a container lookup then fails the command (readConfigurationContainerID). lgr.Write(fmt.Sprintf("Warning: Docker engine: %v", engineErr), log.LevelWarning) } if engine != nil { defer engine.Close() } - containerID := readConfigurationContainerID(ctx, engine, opts, workspaceFolder, result) + var lister containerLister = failingLister{engineErr} + if engine != nil { + lister = engine + } + containerID, lookupErr := readConfigurationContainerID(ctx, lister, opts, workspaceFolder, result) + if lookupErr != nil { + return readConfigurationFailure(lgr, lookupErr) + } containerCfg, inspectErr := inspectContainerConfig(ctx, engine, containerID) if inspectErr != nil { return readConfigurationFailure(lgr, inspectErr) } - cfgMap, subErr := substituteContainerEnv(containerCfg, cfgMap) + cfgMap, idErr := substituteDevContainerID(containerCfg, cfgMap, opts.idLabels, workspaceFolder, result) + if idErr != nil { + return writeErrorResult(out, fmt.Sprintf("resolve configuration container variables: %v", idErr)) + } + configFile := "" + if result != nil { + configFile = result.Config.ConfigFilePath + } + // TS readConfiguration fails like any other error: logged, exit 1, nothing + // on stdout. + cfgMap, subErr := substituteContainerEnv(containerCfg, cfgMap, configFile) if subErr != nil { - return writeErrorResult(out, fmt.Sprintf("resolve configuration container variables: %v", subErr)) + return readConfigurationFailure(lgr, subErr) } output := map[string]any{ @@ -164,16 +183,24 @@ func runReadConfiguration(ctx context.Context, out Output, opts *readConfigOpts) output["cacheKey"] = key } - if ws, ok := workspaceOutput(result, workspaceFolder, opts.containerID); ok { + if ws, ok := workspaceOutput(result); ok { output["workspace"] = orderedObject{ws, workspaceKeys} } // Include features configuration if requested (or needed for merged config without container) var featResult *fetchFeatureResult + if featuresWanted(opts, containerID) { + // TS generateFeaturesConfig (containerFeaturesConfiguration.ts) traces + // the folder it resolves the Features from, the command's cwd. + lgr.Write("workspace root: "+cmp.Or(workspaceFolder, cwd), log.LevelTrace) + if result == nil || len(result.Config.Features) == 0 { + lgr.Write("No user features to update", log.LevelTrace) + } + } if needsFeaturesConfiguration(opts, containerID, result) { lgr := log.New(log.Options{Level: log.ParseLevel(opts.logLevel), Format: opts.logFormat, Writer: logDst}) var featErr error - featResult, featErr = fetchFeatureSets(ctx, lgr, result.Config.Features, filepath.Dir(result.Config.ConfigFilePath), opts.skipFeatureAutoMapping) + featResult, featErr = fetchFeatureSets(ctx, lgr, result.Config.Features, localFeatureRoots{filepath.Dir(result.Config.ConfigFilePath), workspaceFolder}, opts.skipFeatureAutoMapping) if featErr != nil { // TS readConfiguration writes the error to the log and exits 1 with no // result; a Feature that cannot be resolved is not an empty one. @@ -216,9 +243,9 @@ func runReadConfiguration(ctx context.Context, out Output, opts *readConfigOpts) if entriesErr != nil { return readConfigurationFailure(lgr, entriesErr) } - merged, mergeErr := mergedConfigurationOutput(entries, containerCfg, cfgMap) + merged, mergeErr := mergedConfigurationOutput(entries, containerCfg, cfgMap, configFile) if mergeErr != nil { - return mergeErr + return readConfigurationFailure(lgr, mergeErr) } output["mergedConfiguration"] = merged } @@ -228,7 +255,7 @@ func runReadConfiguration(ctx context.Context, out Output, opts *readConfigOpts) output["ociAuthDiagnostics"] = ociAuthDiagnostics(ctx) orderConfigurations(output, result) - data, err := stringifyJSON(orderedObject{output, readConfigurationKeys}, "") + data, err := stringify.JSON(orderedObject{output, readConfigurationKeys}, "") if err != nil { return fmt.Errorf("marshal output: %w", err) } @@ -256,12 +283,19 @@ func readConfigurationPaths(opts *readConfigOpts, cwd string) (workspaceFolder, return p } workspaceFolder = opts.workspaceFolder - if workspaceFolder == "" && opts.containerID == "" && len(opts.idLabels) == 0 { + if readConfigurationHasWorkspace(opts) && workspaceFolder == "" { workspaceFolder = cwd } return abs(workspaceFolder), abs(opts.configPath), abs(opts.overrideConfig) } +// readConfigurationHasWorkspace reports whether read-configuration has a +// workspace folder: --workspace-folder, else cwd unless --container-id or +// --id-label is given (TS readConfigurationOptions' check). +func readConfigurationHasWorkspace(opts *readConfigOpts) bool { + return opts.workspaceFolder != "" || opts.containerID == "" && len(opts.idLabels) == 0 +} + // loadReadConfiguration loads the configuration read-configuration prints, // with the --additional-features merged in. func loadReadConfiguration(workspaceFolder, configPath, overridePath string, opts *readConfigOpts) (*config.LoadResult, error) { @@ -312,12 +346,14 @@ func configurationOutput(result *config.LoadResult, opts *readConfigOpts) map[st } cfgMap[k] = v } - // TS makes a file URI of --config; a configuration found in the workspace, - // or its default path when only --override-config is given, has the + // TS makes a file URI of --config, and without a workspace of + // --override-config, which is then the configuration (configPath = + // overrideConfigFile); a configuration found in the workspace, or its + // default path when only --override-config is given, has the // vscode-fileHost scheme, with or without --container-id // (devContainersSpecCLI.ts readConfiguration). scheme := "vscode-fileHost" - if opts.configPath != "" { + if opts.configPath != "" || opts.overrideConfig != "" && !readConfigurationHasWorkspace(opts) { scheme = "file" } // TS findContainerAndIdLabels reads the URI's fsPath (devContainersSpecCLI.ts @@ -344,10 +380,12 @@ func configFileURI(path, scheme string, withFSPath bool) map[string]any { // readConfigurationContainerID is the container read-configuration reads: the // --container-id, else the one the id labels or the workspace folder and -// config file find. Without a Docker engine, only the --container-id. -func readConfigurationContainerID(ctx context.Context, engine *docker.EngineClient, opts *readConfigOpts, workspaceFolder string, result *config.LoadResult) string { - if opts.containerID != "" || engine == nil { - return opts.containerID +// config file find. A lookup that fails, also for want of a Docker engine +// (lister is then one that fails with engineErr), fails the command, as TS's +// findContainerAndIdLabels throws. +func readConfigurationContainerID(ctx context.Context, lister containerLister, opts *readConfigOpts, workspaceFolder string, result *config.LoadResult) (string, error) { + if opts.containerID != "" { + return opts.containerID, nil } configFile := "" if result != nil { @@ -355,7 +393,7 @@ func readConfigurationContainerID(ctx context.Context, engine *docker.EngineClie } // [local_folder, config_file] so multiple configs in one project resolve // to the right container (fallback to local_folder for legacy ones). - return resolveContainerID(ctx, engine, workspaceFolder, configFile, opts.idLabels) + return resolveContainerID(ctx, lister, workspaceFolder, configFile, opts.idLabels) } // inspectContainerConfig is the config of container cid, nil when there is no @@ -375,13 +413,37 @@ func inspectContainerConfig(ctx context.Context, engine *docker.EngineClient, ci return inspect.Config, nil } +// substituteDevContainerID resolves ${devcontainerId} in cfgMap from the id +// labels of the container read-configuration found, as TS readConfiguration +// adds beforeContainerSubstitute(idLabels) before the container's environment; +// without a container cfgMap is returned as it is. +func substituteDevContainerID(containerCfg *container.Config, cfgMap map[string]any, providedIDLabels []string, workspaceFolder string, result *config.LoadResult) (map[string]any, error) { + if containerCfg == nil { + return cfgMap, nil + } + configFile := "" + if result != nil { + configFile = result.Config.ConfigFilePath + } + idLabels := containerIDLabels(providedIDLabels, containerCfg.Labels, workspaceFolder, configFile) + substituted, err := config.NewVariableResolver().Resolve(config.SubstitutionContext{ + DevContainerID: config.ComputeDevContainerID(config.EnvMap(idLabels)), + }, config.PhaseIdentity, cfgMap) + if err != nil { + return nil, err + } + return substituted.(map[string]any), nil +} + // substituteContainerEnv resolves ${containerEnv:...} in cfgMap with the // container's environment; without a container cfgMap is returned as it is. -func substituteContainerEnv(containerCfg *container.Config, cfgMap map[string]any) (map[string]any, error) { +// configFile is the configuration's file, which an unresolvable variable's +// error names. +func substituteContainerEnv(containerCfg *container.Config, cfgMap map[string]any, configFile string) (map[string]any, error) { if containerCfg == nil { return cfgMap, nil } - substituted, err := resolveContainerVariables(config.EnvMap(containerCfg.Env), cfgMap) + substituted, err := resolveContainerVariables(config.EnvMap(containerCfg.Env), configFile, cfgMap) if err != nil { return nil, err } @@ -392,19 +454,23 @@ func substituteContainerEnv(containerCfg *container.Config, cfgMap map[string]an } // workspaceOutput is read-configuration's "workspace"; ok is false when the -// configuration gives none. With --container-id and no workspace folder it is -// empty, as in TS. -func workspaceOutput(result *config.LoadResult, workspaceFolder, containerIDFlag string) (ws map[string]any, ok bool) { +// configuration gives none. Without a workspace folder it has only the +// configuration's own workspaceFolder and workspaceMount, as TS +// getWorkspaceConfiguration leaves them for an undefined workspace. +func workspaceOutput(result *config.LoadResult) (ws map[string]any, ok bool) { if result == nil || result.WorkspaceConfig == nil { return nil, false } - if workspaceFolder == "" && containerIDFlag != "" { - return map[string]any{}, true + ws = map[string]any{} + if result.WorkspaceConfig.WorkspaceFolder != "" { + ws["workspaceFolder"] = result.WorkspaceConfig.WorkspaceFolder } - ws = map[string]any{ - "workspaceFolder": result.WorkspaceConfig.WorkspaceFolder, - } - if result.WorkspaceConfig.WorkspaceMount != "" { + // A workspaceMount the configuration sets is written as it is set, even + // empty or null (TS readDevContainerConfigFile's `'workspaceMount' in + // config`); otherwise the computed one, if any. + if v, set := result.Raw["workspaceMount"]; set { + ws["workspaceMount"] = v + } else if result.WorkspaceConfig.WorkspaceMount != "" { ws["workspaceMount"] = result.WorkspaceConfig.WorkspaceMount } // The loader's additional mounts are at most the Git worktree common dir's, @@ -419,8 +485,13 @@ func workspaceOutput(result *config.LoadResult, workspaceFolder, containerIDFlag // config's Features: for featuresConfiguration, or for a merged configuration // without a container, which takes the Features' metadata (TS readConfiguration). func needsFeaturesConfiguration(opts *readConfigOpts, containerID string, result *config.LoadResult) bool { - wanted := opts.includeFeaturesCfg || (opts.includeMergedCfg && containerID == "") - return wanted && result != nil && len(result.Config.Features) > 0 + return featuresWanted(opts, containerID) && result != nil && len(result.Config.Features) > 0 +} + +// featuresWanted reports whether TS readConfiguration reads the Features +// configuration, which it does even for a configuration with no Features. +func featuresWanted(opts *readConfigOpts, containerID string) bool { + return opts.includeFeaturesCfg || (opts.includeMergedCfg && containerID == "") } // readConfigurationMetadata is the metadata read-configuration merges. With a @@ -451,14 +522,14 @@ func readConfigurationMetadata(base, featureEntries []imagemeta.Entry, container // mergedConfiguration by spreading the config, so the properties // mergeConfiguration does not own (image, features, build, configFilePath, ...) // come from cfgMap. -func mergedConfigurationOutput(entries []imagemeta.Entry, containerCfg *container.Config, cfgMap map[string]any) (map[string]any, error) { +func mergedConfigurationOutput(entries []imagemeta.Entry, containerCfg *container.Config, cfgMap map[string]any, configFile string) (map[string]any, error) { merged := imagemeta.MergeConfiguration(entries) if containerCfg != nil { mergedJSON, _ := json.Marshal(merged) var mergedGeneric any json.Unmarshal(mergedJSON, &mergedGeneric) - substituted, err := resolveContainerVariables(config.EnvMap(containerCfg.Env), mergedGeneric) + substituted, err := resolveContainerVariables(config.EnvMap(containerCfg.Env), configFile, mergedGeneric) if err != nil { return nil, err } diff --git a/internal/cli/read_configuration_held_test.go b/internal/cli/read_configuration_held_test.go index 73653ea..5cd6a9c 100644 --- a/internal/cli/read_configuration_held_test.go +++ b/internal/cli/read_configuration_held_test.go @@ -6,6 +6,7 @@ import ( "encoding/json" "errors" "io" + "maps" "os" "path/filepath" "reflect" @@ -20,6 +21,7 @@ import ( coreerrors "github.com/devcontainers/cli/internal/errors" "github.com/devcontainers/cli/internal/imagemeta" "github.com/devcontainers/cli/internal/log" + "github.com/devcontainers/cli/internal/stringify" ) // loadFixtureConfig writes a workspace with .devcontainer/devcontainer.json @@ -135,6 +137,11 @@ func TestConfigurationOutput(t *testing.T) { "--config": {readConfigOpts{configPath: cfp}, "file"}, "--override-config": {readConfigOpts{overrideConfig: cfp}, "vscode-fileHost"}, "--container-id": {readConfigOpts{containerID: "abc"}, "vscode-fileHost"}, + // Without a workspace the override is the configuration, a file URI + // (configPath = overrideConfigFile). + "--override-config, --container-id": {readConfigOpts{overrideConfig: cfp, containerID: "abc"}, "file"}, + "--override-config, --id-label": {readConfigOpts{overrideConfig: cfp, idLabels: []string{"a=b"}}, "file"}, + "--override-config, --container-id, --workspace-folder": {readConfigOpts{overrideConfig: cfp, containerID: "abc", workspaceFolder: "/p"}, "vscode-fileHost"}, } { got := configurationOutput(result, &tc.opts) if scheme := got["configFilePath"].(map[string]any)["scheme"]; scheme != tc.scheme { @@ -159,28 +166,34 @@ func TestReadConfigurationContainerID(t *testing.T) { }} engine := &docker.EngineClient{API: api, Log: log.Null} - if got := readConfigurationContainerID(ctx, engine, &readConfigOpts{}, ws, result); got != "by-config" { + if got, err := readConfigurationContainerID(ctx, engine, &readConfigOpts{}, ws, result); err != nil || got != "by-config" { t.Errorf("by workspace and config file = %q, want by-config", got) } - if got := readConfigurationContainerID(ctx, engine, &readConfigOpts{idLabels: []string{"a=b"}}, "", nil); got != "by-label" { + if got, err := readConfigurationContainerID(ctx, engine, &readConfigOpts{idLabels: []string{"a=b"}}, "", nil); err != nil || got != "by-label" { t.Errorf("by id label = %q, want by-label", got) } api.listed = nil - if got := readConfigurationContainerID(ctx, engine, &readConfigOpts{}, ws, nil); got != "" { + if got, err := readConfigurationContainerID(ctx, engine, &readConfigOpts{}, ws, nil); err != nil || got != "" { t.Errorf("without a config = %q, want none", got) } if len(api.listed) != 1 || slices.ContainsFunc(api.listed[0], func(l string) bool { return strings.HasPrefix(l, "devcontainer.config_file=") }) { t.Errorf("without a config, listed by %v; want the local folder alone", api.listed) } api.listed = nil - if got := readConfigurationContainerID(ctx, engine, &readConfigOpts{containerID: "given"}, ws, result); got != "given" || len(api.listed) != 0 { + if got, err := readConfigurationContainerID(ctx, engine, &readConfigOpts{containerID: "given"}, ws, result); err != nil || got != "given" || len(api.listed) != 0 { t.Errorf("--container-id = %q (listed %v), want it as given without a lookup", got, api.listed) } - if got := readConfigurationContainerID(ctx, nil, &readConfigOpts{}, ws, result); got != "" { - t.Errorf("without an engine = %q, want none", got) + // A lookup that fails fails the command, as TS's findContainerAndIdLabels + // throws: without an engine too, though --container-id needs no lookup. + noEngine := failingLister{errors.New("no engine")} + if got, err := readConfigurationContainerID(ctx, noEngine, &readConfigOpts{}, ws, result); got != "" || err == nil || !strings.HasPrefix(err.Error(), "Command failed: docker ps -q -a --filter label=devcontainer.local_folder=/some/workspace") { + t.Errorf("without an engine = %q, %v; want the failed lookup", got, err) } - if got := readConfigurationContainerID(ctx, nil, &readConfigOpts{containerID: "given"}, ws, result); got != "given" { - t.Errorf("--container-id without an engine = %q, want given", got) + if got, err := readConfigurationContainerID(ctx, noEngine, &readConfigOpts{idLabels: []string{"a=b"}}, "", nil); got != "" || err == nil || err.Error() != "Command failed: docker ps -q -a --filter label=a=b" { + t.Errorf("by id label without an engine = %q, %v; want the failed lookup", got, err) + } + if got, err := readConfigurationContainerID(ctx, noEngine, &readConfigOpts{containerID: "given"}, ws, result); err != nil || got != "given" { + t.Errorf("--container-id without an engine = %q, %v; want given", got, err) } } @@ -211,38 +224,78 @@ func TestInspectContainerConfig(t *testing.T) { func TestSubstituteContainerEnv(t *testing.T) { cfgMap := map[string]any{"remoteEnv": map[string]any{"P": "${containerEnv:PATH}"}} - got, err := substituteContainerEnv(nil, cfgMap) + got, err := substituteContainerEnv(nil, cfgMap, "") if err != nil || !reflect.DeepEqual(got, cfgMap) { t.Fatalf("without a container = %v, %v; want the config unchanged", got, err) } - got, err = substituteContainerEnv(&container.Config{Env: []string{"PATH=/bin"}}, cfgMap) + got, err = substituteContainerEnv(&container.Config{Env: []string{"PATH=/bin"}}, cfgMap, "") if err != nil || !reflect.DeepEqual(got, map[string]any{"remoteEnv": map[string]any{"P": "/bin"}}) { t.Fatalf("with a container = %v, %v; want ${containerEnv:PATH} resolved", got, err) } + // A reference without a name is TS's ContainerError naming the file. + _, err = substituteContainerEnv(&container.Config{}, map[string]any{"x": "${containerEnv}"}, "/ws/c.json") + want := "'${containerEnv}' in c.json can not be resolved because no environment variable name is given." + if ce, ok := coreerrors.AsContainerError(err); !ok || ce.Description != want { + t.Errorf("unnamed reference: err = %v, want the ContainerError %q", err, want) + } +} + +// TS readConfiguration resolves ${devcontainerId} in the configuration from +// the found container's id labels (beforeContainerSubstitute); without a +// container it stays as written. +func TestSubstituteDevContainerID(t *testing.T) { + cfgMap := map[string]any{"name": "${devcontainerId}", "remoteEnv": map[string]any{"P": "${containerEnv:PATH}"}} + got, err := substituteDevContainerID(nil, cfgMap, nil, "", nil) + if err != nil || !reflect.DeepEqual(got, cfgMap) { + t.Fatalf("without a container = %v, %v; want the config unchanged", got, err) + } + result := &config.LoadResult{Config: &config.DevContainer{ConfigFilePath: "/ws/.devcontainer/devcontainer.json"}} + labelled := &container.Config{Labels: map[string]string{"devcontainer.config_file": "/other.json"}} + for name, tc := range map[string]struct { + ctr *container.Config + provided []string + ws string + result *config.LoadResult + labels map[string]string + }{ + "--id-label": {labelled, []string{"a=b"}, "/ws", result, map[string]string{"a": "b"}}, + "workspace and config": {labelled, nil, "/ws", result, map[string]string{"devcontainer.local_folder": "/ws", "devcontainer.config_file": "/ws/.devcontainer/devcontainer.json"}}, + "older container": {&container.Config{}, nil, "/ws", result, map[string]string{"devcontainer.local_folder": "/ws"}}, + "--container-id alone": {labelled, nil, "", nil, map[string]string{"devcontainer.local_folder": "undefined", "devcontainer.config_file": "undefined"}}, + } { + got, err := substituteDevContainerID(tc.ctr, cfgMap, tc.provided, tc.ws, tc.result) + if err != nil { + t.Fatalf("%s: %v", name, err) + } + want := map[string]any{"name": config.ComputeDevContainerID(tc.labels), "remoteEnv": map[string]any{"P": "${containerEnv:PATH}"}} + if !reflect.DeepEqual(got, want) { + t.Errorf("%s = %v, want %v", name, got, want) + } + } } func TestWorkspaceOutput(t *testing.T) { withMount := &config.LoadResult{WorkspaceConfig: &config.WorkspaceConfig{WorkspaceFolder: "/workspaces/p", WorkspaceMount: "type=bind,source=/p,target=/workspaces/p"}} withoutMount := &config.LoadResult{WorkspaceConfig: &config.WorkspaceConfig{WorkspaceFolder: "/workspaces/p"}} worktree := &config.LoadResult{WorkspaceConfig: &config.WorkspaceConfig{WorkspaceFolder: "/workspaces/wt", WorkspaceMount: "type=bind,source=/r/wt,target=/workspaces/wt", AdditionalMounts: []string{"type=bind,source=/r/repo/.git,target=/workspaces/repo/.git"}}} + // Without a workspace the loader leaves the configuration's own folder and + // mount, possibly empty: TS writes what is set (undefined is left out). + noWorkspace := &config.LoadResult{WorkspaceConfig: &config.WorkspaceConfig{}} for _, tc := range []struct { - name string - result *config.LoadResult - ws, container string - want map[string]any - ok bool + name string + result *config.LoadResult + want map[string]any + ok bool }{ - {"no config", nil, "/p", "", nil, false}, - {"no workspace config", &config.LoadResult{}, "/p", "", nil, false}, - {"folder and mount", withMount, "/p", "", map[string]any{"workspaceFolder": "/workspaces/p", "workspaceMount": "type=bind,source=/p,target=/workspaces/p"}, true}, - {"folder only", withoutMount, "/p", "", map[string]any{"workspaceFolder": "/workspaces/p"}, true}, - {"worktree common dir", worktree, "/r/wt", "", map[string]any{"workspaceFolder": "/workspaces/wt", "workspaceMount": "type=bind,source=/r/wt,target=/workspaces/wt", "additionalMountString": "type=bind,source=/r/repo/.git,target=/workspaces/repo/.git"}, true}, - {"container id without workspace folder", withMount, "", "abc", map[string]any{}, true}, - {"container id and workspace folder", withoutMount, "/p", "abc", map[string]any{"workspaceFolder": "/workspaces/p"}, true}, - {"id label without workspace folder", withoutMount, "", "", map[string]any{"workspaceFolder": "/workspaces/p"}, true}, + {"no config", nil, nil, false}, + {"no workspace config", &config.LoadResult{}, nil, false}, + {"folder and mount", withMount, map[string]any{"workspaceFolder": "/workspaces/p", "workspaceMount": "type=bind,source=/p,target=/workspaces/p"}, true}, + {"folder only", withoutMount, map[string]any{"workspaceFolder": "/workspaces/p"}, true}, + {"worktree common dir", worktree, map[string]any{"workspaceFolder": "/workspaces/wt", "workspaceMount": "type=bind,source=/r/wt,target=/workspaces/wt", "additionalMountString": "type=bind,source=/r/repo/.git,target=/workspaces/repo/.git"}, true}, + {"no workspace, nothing configured", noWorkspace, map[string]any{}, true}, } { t.Run(tc.name, func(t *testing.T) { - got, ok := workspaceOutput(tc.result, tc.ws, tc.container) + got, ok := workspaceOutput(tc.result) if ok != tc.ok || !reflect.DeepEqual(got, tc.want) { t.Fatalf("workspaceOutput = %v, %v; want %v, %v", got, ok, tc.want, tc.ok) } @@ -252,7 +305,7 @@ func TestWorkspaceOutput(t *testing.T) { // TS writes the workspace as getWorkspaceConfiguration's literal lists it. func TestWorkspaceOutputKeepsTSOrder(t *testing.T) { - got, err := stringifyJSON(orderedObject{map[string]any{"additionalMountString": "a", "workspaceMount": "m", "workspaceFolder": "f"}, workspaceKeys}, "") + got, err := stringify.JSON(orderedObject{map[string]any{"additionalMountString": "a", "workspaceMount": "m", "workspaceFolder": "f"}, workspaceKeys}, "") if err != nil { t.Fatal(err) } @@ -303,7 +356,7 @@ func TestMergedConfigurationOutput(t *testing.T) { if err != nil { t.Fatal(err) } - got, err := mergedConfigurationOutput(entries, cc, cfgMap) + got, err := mergedConfigurationOutput(entries, cc, cfgMap, "") if err != nil { t.Fatal(err) } @@ -418,6 +471,10 @@ func TestConfigMetadataEntry(t *testing.T) { // stdout parsed, or the command's error. func runReadConfigurationCmd(t *testing.T, args ...string) (map[string]any, error) { t.Helper() + // read-configuration looks for the container (TS findContainerAndIdLabels), + // and a lookup that fails fails the command, so an engine with no + // containers answers it. + startFakeEngine(t, nil) stdout, err := readConfigurationStdout(args...) if err != nil { return nil, err @@ -471,6 +528,75 @@ func TestReadConfigurationCommandContainerIDWithoutWorkspace(t *testing.T) { } } +// Without a workspace folder (--id-label, no --workspace-folder), TS reads +// --override-config as the configuration (configPath = overrideConfigFile): +// it is printed, its configFilePath the override's file URI. Go printed an +// empty configuration. +func TestReadConfigurationCommandOverrideConfigWithoutWorkspace(t *testing.T) { + override := filepath.Join(t.TempDir(), "override.json") + if err := os.WriteFile(override, []byte(`{"image":"img:1","name":"${localWorkspaceFolderBasename}"}`), 0o644); err != nil { + t.Fatal(err) + } + out, err := runReadConfigurationCmd(t, "--id-label", "devcontainer-cli-test=override-without-workspace", "--override-config", override) + if err != nil { + t.Fatal(err) + } + cfg, _ := out["configuration"].(map[string]any) + if cfg["image"] != "img:1" || cfg["name"] != "${localWorkspaceFolderBasename}" { + t.Errorf("configuration = %v, want the override's with no workspace to substitute", cfg) + } + want := map[string]any{"$mid": float64(1), "fsPath": override, "path": override, "scheme": "file"} + if !reflect.DeepEqual(cfg["configFilePath"], want) { + t.Errorf("configFilePath = %v, want %v", cfg["configFilePath"], want) + } + if ws, ok := out["workspace"].(map[string]any); !ok || len(ws) != 0 { + t.Errorf("workspace = %v, want an empty object", out["workspace"]) + } +} + +// Without a workspace folder, TS getWorkspaceConfiguration keeps the +// configuration's own workspaceFolder and workspaceMount, which +// read-configuration prints as the workspace. Go printed an empty workspace +// with --container-id, and an empty workspaceFolder without one. +func TestReadConfigurationCommandWorkspaceWithoutWorkspaceFolder(t *testing.T) { + cfg := filepath.Join(t.TempDir(), "devcontainer.json") + if err := os.WriteFile(cfg, []byte(`{"image":"img:1","workspaceFolder":"/wf","workspaceMount":"type=bind,source=/a,target=/wf"}`), 0o644); err != nil { + t.Fatal(err) + } + out, err := runReadConfigurationCmd(t, "--id-label", "devcontainer-cli-test=workspace-without-folder", "--config", cfg) + if err != nil { + t.Fatal(err) + } + want := map[string]any{"workspaceFolder": "/wf", "workspaceMount": "type=bind,source=/a,target=/wf"} + if !reflect.DeepEqual(out["workspace"], want) { + t.Errorf("workspace = %v, want %v", out["workspace"], want) + } +} + +// An unresolvable ${containerEnv} fails read-configuration as TS's +// ContainerError does: logged, naming the configuration's file, exit 1 and +// nothing on stdout. +func TestReadConfigurationCommandUnresolvableContainerVariable(t *testing.T) { + startFakeEngine(t, map[string]map[string]string{"c1": {}}) + cfg := filepath.Join(t.TempDir(), "c.json") + if err := os.WriteFile(cfg, []byte(`{"image":"img","remoteEnv":{"A":"${containerEnv}"}}`), 0o644); err != nil { + t.Fatal(err) + } + logFile := filepath.Join(t.TempDir(), "log.txt") + stdout, err := readConfigurationStdout("--container-id", "c1", "--config", cfg, "--log-file", logFile) + wantExitCode(t, err, 1) + if stdout != "" { + t.Errorf("stdout = %q, want nothing", stdout) + } + logged, readErr := os.ReadFile(logFile) + if readErr != nil { + t.Fatal(readErr) + } + if want := "'${containerEnv}' in c.json can not be resolved because no environment variable name is given."; !strings.Contains(string(logged), want) { + t.Errorf("log = %q, want %q", logged, want) + } +} + // --cache-key over a malformed devcontainer-lock.json is a runtime failure: // the error envelope on stdout naming the lockfile, exit 1. Without the flag // the command does not read the lockfile, as the reference's @@ -605,23 +731,87 @@ func TestReadConfigurationCommandMergedWithoutImageExitsOne(t *testing.T) { } func TestReadConfigurationCommandWithoutDockerEngine(t *testing.T) { - // Without a Docker engine the configuration is still read, with a warning. + // Without a Docker engine the container lookup fails, and so does the + // command, with nothing on stdout: TS's findContainerAndIdLabels runs + // `docker ps`, whose failure exits read-configuration 1. t.Setenv("DOCKER_HOST", "not a docker host") ws := writeConfigFixture(t, map[string]string{".devcontainer/devcontainer.json": `{"image": "img:1"}`}) logFile := filepath.Join(t.TempDir(), "log.txt") - out, err := runReadConfigurationCmd(t, "--workspace-folder", ws, "--log-file", logFile) + stdout, err := readConfigurationStdout("--workspace-folder", ws, "--log-file", logFile) + wantExitCode(t, err, 1) + if stdout != "" { + t.Errorf("stdout = %q, want nothing", stdout) + } + logged, err := os.ReadFile(logFile) if err != nil { t.Fatal(err) } - if cfg, _ := out["configuration"].(map[string]any); cfg["image"] != "img:1" { - t.Errorf("configuration = %v", out["configuration"]) + for _, want := range []string{"Warning: Docker engine:", "Command failed: docker ps -q -a --filter label=devcontainer.local_folder=" + ws} { + if !strings.Contains(string(logged), want) { + t.Errorf("log = %q, want %q", logged, want) + } } - logged, err := os.ReadFile(logFile) - if err != nil { +} + +// A container lookup by --id-label that fails exits 1 with nothing on stdout, +// as TS's findDevContainer `docker ps` failure does. Go printed the +// configuration and exited 0. +func TestReadConfigurationCommandFailedIDLabelLookup(t *testing.T) { + t.Setenv("DOCKER_HOST", "unix:///nonexistent/docker.sock") + cfg := filepath.Join(t.TempDir(), "devcontainer.json") + if err := os.WriteFile(cfg, []byte(`{"image":"img:1"}`), 0o644); err != nil { t.Fatal(err) } - if !strings.Contains(string(logged), "Warning: Docker engine:") { - t.Errorf("log = %q, want the Docker engine warning", logged) + stdout, err := readConfigurationStdout("--id-label", "devcontainer-cli-test=lookup", "--override-config", cfg) + wantExitCode(t, err, 1) + if stdout != "" { + t.Errorf("stdout = %q, want nothing", stdout) + } + if want := "Command failed: docker ps -q -a --filter label=devcontainer-cli-test=lookup"; err.Error() != want { + t.Errorf("err = %v, want %q", err, want) + } +} + +// Reading the Features configuration writes TS generateFeaturesConfig's trace +// lines (containerFeaturesConfiguration.ts): the workspace root, then, with no +// Features, that there are no user Features. Without a request for the +// Features nothing is read. +func TestReadConfigurationCommandTracesReadingTheFeatures(t *testing.T) { + t.Setenv("DOCKER_HOST", "not a docker host") + // traced is the log's lines without their timestamps. + traced := func(ws string, args ...string) []string { + t.Helper() + logFile := filepath.Join(t.TempDir(), "log.txt") + if _, err := runReadConfigurationCmd(t, append([]string{"--workspace-folder", ws, "--log-level", "trace", "--log-file", logFile}, args...)...); err != nil { + t.Fatal(err) + } + logged, err := os.ReadFile(logFile) + if err != nil { + t.Fatal(err) + } + var lines []string + for line := range strings.Lines(string(logged)) { + _, text, _ := strings.Cut(strings.TrimSuffix(line, "\n"), "] ") + lines = append(lines, text) + } + return lines + } + ws := writeConfigFixture(t, map[string]string{".devcontainer/devcontainer.json": `{"image": "img:1"}`}) + lines := traced(ws, "--include-features-configuration") + if i := slices.Index(lines, "workspace root: "+ws); i < 0 || i+1 == len(lines) || lines[i+1] != "No user features to update" { + t.Errorf("log = %q, want the workspace root and then no user features", lines) + } + if lines := traced(ws); slices.ContainsFunc(lines, func(l string) bool { + return strings.HasPrefix(l, "workspace root:") || l == "No user features to update" + }) { + t.Errorf("log without --include-features-configuration = %q", lines) + } + + files := map[string]string{".devcontainer/devcontainer.json": `{"image": "img:1", "features": {"./feat": {}}}`} + maps.Copy(files, localFeatureFiles) + ws = writeConfigFixture(t, files) + if lines := traced(ws, "--include-features-configuration"); !slices.Contains(lines, "workspace root: "+ws) || slices.Contains(lines, "No user features to update") { + t.Errorf("log with a Feature = %q, want the workspace root alone", lines) } } diff --git a/internal/cli/result_order_test.go b/internal/cli/result_order_test.go index dde0a21..b161ed4 100644 --- a/internal/cli/result_order_test.go +++ b/internal/cli/result_order_test.go @@ -14,6 +14,8 @@ import ( // configFilePath last, FeaturesConfig's literal), as the TS oracle printed // for these documents. func TestReadConfigurationWritesTSKeyOrder(t *testing.T) { + // read-configuration looks for the container; this engine has none. + startFakeEngine(t, nil) files := map[string]string{".devcontainer/devcontainer.json": `{ // a comment "name": "ko", diff --git a/internal/cli/run_user_commands.go b/internal/cli/run_user_commands.go index b5f12ba..461b6a2 100644 --- a/internal/cli/run_user_commands.go +++ b/internal/cli/run_user_commands.go @@ -162,7 +162,18 @@ func runUserCommandsInContainer(ctx context.Context, out Output, opts *runUserCo containerID := opts.containerID if containerID == "" { - containerID = resolveContainerID(ctx, engine, opts.workspaceFolder, "", opts.idLabels) + // TS looks the container up by the workspace folder and the + // configuration's file (findContainerAndIdLabels); a failed lookup is + // its error, described as running the user commands. + lookupConfigFile := "" + if loadResult != nil { + lookupConfigFile = loadResult.Config.ConfigFilePath + } + var lookupErr error + containerID, lookupErr = resolveContainerID(ctx, engine, opts.workspaceFolder, lookupConfigFile, opts.idLabels) + if lookupErr != nil { + return writeErrorJSON(out, coreerrors.ToErrorOutput(&coreerrors.ContainerError{Description: "An error occurred running user commands in the container.", OriginalError: lookupErr})) + } } if containerID == "" { return writeErrorResult(out, "Dev container not found.") @@ -197,6 +208,24 @@ func runUserCommandsInContainer(ctx context.Context, out Output, opts *runUserCo return writeErrorResult(out, err.Error()) } merged := imagemeta.MergeConfiguration(allEntries) + // TS doRunUserCommands substitutes the container's environment into the + // configuration and the merged configuration before it runs anything; a + // variable that cannot be resolved is the ContainerError it reports, + // naming the configuration's file. + var rawConfig any = cfg + if loadResult != nil { + rawConfig = loadResult.Raw + } + if cfg != nil { + configFile = cfg.ConfigFilePath + } + var containerEnv map[string]string + if inspect.Config != nil { + containerEnv = config.EnvMap(inspect.Config.Env) + } + if err := checkContainerSubstitution(containerEnv, configFile, rawConfig, merged); err != nil { + return writeErrorJSON(out, coreerrors.ToErrorOutput(err)) + } remoteUser := lifecycleRemoteUser(merged, cfg) probed := lifecycle.ProbedRemoteEnv(ctx, engine, logger, containerID, remoteUser, lifecycleUserEnvProbe(opts.defaultUserEnvProbe, cfg), opts.containerSessionDataFolder) diff --git a/internal/cli/run_user_commands_held_test.go b/internal/cli/run_user_commands_held_test.go index 2ee3db8..1f96155 100644 --- a/internal/cli/run_user_commands_held_test.go +++ b/internal/cli/run_user_commands_held_test.go @@ -111,6 +111,32 @@ func TestRunUserCommands_ConfigLoadError_PreDocker(t *testing.T) { } } +// A container lookup that fails is TS's error: the failed `docker ps` by the +// workspace folder's and the configuration's labels as the message, running +// the user commands as the description. Go reported "Dev container not +// found.". +func TestRunUserCommandsFailedContainerLookup(t *testing.T) { + t.Setenv("DOCKER_HOST", "unix:///nonexistent/devcontainer-cli-test.sock") + opts := validRunUserCommandsOpts() + opts.workspaceFolder = t.TempDir() + cfg := filepath.Join(opts.workspaceFolder, ".devcontainer", "devcontainer.json") + if err := os.MkdirAll(filepath.Dir(cfg), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(cfg, []byte(`{"image":"img"}`), 0o644); err != nil { + t.Fatal(err) + } + out := &captureOutput{} + err := runUserCommands(t.Context(), out, opts) + if ece := asExitCode(t, err); ece.Code != 1 { + t.Errorf("exit code = %d, want 1", ece.Code) + } + want := `{"outcome":"error","message":"Command failed: docker ps -q -a --filter label=devcontainer.local_folder=` + opts.workspaceFolder + ` --filter label=devcontainer.config_file=` + cfg + `","description":"An error occurred running user commands in the container."}` + if got := strings.TrimSpace(out.out.String()); got != want { + t.Errorf("stdout = %s\nwant %s", got, want) + } +} + func TestConfigFromFlag(t *testing.T) { dir := t.TempDir() t.Chdir(dir) diff --git a/internal/cli/setup.go b/internal/cli/setup.go index 7e64d24..15e8c36 100644 --- a/internal/cli/setup.go +++ b/internal/cli/setup.go @@ -158,6 +158,10 @@ func setUpContainer(ctx context.Context, out Output, engine *docker.EngineClient } entries := setUpEntries(inspect.Config.Labels, cfg, logger) merged := imagemeta.MergeConfiguration(entries) + result, err := setUpResult(loaded, entries, inspect.Config, opts.includeConfig, opts.includeMerged) + if err != nil { + return writeErrorJSON(out, setUpErrorOutput(err)) + } var cfgProbe string var cfgRemoteEnv map[string]*string @@ -190,10 +194,6 @@ func setUpContainer(ctx context.Context, out Output, engine *docker.EngineClient } } - result, err := setUpResult(loaded, entries, inspect.Config, opts.includeConfig, opts.includeMerged) - if err != nil { - return writeErrorResult(out, err.Error()) - } orderConfigurations(result, loaded) return writeSuccessJSON(out, result, setUpResultKeys...) } @@ -232,33 +232,37 @@ func setUpEntries(labels map[string]string, cfg *config.DevContainer, logger log // merge of entries over it, both with ${containerEnv:...} resolved from the // container's environment. configFilePath is the file URI, which TS never // substitutes. +// +// TS substitutes both before it runs the lifecycle hooks, whether or not they +// are reported, so set-up computes the result first: a variable that cannot be +// resolved is the ContainerError it fails with, naming the configuration's +// file. func setUpResult(loaded *config.LoadResult, entries []imagemeta.Entry, containerCfg *container.Config, includeConfig, includeMerged bool) (map[string]any, error) { - result := map[string]any{"outcome": "success"} - if !includeConfig && !includeMerged { - return result, nil - } cfgMap := map[string]any{} var configFileURI any + configFile := "" if loaded != nil { - cfgMap = configurationOutput(loaded, &readConfigOpts{configPath: loaded.Config.ConfigFilePath}) + configFile = loaded.Config.ConfigFilePath + cfgMap = configurationOutput(loaded, &readConfigOpts{configPath: configFile}) configFileURI = cfgMap["configFilePath"] delete(cfgMap, "configFilePath") } - substituted, err := substituteContainerEnv(containerCfg, cfgMap) + substituted, err := substituteContainerEnv(containerCfg, cfgMap, configFile) if err != nil { - return nil, fmt.Errorf("resolve configuration container variables: %w", err) + return nil, err } if configFileURI != nil { substituted["configFilePath"] = configFileURI } + merged, err := mergedConfigurationOutput(entries, containerCfg, substituted, configFile) + if err != nil { + return nil, err + } + result := map[string]any{"outcome": "success"} if includeConfig { result["configuration"] = substituted } if includeMerged { - merged, err := mergedConfigurationOutput(entries, containerCfg, substituted) - if err != nil { - return nil, fmt.Errorf("resolve merged container variables: %w", err) - } result["mergedConfiguration"] = merged } return result, nil diff --git a/internal/cli/setup_held_test.go b/internal/cli/setup_held_test.go index 0daf5bc..57650f8 100644 --- a/internal/cli/setup_held_test.go +++ b/internal/cli/setup_held_test.go @@ -11,6 +11,7 @@ import ( "testing" "github.com/devcontainers/cli/internal/config" + coreerrors "github.com/devcontainers/cli/internal/errors" "github.com/devcontainers/cli/internal/imagemeta" "github.com/devcontainers/cli/internal/log" "github.com/moby/moby/api/types/container" @@ -290,13 +291,21 @@ func TestSetUpResultLeavesTheConfigFilePathUnsubstituted(t *testing.T) { } } +// A ${containerEnv} without a name is TS's ContainerError, naming the +// configuration's file, and TS substitutes the configuration and the merged +// configuration whether or not they are reported. func TestSetUpResultReportsAContainerEnvReferenceWithoutAName(t *testing.T) { loaded := loadedConfig(t, "/ws/devcontainer.json", map[string]any{"remoteEnv": map[string]any{"X": "${containerEnv}"}}) - if _, err := setUpResult(loaded, nil, &container.Config{}, true, false); err == nil || !strings.HasPrefix(err.Error(), "resolve configuration container variables: ") { - t.Errorf("configuration error = %v, want the substitution error", err) + want := "'${containerEnv}' in devcontainer.json can not be resolved because no environment variable name is given." + for _, include := range []bool{true, false} { + _, err := setUpResult(loaded, nil, &container.Config{}, include, false) + if ce, ok := coreerrors.AsContainerError(err); !ok || ce.Description != want { + t.Errorf("configuration (reported %v) error = %v, want the ContainerError %q", include, err, want) + } } entries := []imagemeta.Entry{{RemoteEnv: map[string]*string{"X": strPtr("${containerEnv}")}}} - if _, err := setUpResult(nil, entries, &container.Config{}, false, true); err == nil || !strings.HasPrefix(err.Error(), "resolve merged container variables: ") { - t.Errorf("merged error = %v, want the substitution error", err) + want = "'${containerEnv}' can not be resolved because no environment variable name is given." + if _, err := setUpResult(nil, entries, &container.Config{}, false, false); err == nil || err.Error() != want { + t.Errorf("merged error = %v, want %q", err, want) } } diff --git a/internal/cli/stop.go b/internal/cli/stop.go index 4a61f58..841b19b 100644 --- a/internal/cli/stop.go +++ b/internal/cli/stop.go @@ -147,5 +147,8 @@ func resolveWorkspaceContainer(ctx context.Context, engine *docker.EngineClient, if opts.configPath != "" { configFile = resolvePath(opts.configPath) } - return resolveContainerID(ctx, engine, opts.workspaceFolder, configFile, opts.idLabels) + // A failed lookup is no container here, as it was before lookups reported + // their failure. + id, _ := resolveContainerID(ctx, engine, opts.workspaceFolder, configFile, opts.idLabels) + return id } diff --git a/internal/cli/stringify.go b/internal/cli/stringify.go deleted file mode 100644 index f2f5696..0000000 --- a/internal/cli/stringify.go +++ /dev/null @@ -1,54 +0,0 @@ -package cli - -import "encoding/json" - -// stringifyEscapes are the escapes encoding/json writes that JSON.stringify -// does not: it writes &, <, > and U+2028/U+2029 as they are. -var stringifyEscapes = map[string]string{ - "\\u0026": "&", - "\\u003c": "<", - "\\u003e": ">", - "\\u2028": "\u2028", - "\\u2029": "\u2029", -} - -// stringifyJSON encodes v as JSON.stringify(v) would, so a command result is -// byte-compatible with the TS CLI's. encoding/json HTML-escapes every string, -// including the ones nested json.Marshaler implementations write, so the -// escapes are undone on the encoded document. A non-empty indent indents it -// as JSON.stringify(v, undefined, indent) does. -func stringifyJSON(v any, indent string) ([]byte, error) { - var data []byte - var err error - if indent == "" { - data, err = json.Marshal(v) - } else { - data, err = json.MarshalIndent(v, "", indent) - } - if err != nil { - return nil, err - } - return unescapeStringify(data), nil -} - -// unescapeStringify rewrites, in encoded JSON, the escapes in -// stringifyEscapes as the characters they stand for. A backslash only occurs -// in a string, always as the start of an escape, so taking escapes in pairs -// leaves an escaped backslash followed by "u0026" alone. -func unescapeStringify(data []byte) []byte { - out := make([]byte, 0, len(data)) - for i := 0; i < len(data); i++ { - if data[i] != '\\' { - out = append(out, data[i]) - continue - } - if s, ok := stringifyEscapes[string(data[i:min(i+6, len(data))])]; ok { - out = append(out, s...) - i += 5 - continue - } - out = append(out, data[i], data[i+1]) - i++ - } - return out -} diff --git a/internal/cli/stringify_test.go b/internal/cli/stringify_test.go index 7086ebe..7a60fd2 100644 --- a/internal/cli/stringify_test.go +++ b/internal/cli/stringify_test.go @@ -6,45 +6,6 @@ import ( coreerrors "github.com/devcontainers/cli/internal/errors" ) -// JSON.stringify writes &, <, > and U+2028/U+2029 as they are, and every -// other escape as encoding/json does. -func TestStringifyJSONWritesWhatJSONStringifyWrites(t *testing.T) { - cases := []struct { - name string - v any - indent string - want string - }{ - {"html characters", "a && b ", "", "\"a && b \""}, - {"line and paragraph separators", "\u2028\u2029", "", "\"\u2028\u2029\""}, - {"an escaped backslash before u0026", `\u0026`, "", `"\\u0026"`}, - {"an escaped quote at the end", `"`, "", `"\""`}, - {"control characters", "\b\f\n\r\t\x01", "", `"\b\f\n\r\t\u0001"`}, - {"a nested marshaler", orderedObject{map[string]any{"": "&"}, nil}, "", `{"":"&"}`}, - {"indented", map[string]any{"k": ""}, " ", "{\n \"k\": \"\"\n}"}, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - got, err := stringifyJSON(tc.v, tc.indent) - if err != nil { - t.Fatal(err) - } - if string(got) != tc.want { - t.Errorf("stringifyJSON = %s, want %s", got, tc.want) - } - }) - } -} - -func TestStringifyJSONReportsAnUnencodableValue(t *testing.T) { - if _, err := stringifyJSON(make(chan int), ""); err == nil { - t.Error("stringifyJSON of a channel succeeded") - } - if _, err := stringifyJSON(make(chan int), " "); err == nil { - t.Error("indented stringifyJSON of a channel succeeded") - } -} - // A command's error result is written as JSON.stringify writes it. func TestWriteErrorJSONDoesNotEscapeHTML(t *testing.T) { var out captureOutput diff --git a/internal/cli/templates_apply.go b/internal/cli/templates_apply.go index 4bced62..3178c12 100644 --- a/internal/cli/templates_apply.go +++ b/internal/cli/templates_apply.go @@ -7,6 +7,7 @@ import ( "github.com/devcontainers/cli/internal/jsonc" "github.com/devcontainers/cli/internal/log" + "github.com/devcontainers/cli/internal/stringify" "github.com/devcontainers/cli/internal/templates" "github.com/spf13/cobra" ) @@ -51,7 +52,7 @@ func realTemplatesApplyCmd() *cobra.Command { return err } - data, _ := stringifyJSON(map[string]any{"files": files}, "") + data, _ := stringify.JSON(map[string]any{"files": files}, "") fmt.Fprintln(outputFor(cmd).Stdout(), string(data)) return nil }, diff --git a/internal/cli/up.go b/internal/cli/up.go index 7bbe661..5b3543b 100644 --- a/internal/cli/up.go +++ b/internal/cli/up.go @@ -441,6 +441,19 @@ func runUp(ctx context.Context, out Output, opts *upOpts) error { remoteWorkspaceFolder := loadResult.WorkspaceConfig.WorkspaceFolder + // TS setupInContainer substitutes the container's environment into the + // configuration and the merged configuration before the hooks, even with + // --skip-post-create: a variable that cannot be resolved fails up, with + // the container reported. + if subErr := run.checkContainerSubstitution(inspectResp, cfg, loadResult.Raw); subErr != nil { + ce, ok := coreerrors.AsContainerError(subErr) + if !ok { + ce = &coreerrors.ContainerError{Description: "An error occurred setting up the container.", OriginalError: subErr} + } + ce.ContainerID = containerID + return writeErrorJSON(out, coreerrors.ToErrorOutput(ce)) + } + // Run lifecycle hooks (unless skipped). A hook failure aborts `up` with an // error outcome (matching the TS CLI), instead of reporting success. if !opts.skipPostCreate { @@ -558,7 +571,7 @@ func addUpConfiguration(result map[string]any, opts *upOpts, fsPathRead bool, cf cfgJSON, _ := json.Marshal(cfg) var cfgGeneric any json.Unmarshal(cfgJSON, &cfgGeneric) - substituted, subErr := resolveContainerVariables(containerEnv, cfgGeneric) + substituted, subErr := resolveContainerVariables(containerEnv, cfg.ConfigFilePath, cfgGeneric) if subErr != nil { return fmt.Errorf("resolve configuration container variables: %w", subErr) } @@ -600,7 +613,7 @@ func addUpConfiguration(result map[string]any, opts *upOpts, fsPathRead bool, cf mergedJSON, _ := json.Marshal(merged) var mergedGeneric any json.Unmarshal(mergedJSON, &mergedGeneric) - mergedSub, subErr := resolveContainerVariables(containerEnv, mergedGeneric) + mergedSub, subErr := resolveContainerVariables(containerEnv, cfg.ConfigFilePath, mergedGeneric) if subErr != nil { return fmt.Errorf("resolve merged container variables: %w", subErr) } @@ -960,13 +973,14 @@ func containerRunUser(metadataUser string, img image.InspectResponse) string { // upWorkspaceMounts are the container's workspace mounts. The workspace mount is // the computed workspaceMount (which honors a custom `workspaceMount` in the // config and the git-root source folder), else a bind of the workspace folder; -// with no workspace and no workspaceMount (up --config alone) TS mounts nothing. +// with no workspace and no workspaceMount (up --config alone), or with an empty +// or null workspaceMount, TS mounts nothing. // The extra workspace-derived mounts (currently the Git worktree common dir from // --mount-git-worktree-common-dir) follow so Git works inside the container. // --workspace-mount-consistency, when set, applies to all of them. func upWorkspaceMounts(wc *config.WorkspaceConfig, workspaceFolder, consistency string) ([]dockermount.Mount, error) { var mounts []dockermount.Mount - if spec := wc.WorkspaceMount; spec != "" || workspaceFolder != "" { + if spec := wc.WorkspaceMount; !wc.NoWorkspaceMount && (spec != "" || workspaceFolder != "") { wsMount := dockermount.Mount{Type: dockermount.TypeBind, Source: workspaceFolder, Target: wc.WorkspaceFolder} if spec != "" { parsed, parseErr := docker.ParseMountSpec(spec) @@ -1237,6 +1251,24 @@ func (r *upRunner) runLifecycleForUp(ctx context.Context, containerID string, cf return nil } +// checkContainerSubstitution is TS setupInContainer's containerSubstitute +// over the configuration (raw) and the merged configuration (the container's +// metadata), for its failure alone. +// +// mutate-lane: TestParityMatrix - up.container-env-without-name-error fails +// up through it against a real container. +func (r *upRunner) checkContainerSubstitution(inspectResp container.InspectResponse, cfg *config.DevContainer, raw map[string]any) error { + entries, err := r.metadataOfContainer(inspectResp, cfg) + if err != nil { + return err + } + var containerEnv map[string]string + if inspectResp.Config != nil { + containerEnv = config.EnvMap(inspectResp.Config.Env) + } + return checkContainerSubstitution(containerEnv, cfg.ConfigFilePath, raw, imagemeta.MergeConfiguration(entries)) +} + // secretsRemoteEnv are the --secrets-file variables (NAME=value) for the // lifecycle hooks. A file that cannot be read or parsed is a warning, not an // error, and contributes nothing. @@ -1376,6 +1408,7 @@ func (r *upRunner) featureBuildOptions(cfg *config.DevContainer, loadResult *con Secrets: buildSecretsFromFile(opts.secretsFile), OverrideFeatureInstallOrder: cfg.OverrideFeatureInstallOrder, FeaturesBasePath: filepath.Dir(cfg.ConfigFilePath), + WorkspaceFolder: opts.workspaceFolder, SkipFeatureAutoMapping: opts.skipFeatureAutoMapping, FrozenLockfile: opts.experimentalFrozenLockfile, NoLockfile: opts.noLockfile, diff --git a/internal/cli/up_held_test.go b/internal/cli/up_held_test.go index af98648..940dd01 100644 --- a/internal/cli/up_held_test.go +++ b/internal/cli/up_held_test.go @@ -337,6 +337,8 @@ func TestUpWorkspaceMounts(t *testing.T) { []dockermount.Mount{bind("/s", "/w", "consistent"), bind("/git", "/git", "delegated")}}, {"worktree common dir", config.WorkspaceConfig{WorkspaceFolder: "/w", AdditionalMounts: []string{"type=bind,source=/git,target=/git"}}, "/src", "cached", []dockermount.Mount{bind("/src", "/w", "cached"), bind("/git", "/git", "cached")}}, + {"empty workspaceMount mounts no workspace", config.WorkspaceConfig{WorkspaceFolder: "/w", NoWorkspaceMount: true, AdditionalMounts: []string{"type=bind,source=/git,target=/git"}}, "/src", "cached", + []dockermount.Mount{bind("/git", "/git", "cached")}}, } { t.Run(tc.name, func(t *testing.T) { got, err := upWorkspaceMounts(&tc.wc, tc.ws, tc.consistency) diff --git a/internal/config/loader.go b/internal/config/loader.go index 62bcec7..1444218 100644 --- a/internal/config/loader.go +++ b/internal/config/loader.go @@ -6,6 +6,7 @@ import ( "encoding/json" "errors" "fmt" + "io/fs" "maps" "os" "path" @@ -91,6 +92,10 @@ type WorkspaceConfig struct { // operations work inside the container for `git worktree add --relative-paths` // checkouts (--mount-git-worktree-common-dir). AdditionalMounts []string `json:"-"` + // NoWorkspaceMount is set when the configuration's workspaceMount is empty + // or null: the container gets no workspace mount (TS spawnDevContainer + // passes no --mount for a falsy workspaceMount). + NoWorkspaceMount bool `json:"-"` } // Workspace describes the local workspace. @@ -162,9 +167,17 @@ func LoadDevContainerConfig(workspaceFolder, configPath, overrideConfigPath stri func LoadDevContainerConfigWithMounts(workspaceFolder, configPath, overrideConfigPath string, mounts MountOptions) (*LoadResult, error) { workspace := WorkspaceFromPath(workspaceFolder) - // Discover config path - if configPath == "" { + // Discover config path. As TS does (getDevContainerConfigPathIn, then + // getDefaultDevContainerConfigPath), a workspace without a devcontainer.json + // names its default path when --override-config is given: that path is the + // configFilePath relative paths in the override resolve against. Without a + // workspace, only the override names the configuration (configPath = + // overrideConfigFile), so nothing is discovered in the current directory. + if configPath == "" && (workspaceFolder != "" || overrideConfigPath == "") { configPath = FindConfigFile(workspace.ConfigFolderPath) + if configPath == "" && overrideConfigPath != "" { + configPath = filepath.Join(workspace.ConfigFolderPath, ".devcontainer", "devcontainer.json") + } } if configPath == "" && overrideConfigPath == "" { defaultPath := filepath.Join(workspaceFolder, ".devcontainer", "devcontainer.json") @@ -197,6 +210,12 @@ func LoadDevContainerConfigWithMounts(workspaceFolder, configPath, overrideConfi } if overrideConfigPath != "" { overrideData, oerr := os.ReadFile(overrideConfigPath) + if errors.Is(oerr, fs.ErrNotExist) { + // TS reads the override in place of the configuration + // (readDocument(overrideConfigFile ?? configFile)): a missing one is the + // configuration not found, named by the configuration's path. + return nil, &NotFoundError{Path: objectPath} + } if oerr != nil { return nil, fmt.Errorf("read override config %s: %w", overrideConfigPath, oerr) } @@ -224,6 +243,8 @@ func LoadDevContainerConfigWithMounts(workspaceFolder, configPath, overrideConfi configFilePath = overrideConfigPath } + _, config.workspaceMountSet = raw["workspaceMount"] + // Compute workspace config using the caller's mount options. wsConfig := computeWorkspaceConfig(workspace, &config, mounts.MountWorkspaceGitRoot, mounts.MountGitWorktreeCommonDir) @@ -238,9 +259,11 @@ func LoadDevContainerConfigWithMounts(workspaceFolder, configPath, overrideConfi ConfigFilePath: configFilePath, } + // A variable that cannot be resolved is TS's ContainerError, reported as + // it is (no prefix of Go's own). substituted, err := NewVariableResolver().Resolve(SubstitutionContext{HostSubContext: ctx}, PhaseHost, raw) if err != nil { - return nil, fmt.Errorf("apply host substitutions: %w", err) + return nil, err } if m, ok := substituted.(map[string]any); ok { raw = m @@ -257,8 +280,12 @@ func LoadDevContainerConfigWithMounts(workspaceFolder, configPath, overrideConfi if config.WorkspaceFolder != "" { wsConfig.WorkspaceFolder = config.WorkspaceFolder } - if config.WorkspaceMount != "" { + // A workspaceMount the configuration sets replaces the computed one, even + // when it is empty or null, which mounts no workspace (TS + // readDevContainerConfigFile's `'workspaceMount' in config`). + if config.workspaceMountSet { wsConfig.WorkspaceMount = config.WorkspaceMount + wsConfig.NoWorkspaceMount = config.WorkspaceMount == "" } config.ConfigFilePath = configFilePath @@ -398,16 +425,15 @@ func computeWorkspaceConfig(workspace *Workspace, config *DevContainer, mountWor // gitdir still resolves to the mounted common dir inside the container. containerMountFolder := "/workspaces/" + filepath.Base(sourceFolder) var additionalMounts []string - if mountWorkspaceGitRoot && mountGitWorktreeCommonDir { - // A custom workspaceMount defines where the worktree is actually mounted in - // the container; resolve the common dir relative to that (substituted) - // target so the relative gitdir still resolves. Otherwise fall back to the - // computed container mount folder (#1261). - customTarget := "" - if config.WorkspaceMount != "" { - customTarget = substituteHostString(workspace, mountTarget(config.WorkspaceMount)) - } - if remapped, commonDirMount, ok := gitWorktreeCommonDirMount(sourceFolder, customTarget, consistency); ok { + // TS getWorkspaceConfiguration computes the workspace mount, and with it the + // worktree common-dir mount, only when the config leaves workspaceFolder or + // workspaceMount to it: a config that sets both gets neither. The common dir + // lands relative to the computed container mount folder, whatever target a + // custom workspaceMount names. + hasWorkspaceMount := config.WorkspaceMount != "" || config.workspaceMountSet + computed := config.WorkspaceFolder == "" || !hasWorkspaceMount + if computed && mountWorkspaceGitRoot && mountGitWorktreeCommonDir { + if remapped, commonDirMount, ok := gitWorktreeCommonDirMount(sourceFolder, consistency); ok { containerMountFolder = remapped additionalMounts = append(additionalMounts, commonDirMount) } @@ -431,7 +457,7 @@ func computeWorkspaceConfig(workspace *Workspace, config *DevContainer, mountWor if config.WorkspaceFolder != "" { wc.WorkspaceFolder = config.WorkspaceFolder } - if config.WorkspaceMount != "" { + if hasWorkspaceMount { wc.WorkspaceMount = config.WorkspaceMount } return wc @@ -460,7 +486,7 @@ func bindMount(source, target, consistency string) string { // the shared common dir (the main repo's `.git`) into the container. ok is false // for a normal clone (`.git` is a directory), an absolute gitdir, or a missing // gitlink — all cases where no extra mount is needed. -func gitWorktreeCommonDirMount(hostMountFolder, customContainerTarget, consistency string) (containerMountFolder, additionalMount string, ok bool) { +func gitWorktreeCommonDirMount(hostMountFolder, consistency string) (containerMountFolder, additionalMount string, ok bool) { info, err := os.Stat(filepath.Join(hostMountFolder, ".git")) if err != nil || !info.Mode().IsRegular() { return "", "", false @@ -487,68 +513,10 @@ func gitWorktreeCommonDirMount(hostMountFolder, customContainerTarget, consisten } containerMountFolder = path.Join(append([]string{"/workspaces"}, segments...)...) - // The common dir lands at the same relative offset from wherever the worktree - // is mounted: a custom workspaceMount target when the config sets one, else the - // computed container mount folder. - worktreeContainerFolder := containerMountFolder - if customContainerTarget != "" { - worktreeContainerFolder = customContainerTarget - } - containerGitCommonDir := path.Clean(path.Join(worktreeContainerFolder, filepath.ToSlash(gitdir), "..", "..")) + containerGitCommonDir := path.Clean(path.Join(containerMountFolder, filepath.ToSlash(gitdir), "..", "..")) return containerMountFolder, bindMount(gitCommonDir, containerGitCommonDir, consistency), true } -// mountTarget extracts the target= value from a `type=bind,...` mount spec, -// tolerating a quoted source/target that itself contains commas. -func mountTarget(spec string) string { - for _, f := range splitMountFields(spec) { - if t, ok := strings.CutPrefix(f, "target="); ok { - return t - } - } - return "" -} - -// splitMountFields splits a docker mount spec on commas that are not inside -// double quotes, stripping the quotes. -func splitMountFields(spec string) []string { - var fields []string - var b strings.Builder - inQuote := false - for _, r := range spec { - switch { - case r == '"': - inQuote = !inQuote - case r == ',' && !inQuote: - fields = append(fields, b.String()) - b.Reset() - default: - b.WriteRune(r) - } - } - return append(fields, b.String()) -} - -// substituteHostString applies host-phase variable substitution to s using a -// minimal host context (platform, local workspace folder, env). Used for the -// custom workspaceMount target, which is resolved before the main substitution -// pass. Returns s unchanged on error or when it has no ${...} tags. -func substituteHostString(workspace *Workspace, s string) string { - if !strings.Contains(s, "${") { - return s - } - ctx := HostSubContext{ - Platform: currentPlatform(), - LocalWorkspaceFolder: strings.TrimRight(workspace.RootFolderPath, "/\\"), - Env: EnvMap(os.Environ()), - } - out, err := NewVariableResolver().resolveString(SubstitutionContext{HostSubContext: ctx}, PhaseHost, s) - if err != nil { - return s - } - return out -} - // parseGitdir extracts the target of a `gitdir: ` line from a `.git` // gitlink file. No regexp, matching the rest of the package. func parseGitdir(content string) (string, bool) { diff --git a/internal/config/loader_nodocker_held_test.go b/internal/config/loader_nodocker_held_test.go index bac68f8..835ce40 100644 --- a/internal/config/loader_nodocker_held_test.go +++ b/internal/config/loader_nodocker_held_test.go @@ -31,10 +31,12 @@ func TestLoadReportsAMissingConfigFile(t *testing.T) { } } -// The config file path is the --config file, else the override file (TS -// readDevContainerConfigFile: overrideConfigFile ?? configFile) - never a -// "configFilePath" property of the file - and it is what host substitution -// errors name. +// The config file path is the --config file, else the one found in the +// workspace or, with only an override, the workspace's default path, else +// without a workspace the override file (TS resolveWithLocalFolder's +// configPath; readDevContainerConfigFile reads overrideConfigFile ?? +// configFile but keeps configFile) - never a "configFilePath" property of the +// file - and it is what host substitution errors name. func TestLoadSetsTheConfigFilePath(t *testing.T) { ws := t.TempDir() cfg := filepath.Join(ws, "base.json") @@ -49,18 +51,30 @@ func TestLoadSetsTheConfigFilePath(t *testing.T) { override := filepath.Join(ws, "override.json") writeFile(t, override, `{"image":"y"}`) + defaultPath := filepath.Join(ws, ".devcontainer", "devcontainer.json") res, err = LoadDevContainerConfigWithMounts(ws, "", override, MountOptions{}) if err != nil { t.Fatal(err) } + if res.Config.ConfigFilePath != defaultPath || res.HostSub.ConfigFilePath != defaultPath { + t.Errorf("override only: ConfigFilePath = %q, HostSub.ConfigFilePath = %q, want %q", res.Config.ConfigFilePath, res.HostSub.ConfigFilePath, defaultPath) + } + res, err = LoadDevContainerConfigWithMounts("", "", override, MountOptions{}) + if err != nil { + t.Fatal(err) + } if res.Config.ConfigFilePath != override || res.HostSub.ConfigFilePath != override { - t.Errorf("override only: ConfigFilePath = %q, HostSub.ConfigFilePath = %q, want %q", res.Config.ConfigFilePath, res.HostSub.ConfigFilePath, override) + t.Errorf("override without a workspace: ConfigFilePath = %q, HostSub.ConfigFilePath = %q, want %q", res.Config.ConfigFilePath, res.HostSub.ConfigFilePath, override) } writeFile(t, override, `{"image":"y","containerEnv":{"A":"${localEnv}"}}`) _, err = LoadDevContainerConfigWithMounts(ws, "", override, MountOptions{}) + if err == nil || !strings.Contains(err.Error(), "in devcontainer.json") { + t.Errorf("substitution error = %v, want it to name the default devcontainer.json", err) + } + _, err = LoadDevContainerConfigWithMounts("", "", override, MountOptions{}) if err == nil || !strings.Contains(err.Error(), "in override.json") { - t.Errorf("substitution error = %v, want it to name override.json", err) + t.Errorf("substitution error without a workspace = %v, want it to name override.json", err) } } @@ -109,18 +123,6 @@ func TestComputeWorkspaceConfigWorksInTheGitRootsSubfolder(t *testing.T) { } } -// substituteHostString resolves the host variables of a string, and leaves a -// string whose substitution fails as written. -func TestSubstituteHostString(t *testing.T) { - w := &Workspace{RootFolderPath: "/home/me/proj/"} - if got := substituteHostString(w, "/w/${localWorkspaceFolderBasename}"); got != "/w/proj" { - t.Errorf("substituteHostString = %q, want /w/proj", got) - } - if got := substituteHostString(w, "/w/${localEnv}"); got != "/w/${localEnv}" { - t.Errorf("failed substitution = %q, want it unchanged", got) - } -} - // An empty COMPOSE_FILE is no COMPOSE_FILE, and a .env's COMPOSE_FILE comes // next (TS getDockerComposeFilePaths: `if (envComposeFile)`, then the .env file). func TestDockerComposeFilePathsFromTheDotEnv(t *testing.T) { diff --git a/internal/config/loader_test.go b/internal/config/loader_test.go index 5a1d2ad..c32235e 100644 --- a/internal/config/loader_test.go +++ b/internal/config/loader_test.go @@ -1,9 +1,13 @@ package config import ( + "errors" "os" "path/filepath" + "strings" "testing" + + coreerrors "github.com/devcontainers/cli/internal/errors" ) func TestFindConfigFile(t *testing.T) { @@ -433,3 +437,122 @@ func TestLoadMovesDeprecatedPropertiesAndKeepsRawConfig(t *testing.T) { t.Error("RawConfig keeps the deprecated extensions") } } + +// TS names a workspace's default config path (getDefaultDevContainerConfigPath) +// as configFilePath when the workspace has no devcontainer.json and +// --override-config is given, so relative paths in the override (a Dockerfile, +// a local Feature, ${localWorkspaceFolder}-relative files) resolve against +// /.devcontainer, not the override's folder. +func TestLoadDevContainerConfigOverrideWithoutWorkspaceConfig(t *testing.T) { + ws := t.TempDir() + override := filepath.Join(t.TempDir(), "override.json") + if err := os.WriteFile(override, []byte(`{"build":{"dockerfile":"Dockerfile"},"name":"${localWorkspaceFolderBasename}"}`), 0o644); err != nil { + t.Fatal(err) + } + result, err := LoadDevContainerConfig(ws, "", override) + if err != nil { + t.Fatal(err) + } + if want := filepath.Join(ws, ".devcontainer", "devcontainer.json"); result.Config.ConfigFilePath != want { + t.Errorf("ConfigFilePath = %q, want %q", result.Config.ConfigFilePath, want) + } + if result.HostSub.ConfigFilePath != result.Config.ConfigFilePath { + t.Errorf("substitution config file = %q, want %q", result.HostSub.ConfigFilePath, result.Config.ConfigFilePath) + } + if result.Config.Name != filepath.Base(ws) { + t.Errorf("name = %q, want the override's, substituted", result.Config.Name) + } +} + +// Without a workspace, TS's configuration is the override itself +// (configPath = overrideConfigFile): a devcontainer.json in the current +// directory is not discovered. +func TestLoadDevContainerConfigOverrideWithoutWorkspace(t *testing.T) { + cwd := t.TempDir() + if err := os.MkdirAll(filepath.Join(cwd, ".devcontainer"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(cwd, ".devcontainer", "devcontainer.json"), []byte(`{"image":"cwd","remoteUser":"cwd"}`), 0o644); err != nil { + t.Fatal(err) + } + t.Chdir(cwd) + override := filepath.Join(t.TempDir(), "override.json") + if err := os.WriteFile(override, []byte(`{"image":"override"}`), 0o644); err != nil { + t.Fatal(err) + } + result, err := LoadDevContainerConfig("", "", override) + if err != nil { + t.Fatal(err) + } + if result.Config.ConfigFilePath != override { + t.Errorf("ConfigFilePath = %q, want the override %q", result.Config.ConfigFilePath, override) + } + if result.Config.Image != "override" || result.Config.RemoteUser != "" { + t.Errorf("image, remoteUser = %q, %q; want the override's alone", result.Config.Image, result.Config.RemoteUser) + } +} + +// TS reads the override in place of the configuration, so a missing override +// is the configuration not found, named by the configuration's path: --config, +// the one found in the workspace, the workspace's default path, or without a +// workspace the override itself (configContainer.ts resolveWithLocalFolder). +func TestLoadDevContainerConfigMissingOverrideIsNotFound(t *testing.T) { + ws := t.TempDir() + withConfig := t.TempDir() + found := filepath.Join(withConfig, ".devcontainer", "devcontainer.json") + if err := os.MkdirAll(filepath.Dir(found), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(found, []byte(`{"image":"base"}`), 0o644); err != nil { + t.Fatal(err) + } + missing := filepath.Join(t.TempDir(), "missing.json") + for _, tc := range []struct { + name, workspace, config, want string + }{ + {"default path", ws, "", filepath.Join(ws, ".devcontainer", "devcontainer.json")}, + {"found config", withConfig, "", found}, + {"--config", "", found, found}, + {"no workspace", "", "", missing}, + } { + t.Run(tc.name, func(t *testing.T) { + _, err := LoadDevContainerConfig(tc.workspace, tc.config, missing) + nf, ok := errors.AsType[*NotFoundError](err) + if !ok { + t.Fatalf("err = %v, want a NotFoundError", err) + } + if nf.Path != tc.want { + t.Errorf("path = %q, want %q", nf.Path, tc.want) + } + }) + } +} + +// Only a missing override is a configuration not found; one that cannot be +// read for another reason keeps its read error. +func TestLoadDevContainerConfigUnreadableOverride(t *testing.T) { + _, err := LoadDevContainerConfig(t.TempDir(), "", t.TempDir()) + if _, ok := errors.AsType[*NotFoundError](err); ok || err == nil || !strings.Contains(err.Error(), "read override config") { + t.Errorf("err = %v, want the override's read error", err) + } +} + +// A host variable without a name is TS's ContainerError (variableSubstitution.ts), +// its description the whole text: the loader adds no prefix of its own, so +// commands report it as TS does. +func TestLoadDevContainerConfigUnresolvableVariableIsAContainerError(t *testing.T) { + ws := t.TempDir() + cfg := filepath.Join(ws, ".devcontainer", "devcontainer.json") + if err := os.MkdirAll(filepath.Dir(cfg), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(cfg, []byte(`{"image":"img","containerEnv":{"A":"${localEnv}"}}`), 0o644); err != nil { + t.Fatal(err) + } + _, err := LoadDevContainerConfig(ws, "", "") + want := "'${localEnv}' in devcontainer.json can not be resolved because no environment variable name is given." + ce, ok := coreerrors.AsContainerError(err) + if !ok || err.Error() != want || ce.Description != want { + t.Errorf("err = %#v, want a ContainerError %q", err, want) + } +} diff --git a/internal/config/types.go b/internal/config/types.go index 21f2884..6f8decc 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -11,6 +11,11 @@ import ( type DevContainer struct { // Runtime — not serialized from JSON ConfigFilePath string `json:"configFilePath,omitempty"` + // workspaceMountSet records that the configuration has a workspaceMount + // property, even an empty or null one, which WorkspaceMount cannot tell + // from an absent one (TS getWorkspaceConfiguration's `'workspaceMount' in + // config`). + workspaceMountSet bool // --- Common properties (all variants) --- Name string `json:"name,omitempty"` diff --git a/internal/config/varsub.go b/internal/config/varsub.go index 4368fdc..992bbd8 100644 --- a/internal/config/varsub.go +++ b/internal/config/varsub.go @@ -11,6 +11,8 @@ import ( "slices" "strings" "unicode/utf16" + + coreerrors "github.com/devcontainers/cli/internal/errors" ) type SubstitutionPhase uint8 @@ -216,7 +218,8 @@ func resolveEnvTag(isWin bool, env map[string]string, args []string, tag, config if configFilePath != "" { location = " in " + path.Base(configFilePath) } - return "", false, fmt.Errorf("'${%s}'%s can not be resolved because no environment variable name is given.", tag, location) + // TS throws a ContainerError with this description (variableSubstitution.ts). + return "", false, &coreerrors.ContainerError{Description: fmt.Sprintf("'${%s}'%s can not be resolved because no environment variable name is given.", tag, location)} } name := args[0] if isWin { diff --git a/internal/config/worktree_test.go b/internal/config/worktree_test.go index ef56662..92a2a0c 100644 --- a/internal/config/worktree_test.go +++ b/internal/config/worktree_test.go @@ -15,6 +15,7 @@ import ( "os" "path/filepath" "runtime" + "slices" "testing" ) @@ -197,69 +198,122 @@ func TestLoadDevContainerConfigWithMountsWorktree(t *testing.T) { } } -// TestWorktreeCommonDirCustomWorkspaceMount ports upstream #1261: when the config -// sets a custom workspaceMount/workspaceFolder, the common-dir mount is still -// added, with its target resolved relative to the custom mount target. +// TestWorktreeCommonDirCustomWorkspaceMount holds TS getWorkspaceConfiguration +// (utils.ts): the common-dir mount is computed with the workspace mount, so a +// config that sets both workspaceFolder and workspaceMount gets none, and one +// that sets only workspaceMount gets it at the computed container mount folder, +// whatever the custom mount's type or target. func TestWorktreeCommonDirCustomWorkspaceMount(t *testing.T) { if runtime.GOOS != "linux" { t.Skip("oracle pins the linux consistency behavior (empty suffix)") } - t.Run("common dir target relative to custom mount", func(t *testing.T) { - base := t.TempDir() - wt := writeGitlink(t, base, "worktrees/feature", "gitdir: ../main/.git/worktrees/feature") - mainGit := filepath.Join(base, "worktrees", "main", ".git") + cases := []struct { + name, mount, folder string + wantFolder string + wantCommonDir bool + }{ + {"custom bind mount and folder", "type=bind,source=/host/wt,target=/workspace", "/workspace", "/workspace", false}, + {"custom volume mount and folder", "type=volume,source=v,target=/workspace", "/workspace", "/workspace", false}, + {"custom bind mount only", "type=bind,source=/host/wt,target=/workspace", "", "/workspaces/feature", true}, + {"custom volume mount only", "type=volume,source=v,target=/workspace", "", "/workspaces/feature", true}, + {"custom folder only", "", "/workspace", "/workspace", true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + base := t.TempDir() + wt := writeGitlink(t, base, "worktrees/feature", "gitdir: ../main/.git/worktrees/feature") + mainGit := filepath.Join(base, "worktrees", "main", ".git") - wc := computeWorkspaceConfig(&Workspace{RootFolderPath: wt}, &DevContainer{ - WorkspaceMount: "type=bind,source=/host/wt,target=/workspace", - WorkspaceFolder: "/workspace", - }, true, true) + wc := computeWorkspaceConfig(&Workspace{RootFolderPath: wt}, &DevContainer{ + WorkspaceMount: tc.mount, + WorkspaceFolder: tc.folder, + }, true, true) - // Custom mount/folder respected verbatim; common dir mounted, target - // resolved relative to /workspace → /main/.git. - if wc.WorkspaceMount != "type=bind,source=/host/wt,target=/workspace" { - t.Errorf("workspaceMount = %q", wc.WorkspaceMount) + wantMount := tc.mount + if wantMount == "" { + wantMount = "type=bind,source=" + wt + ",target=/workspaces/feature" + } + if wc.WorkspaceMount != wantMount { + t.Errorf("workspaceMount = %q, want %q", wc.WorkspaceMount, wantMount) + } + if wc.WorkspaceFolder != tc.wantFolder { + t.Errorf("workspaceFolder = %q, want %q", wc.WorkspaceFolder, tc.wantFolder) + } + var want []string + if tc.wantCommonDir { + want = []string{"type=bind,source=" + mainGit + ",target=/workspaces/main/.git"} + } + if !slices.Equal(wc.AdditionalMounts, want) { + t.Errorf("additionalMounts = %q, want %q", wc.AdditionalMounts, want) + } + }) + } +} + +// TestLoadKeepsAnEmptyWorkspaceMount holds TS getWorkspaceConfiguration and +// readDevContainerConfigFile (utils.ts, configContainer.ts), which test +// `'workspaceMount' in config`: an empty or null workspaceMount is set, so it +// replaces the computed mount and mounts no workspace, and with workspaceFolder +// also set the worktree common dir is not mounted either. +func TestLoadKeepsAnEmptyWorkspaceMount(t *testing.T) { + if runtime.GOOS != "linux" { + t.Skip("oracle pins the linux consistency behavior (empty suffix)") + } + cases := []struct { + name, config, wantFolder string + wantCommonDir bool + }{ + {"empty", `{"image":"alpine","workspaceMount":""}`, "/workspaces/feature", true}, + {"null", `{"image":"alpine","workspaceMount":null}`, "/workspaces/feature", true}, + {"empty with workspaceFolder", `{"image":"alpine","workspaceMount":"","workspaceFolder":"/w"}`, "/w", false}, + {"empty after substitution", `{"image":"alpine","workspaceMount":"${localEnv:DEVCONTAINER_CLI_TEST_UNSET}","workspaceFolder":"/w"}`, "/w", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + base := t.TempDir() + wt := writeGitlink(t, base, "worktrees/feature", "gitdir: ../main/.git/worktrees/feature") + if err := os.MkdirAll(filepath.Join(wt, ".devcontainer"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(wt, ".devcontainer", "devcontainer.json"), []byte(tc.config), 0o644); err != nil { + t.Fatal(err) + } + res, err := LoadDevContainerConfigWithMounts(wt, "", "", MountOptions{MountWorkspaceGitRoot: true, MountGitWorktreeCommonDir: true}) + if err != nil { + t.Fatalf("load: %v", err) + } + wc := res.WorkspaceConfig + if wc.WorkspaceMount != "" || !wc.NoWorkspaceMount { + t.Errorf("WorkspaceMount = %q, NoWorkspaceMount = %v, want empty and true", wc.WorkspaceMount, wc.NoWorkspaceMount) + } + if wc.WorkspaceFolder != tc.wantFolder { + t.Errorf("WorkspaceFolder = %q, want %q", wc.WorkspaceFolder, tc.wantFolder) + } + var want []string + if tc.wantCommonDir { + want = []string{"type=bind,source=" + filepath.Join(base, "worktrees", "main", ".git") + ",target=/workspaces/main/.git"} + } + if !slices.Equal(wc.AdditionalMounts, want) { + t.Errorf("AdditionalMounts = %q, want %q", wc.AdditionalMounts, want) + } + }) + } + + t.Run("absent", func(t *testing.T) { + dir := t.TempDir() + if err := os.MkdirAll(filepath.Join(dir, ".devcontainer"), 0o755); err != nil { + t.Fatal(err) } - if wc.WorkspaceFolder != "/workspace" { - t.Errorf("workspaceFolder = %q", wc.WorkspaceFolder) + if err := os.WriteFile(filepath.Join(dir, ".devcontainer", "devcontainer.json"), []byte(`{"image":"alpine"}`), 0o644); err != nil { + t.Fatal(err) } - want := "type=bind,source=" + mainGit + ",target=/main/.git" - if len(wc.AdditionalMounts) != 1 || wc.AdditionalMounts[0] != want { - t.Errorf("additionalMounts = %q, want [%q]", wc.AdditionalMounts, want) + res, err := LoadDevContainerConfig(dir, "", "") + if err != nil { + t.Fatalf("load: %v", err) } - }) - - t.Run("variables in the custom mount target are substituted", func(t *testing.T) { - base := t.TempDir() - wt := writeGitlink(t, base, "worktrees/feature", "gitdir: ../main/.git/worktrees/feature") - mainGit := filepath.Join(base, "worktrees", "main", ".git") - - // target uses ${localWorkspaceFolderBasename} → "feature". - wc := computeWorkspaceConfig(&Workspace{RootFolderPath: wt}, &DevContainer{ - WorkspaceMount: "type=bind,source=/host/wt,target=/src/${localWorkspaceFolderBasename}", - WorkspaceFolder: "/src/feature", - }, true, true) - - // common dir resolved relative to the substituted target /src/feature → - // /src/main/.git. - want := "type=bind,source=" + mainGit + ",target=/src/main/.git" - if len(wc.AdditionalMounts) != 1 || wc.AdditionalMounts[0] != want { - t.Errorf("additionalMounts = %q, want [%q]", wc.AdditionalMounts, want) + if wc := res.WorkspaceConfig; wc.WorkspaceMount == "" || wc.NoWorkspaceMount { + t.Errorf("WorkspaceMount = %q, NoWorkspaceMount = %v, want the computed mount", wc.WorkspaceMount, wc.NoWorkspaceMount) } }) } - -func TestMountTarget(t *testing.T) { - cases := map[string]string{ - "type=bind,source=/a,target=/b": "/b", - "type=bind,source=/a,target=/b,consistency=cached": "/b", - `type=bind,source="/a,x",target="/b,y"`: "/b,y", - "type=volume,source=vol,target=/data": "/data", - "type=bind,source=/a": "", - } - for spec, want := range cases { - if got := mountTarget(spec); got != want { - t.Errorf("mountTarget(%q) = %q, want %q", spec, got, want) - } - } -} diff --git a/internal/devtools/command_test.go b/internal/devtools/command_test.go index 3afcc5a..7e1459e 100644 --- a/internal/devtools/command_test.go +++ b/internal/devtools/command_test.go @@ -76,15 +76,38 @@ func assertMode(t *testing.T, path string, want os.FileMode) { } // gitRepo makes an empty repository in a new directory and enters it, with no -// user or system configuration to sign or hook its commits. +// user or system configuration to sign or hook its commits. The variables git +// exports to hooks and `rebase --exec` are cleared, so the tests never write +// to the repository they run in. func gitRepo(t *testing.T) { t.Helper() t.Chdir(t.TempDir()) + for _, k := range []string{"GIT_DIR", "GIT_WORK_TREE", "GIT_INDEX_FILE", "GIT_COMMON_DIR", "GIT_OBJECT_DIRECTORY", "GIT_ALTERNATE_OBJECT_DIRECTORIES", "GIT_PREFIX"} { + t.Setenv(k, "") + os.Unsetenv(k) + } t.Setenv("GIT_CONFIG_GLOBAL", os.DevNull) t.Setenv("GIT_CONFIG_NOSYSTEM", "1") git(t, "init", "-q") } +// A repository named by GIT_DIR, as `git rebase --exec` names the one it +// runs in, is left alone by gitRepo and the commits made after it. +func TestGitRepoLeavesTheOuterRepositoryAlone(t *testing.T) { + outer := t.TempDir() + t.Chdir(outer) + t.Setenv("GIT_CONFIG_GLOBAL", os.DevNull) + git(t, "init", "-q") + t.Setenv("GIT_DIR", filepath.Join(outer, ".git")) + t.Setenv("GIT_WORK_TREE", outer) + + gitRepo(t) + commit(t, "a.txt", 1_700_000_000) + if out, err := exec.Command("git", "-C", outer, "rev-list", "--all").Output(); err != nil || len(out) != 0 { + t.Errorf("outer repository commits = %q, err = %v, want none", out, err) + } +} + func git(t *testing.T, args ...string) string { t.Helper() cmd := exec.Command("git", args...) diff --git a/internal/docker/compose.go b/internal/docker/compose.go index fdf54c2..f565f55 100644 --- a/internal/docker/compose.go +++ b/internal/docker/compose.go @@ -42,10 +42,17 @@ func NewComposeClient(dockerPath, composePath string, env []string, logger log.L if dockerPath == "" { dockerPath = "docker" } - if out, err := osexec.Command(dockerPath, "compose", "version", "--short").Output(); err == nil { + // TS dockerComposeCLIConfig logs the command and the version it found. + runLine := "Run: " + dockerPath + " compose version --short" + start := logger.Start(runLine, log.LevelDebug) + out, err := osexec.Command(dockerPath, "compose", "version", "--short").Output() + logger.Stop(runLine, start, log.LevelDebug) + if err == nil { + version := strings.TrimSpace(string(out)) + logger.Write("Docker Compose version: "+version, log.LevelDebug) return &ComposeClient{ Command: []string{dockerPath, "compose"}, - Version: parseComposeVersion(strings.TrimSpace(string(out))), + Version: parseComposeVersion(version), Env: withBakeFSEntitlement(env, os.LookupEnv), Log: logger, }, nil @@ -76,13 +83,13 @@ func withBakeFSEntitlement(env []string, lookup func(string) (string, bool)) []s // Run executes a compose command and captures output (no live streaming). func (c *ComposeClient) Run(ctx context.Context, args ...string) (*ExecResult, error) { - return c.run(ctx, false, args...) + return c.run(ctx, log.LevelDebug, false, args...) } -// run executes a compose command. When stream is true and ProgressWriter is set, -// the child's output is also forwarded live through the runner. ctx cancels the -// subprocess. -func (c *ComposeClient) run(ctx context.Context, stream bool, args ...string) (*ExecResult, error) { +// run executes a compose command, its "Run:" lines logged at level. When +// stream is true and ProgressWriter is set, the child's output is also +// forwarded live through the runner. ctx cancels the subprocess. +func (c *ComposeClient) run(ctx context.Context, level log.Level, stream bool, args ...string) (*ExecResult, error) { fullArgs := append(c.Command[1:], args...) // Emit a "Run:" start/stop pair around the subprocess, like the TS CLI wraps @@ -90,7 +97,7 @@ func (c *ComposeClient) run(ctx context.Context, stream bool, args ...string) (* // subprocess, so they don't produce "Run:" events — a deliberate difference // from the TS CLI, which shells out to the docker CLI for everything.) runLine := fmt.Sprintf("Run: %s %s", c.Command[0], strings.Join(fullArgs, " ")) - startTS := c.Log.Start(runLine, log.LevelDebug) + startTS := c.Log.Start(runLine, level) var runner exec.Runner = exec.OSRunner{Env: c.Env} if c.Runner != nil { @@ -101,7 +108,7 @@ func (c *ComposeClient) run(ctx context.Context, stream bool, args ...string) (* live = c.ProgressWriter } stdout, stderr, exitCode, err := runner.Run(ctx, live, c.Command[0], fullArgs...) - c.Log.Stop(runLine, startTS, log.LevelDebug) + c.Log.Stop(runLine, startTS, level) if err != nil { return nil, fmt.Errorf("exec compose: %w", err) } @@ -122,7 +129,12 @@ func (c *ComposeClient) Config(ctx context.Context, composeFiles []string, envFi args := c.buildGlobalArgs(composeFiles, envFile) args = append(args, "--profile", "*", "config") - res, err := c.Run(ctx, args...) + // TS readDockerComposeConfig runs the command with an info-level log and + // writes its output to the log. + res, err := c.run(ctx, log.LevelInfo, false, args...) + if err == nil && res.ExitCode == 0 { + c.Log.Write(string(res.Stdout)) + } if err == nil && res.ExitCode != 0 { c.Log.Write(string(res.Stdout)+string(res.Stderr), log.LevelError) err = &CommandFailedError{Command: strings.Join(append(slices.Clone(c.Command), args...), " "), ExitCode: res.ExitCode} @@ -200,7 +212,7 @@ func (c *ComposeClient) Build(ctx context.Context, composeFiles []string, envFil } args = append(args, services...) - res, err := c.run(ctx, true, args...) + res, err := c.run(ctx, log.LevelDebug, true, args...) if err != nil { return err } @@ -223,7 +235,7 @@ func (c *ComposeClient) Up(ctx context.Context, composeFiles []string, envFile s } args = append(args, services...) - res, err := c.run(ctx, true, args...) + res, err := c.run(ctx, log.LevelDebug, true, args...) if err != nil { return err } diff --git a/internal/docker/compose_log_test.go b/internal/docker/compose_log_test.go new file mode 100644 index 0000000..b190a55 --- /dev/null +++ b/internal/docker/compose_log_test.go @@ -0,0 +1,79 @@ +package docker + +import ( + "bytes" + "encoding/json" + "os" + "path/filepath" + "slices" + "strings" + "testing" + + "github.com/devcontainers/cli/internal/log" +) + +// jsonLog is a --log-format json logger and the events it writes, decoded, +// each as " : ". +func jsonLog() (log.Logger, func() []string) { + var buf bytes.Buffer + lgr := log.New(log.Options{Format: "json", Writer: &buf}) + return lgr, func() []string { + var lines []string + for line := range strings.Lines(buf.String()) { + var e log.Event + if err := json.Unmarshal([]byte(line), &e); err != nil { + return []string{err.Error()} + } + lines = append(lines, e.Type+" "+e.Level.String()+": "+e.Text) + } + return lines + } +} + +// fakeDocker is an executable docker that answers `compose version --short` +// with version, or fails when version is empty. +func fakeDocker(t *testing.T, version string) string { + t.Helper() + path := filepath.Join(t.TempDir(), "docker") + script := "#!/bin/sh\nexit 1\n" + if version != "" { + script = "#!/bin/sh\n[ \"$*\" = 'compose version --short' ] && echo '" + version + "' && exit 0\nexit 1\n" + } + if err := os.WriteFile(path, []byte(script), 0o755); err != nil { + t.Fatal(err) + } + return path +} + +// Detecting Compose logs the command it runs and the version it found, as TS +// dockerComposeCLIConfig does (spec-node/dockerCompose.ts). +func TestNewComposeClientLogsTheComposeVersion(t *testing.T) { + docker := fakeDocker(t, "2.24.5") + lgr, events := jsonLog() + c, err := NewComposeClient(docker, "", nil, lgr) + if err != nil { + t.Fatal(err) + } + if !slices.Equal(c.Version, []int{2, 24, 5}) { + t.Errorf("Version = %v", c.Version) + } + run := "Run: " + docker + " compose version --short" + want := []string{"start debug: " + run, "stop debug: " + run, "text debug: Docker Compose version: 2.24.5"} + if got := events(); !slices.Equal(got, want) { + t.Errorf("log = %q, want %q", got, want) + } +} + +// When Compose v2 is not there, the command is still logged, but no version. +func TestNewComposeClientLogsAFailedVersionCommand(t *testing.T) { + docker := fakeDocker(t, "") + lgr, events := jsonLog() + if _, err := NewComposeClient(docker, "", nil, lgr); err == nil { + t.Fatal("NewComposeClient succeeded without Compose") + } + run := "Run: " + docker + " compose version --short" + want := []string{"start debug: " + run, "stop debug: " + run} + if got := events(); !slices.Equal(got, want) { + t.Errorf("log = %q, want %q", got, want) + } +} diff --git a/internal/docker/runner_seam_test.go b/internal/docker/runner_seam_test.go index 325c61f..0713e44 100644 --- a/internal/docker/runner_seam_test.go +++ b/internal/docker/runner_seam_test.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "io" + "slices" "strings" "testing" @@ -213,3 +214,34 @@ func TestComposeConfigFailureIsTheReferenceContainerError(t *testing.T) { t.Errorf("malformed output: err = %v", err) } } + +// compose config runs at info and its output is logged, as TS +// readDockerComposeConfig does. +func TestComposeConfigLogsTheCommandAtInfoAndItsOutput(t *testing.T) { + fr := &fakeRunner{stdout: []byte("services:\n app:\n image: alpine\n")} + lgr, events := jsonLog() + c := &ComposeClient{Command: []string{"docker", "compose"}, Log: lgr, Runner: fr} + if _, err := c.Config(t.Context(), []string{"/ws/a.yml"}, ""); err != nil { + t.Fatal(err) + } + run := "Run: docker compose -f /ws/a.yml --profile * config" + want := []string{"start info: " + run, "stop info: " + run, "text debug: services:\n app:\n image: alpine\n"} + if got := events(); !slices.Equal(got, want) { + t.Errorf("log = %q, want %q", got, want) + } +} + +// A failing compose config logs its output once, as the error. +func TestComposeConfigLogsAFailureOnce(t *testing.T) { + fr := &fakeRunner{stdout: []byte("out"), stderr: []byte("err"), code: 1} + lgr, events := jsonLog() + c := &ComposeClient{Command: []string{"docker", "compose"}, Log: lgr, Runner: fr} + if _, err := c.Config(t.Context(), []string{"/ws/a.yml"}, ""); err == nil { + t.Fatal("Config succeeded") + } + run := "Run: docker compose -f /ws/a.yml --profile * config" + want := []string{"start info: " + run, "stop info: " + run, "text error: outerr"} + if got := events(); !slices.Equal(got, want) { + t.Errorf("log = %q, want %q", got, want) + } +} diff --git a/internal/features/graph.go b/internal/features/graph.go index 443e311..9aba013 100644 --- a/internal/features/graph.go +++ b/internal/features/graph.go @@ -8,6 +8,20 @@ import ( "github.com/devcontainers/cli/internal/log" ) +// unprocessedFeatureError is the plain Error TS buildDependencyGraph throws +// for a Feature its processFeature returned nothing for; up and build report +// its message as their error result's message (errors.ReferenceError). +type unprocessedFeatureError struct{ msg string } + +func unprocessedFeature(format string, args ...any) error { + return &unprocessedFeatureError{msg: fmt.Sprintf(format, args...)} +} + +func (e *unprocessedFeatureError) Error() string { return e.msg } + +// ReferenceError marks the message as the reference's own. +func (e *unprocessedFeatureError) ReferenceError() {} + // ProcessFeature resolves a single feature (identified by node.UserFeatureID and // node.Options) into a Set. The returned Set must have its // SourceInfo populated and Features[0] carrying the feature metadata @@ -92,7 +106,7 @@ func resolveDependencyGraph( return nil, err } if processed == nil { - return nil, fmt.Errorf("ERR: Feature '%s' could not be processed. You may not have permission to access this Feature, or may not be logged in. If the issue persists, report this to the Feature author.", current.UserFeatureID) + return nil, unprocessedFeature("ERR: Feature '%s' could not be processed. You may not have permission to access this Feature, or may not be logged in. If the issue persists, report this to the Feature author.", current.UserFeatureID) } current.Set = processed @@ -140,7 +154,7 @@ func resolveDependencyGraph( return nil, err } if softSet == nil { - return nil, fmt.Errorf("installsAfter dependency '%s' of Feature '%s' could not be processed.", iaID, current.UserFeatureID) + return nil, unprocessedFeature("installsAfter dependency '%s' of Feature '%s' could not be processed.", iaID, current.UserFeatureID) } dep.Set = softSet if len(softSet.Features) > 0 { @@ -186,7 +200,7 @@ func applyOverrideFeatureInstallOrder( return err } if processed == nil { - return fmt.Errorf("Feature '%s' in 'overrideFeatureInstallOrder' could not be processed.", overrideID) + return unprocessedFeature("Feature '%s' in 'overrideFeatureInstallOrder' could not be processed.", overrideID) } override.Set = processed if len(processed.Features) > 0 { diff --git a/internal/features/jsobject.go b/internal/features/jsobject.go new file mode 100644 index 0000000..f663b07 --- /dev/null +++ b/internal/features/jsobject.go @@ -0,0 +1,200 @@ +package features + +import ( + "bytes" + "cmp" + "encoding/json" + "fmt" + "io" + "slices" + "strconv" + "strings" + "unicode/utf16" +) + +// jsObject is a JavaScript object as JSON.parse builds one: a property set +// again keeps its first position and takes the last value, and JSON.stringify +// writes the integer-index properties first, ascending, then the others in +// the order they were added. Its values are jsObject, []any, string, float64, +// bool or nil. +type jsObject struct { + keys []string + vals map[string]any +} + +func newJSObject() *jsObject { return &jsObject{vals: map[string]any{}} } + +// set is obj[k] = v. +func (o *jsObject) set(k string, v any) { + if _, ok := o.vals[k]; !ok { + o.keys = append(o.keys, k) + } + o.vals[k] = v +} + +func (o *jsObject) get(k string) (any, bool) { + v, ok := o.vals[k] + return v, ok +} + +// remove is delete obj[k]. +func (o *jsObject) remove(k string) { + if _, ok := o.vals[k]; ok { + delete(o.vals, k) + o.keys = slices.DeleteFunc(o.keys, func(s string) bool { return s == k }) + } +} + +// spread is {...o, ...src} for an object src: src's properties set on a copy of o. +func (o *jsObject) spread(src *jsObject) *jsObject { + out := &jsObject{keys: slices.Clone(o.keys), vals: make(map[string]any, len(o.vals))} + for k, v := range o.vals { + out.vals[k] = v + } + for _, k := range src.keys { + out.set(k, src.vals[k]) + } + return out +} + +// isArrayIndex reports whether k is an array index (a canonical integer +// below 2^32-1), which JS orders first. +func isArrayIndex(k string) bool { + n, err := strconv.ParseUint(k, 10, 32) + return err == nil && n != 1<<32-1 && strconv.FormatUint(n, 10) == k +} + +// order is the order JSON.stringify writes o's properties in. +func (o *jsObject) order() []string { + var idx, rest []string + for _, k := range o.keys { + if isArrayIndex(k) { + idx = append(idx, k) + } else { + rest = append(rest, k) + } + } + // Array indexes are canonical: shorter is smaller, equal lengths compare + // as strings. + slices.SortFunc(idx, func(a, b string) int { + return cmp.Or(cmp.Compare(len(a), len(b)), strings.Compare(a, b)) + }) + return append(idx, rest...) +} + +func (o *jsObject) MarshalJSON() ([]byte, error) { + var buf bytes.Buffer + buf.WriteByte('{') + for i, k := range o.order() { + if i > 0 { + buf.WriteByte(',') + } + key, err := json.Marshal(k) + if err != nil { + return nil, err + } + val, err := json.Marshal(o.vals[k]) + if err != nil { + return nil, err + } + buf.Write(key) + buf.WriteByte(':') + buf.Write(val) + } + buf.WriteByte('}') + return buf.Bytes(), nil +} + +// parseJS is JSON.parse(data): data's value with its objects as jsObject. +func parseJS(data []byte) (any, error) { + dec := json.NewDecoder(bytes.NewReader(data)) + v, err := parseJSValue(dec) + if err != nil { + return nil, err + } + if _, err := dec.Token(); err != io.EOF { + return nil, fmt.Errorf("parse JSON: data after the value") + } + return v, nil +} + +func parseJSValue(dec *json.Decoder) (any, error) { + tok, err := dec.Token() + if err != nil { + return nil, err + } + switch tok { + case json.Delim('{'): + obj := newJSObject() + for dec.More() { + k, err := dec.Token() + if err != nil { + return nil, err + } + v, err := parseJSValue(dec) + if err != nil { + return nil, err + } + obj.set(k.(string), v) + } + _, err := dec.Token() + return obj, err + case json.Delim('['): + arr := []any{} + for dec.More() { + v, err := parseJSValue(dec) + if err != nil { + return nil, err + } + arr = append(arr, v) + } + _, err := dec.Token() + return arr, err + } + return tok, nil +} + +// toJS is v as JSON.parse(JSON.stringify(v)) gives it back. +func toJS(v any) (any, error) { + data, err := json.Marshal(v) + if err != nil { + return nil, err + } + return parseJS(data) +} + +// jsTruthy is JavaScript's truthiness of a parsed JSON value. +func jsTruthy(v any) bool { + switch x := v.(type) { + case nil: + return false + case bool: + return x + case string: + return x != "" + case float64: + return x != 0 + } + return true +} + +// spreadSource is the own enumerable properties {...v} copies from v: an +// object's, an array's or a string's (by index); none for other values. +func spreadSource(v any) *jsObject { + out := newJSObject() + switch x := v.(type) { + case *jsObject: + return x + case []any: + for i, e := range x { + out.set(strconv.Itoa(i), e) + } + case string: + // A string spreads into its UTF-16 code units; half of a surrogate + // pair, which a Go string cannot hold, becomes U+FFFD. + for i, u := range utf16.Encode([]rune(x)) { + out.set(strconv.Itoa(i), string(utf16.Decode([]uint16{u}))) + } + } + return out +} diff --git a/internal/features/jsobject_test.go b/internal/features/jsobject_test.go new file mode 100644 index 0000000..b4b4c16 --- /dev/null +++ b/internal/features/jsobject_test.go @@ -0,0 +1,61 @@ +package features + +import ( + "encoding/json" + "testing" +) + +// JSON.parse then JSON.stringify: a repeated property keeps its first +// position and its last value, array-index properties come first, ascending +// (4294967295 and above, "01" and "-1" are not indexes), the rest in insertion order. +func TestJSObjectRoundTripsAsJavaScript(t *testing.T) { + src := `{"b":1,"4294967295":2,"10":3,"01":4,"2":5,"b":6,"-1":7,"4294967296":8,"9":9,"4294967294":10,"a":[{"z":1,"y":[]}],"n":1.50,"e":1e21}` + v, err := parseJS([]byte(src)) + if err != nil { + t.Fatal(err) + } + got, err := json.Marshal(v) + want := `{"2":5,"9":9,"10":3,"4294967294":10,"b":6,"4294967295":2,"01":4,"-1":7,"4294967296":8,"a":[{"z":1,"y":[]}],"n":1.5,"e":1e+21}` + if err != nil || string(got) != want { + t.Errorf("got %s, %v\nwant %s", got, err, want) + } + for _, bad := range []string{`{"a":}`, `{"a":1} 2`, ``, `[1,`} { + if _, err := parseJS([]byte(bad)); err == nil { + t.Errorf("parseJS(%q): no error", bad) + } + } +} + +// A manifest is written as the registry served it, or as the fallback when +// its body does not parse. +func TestManifestJSON(t *testing.T) { + got, _ := json.Marshal(ManifestJSON([]byte(`{"schemaVersion":2,"config":{"mediaType":"m","digest":"d"}}`), nil)) + if string(got) != `{"schemaVersion":2,"config":{"mediaType":"m","digest":"d"}}` { + t.Errorf("manifest = %s", got) + } + if got := ManifestJSON(nil, "fallback"); got != "fallback" { + t.Errorf("unparsable body: %v, want the fallback", got) + } +} + +// Metadata that is not an object spreads as TS spreads it: an array by index, +// a number not at all; none at all is an empty object. +func TestMetadataObject(t *testing.T) { + for _, tc := range []struct{ data, want string }{ + {``, `{}`}, + {`[5,6]`, `{"0":5,"1":6}`}, + {`3`, `{}`}, + {`{"a":1}`, `{"a":1}`}, + } { + obj, err := metadataObject(json.RawMessage(tc.data)) + if err != nil { + t.Fatalf("%q: %v", tc.data, err) + } + if got, _ := json.Marshal(obj); string(got) != tc.want { + t.Errorf("%q: %s, want %s", tc.data, got, tc.want) + } + } + if _, err := metadataObject(json.RawMessage(`{`)); err == nil { + t.Error("broken metadata: no error") + } +} diff --git a/internal/features/lockfile.go b/internal/features/lockfile.go index e13673a..39c3972 100644 --- a/internal/features/lockfile.go +++ b/internal/features/lockfile.go @@ -350,7 +350,9 @@ type WriteOptions struct { // does: it creates the file when there is none and rewrites it when its // content would change. With opts.Frozen set, it fails instead of writing when // the lockfile is missing or would change. A lockfile that exists but cannot be -// read is an error, as writeLockfile rethrows all but ENOENT. +// read is an error, as writeLockfile rethrows all but ENOENT. A new lockfile +// gets Node's fs.writeFile mode, 0666 less the umask; a rewritten one keeps +// its mode. func WriteLockfile(configPath string, lockfile *Lockfile, opts WriteOptions) error { path := LockfilePath(configPath) @@ -384,7 +386,7 @@ func WriteLockfile(configPath string, lockfile *Lockfile, opts WriteOptions) err return nil } - return os.WriteFile(path, newData, 0644) + return os.WriteFile(path, newData, 0o666) } // LockfilePath returns the lockfile path for a given config path. diff --git a/internal/features/lockfile_test.go b/internal/features/lockfile_test.go index 51ffb5e..5bf6348 100644 --- a/internal/features/lockfile_test.go +++ b/internal/features/lockfile_test.go @@ -4,6 +4,7 @@ import ( "encoding/json" "os" "path/filepath" + "syscall" "testing" ) @@ -230,3 +231,40 @@ func indexOf(s, substr string) int { } return -1 } + +// A new lockfile gets Node's default file mode, 0666 less the umask, as TS +// writeLockfile's fs.writeFile gives it; rewriting one keeps its mode. +func TestWriteLockfileMode(t *testing.T) { + old := syscall.Umask(0o002) + t.Cleanup(func() { syscall.Umask(old) }) + + configPath := filepath.Join(t.TempDir(), "devcontainer.json") + lf := &Lockfile{Features: map[string]LockfileEntry{"ghcr.io/o/f:1": {Version: "1.0.0"}}} + if err := WriteLockfile(configPath, lf, WriteOptions{}); err != nil { + t.Fatal(err) + } + path := LockfilePath(configPath) + if got := fileMode(t, path); got != 0o664 { + t.Fatalf("new lockfile mode = %v, want 0664", got) + } + + if err := os.Chmod(path, 0o600); err != nil { + t.Fatal(err) + } + lf.Features["ghcr.io/o/f:1"] = LockfileEntry{Version: "1.0.1"} + if err := WriteLockfile(configPath, lf, WriteOptions{}); err != nil { + t.Fatal(err) + } + if got := fileMode(t, path); got != 0o600 { + t.Fatalf("rewritten lockfile mode = %v, want its 0600 kept", got) + } +} + +func fileMode(t *testing.T, path string) os.FileMode { + t.Helper() + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + return info.Mode().Perm() +} diff --git a/internal/features/resolve.go b/internal/features/resolve.go index a8036a6..e8f3635 100644 --- a/internal/features/resolve.go +++ b/internal/features/resolve.go @@ -2,6 +2,7 @@ package features import ( "fmt" + "path/filepath" "strings" ) @@ -22,7 +23,9 @@ func ClassifyID(id string) SourceType { if strings.HasPrefix(id, "http://") || strings.HasPrefix(id, "https://") { return SourceDirectTarball } - if strings.HasPrefix(id, "./") || strings.HasPrefix(id, "../") { + // TS getFeatureIdType: "./", "../" and "/" all mark a Feature on disk; an + // absolute one is then rejected by LocalFeaturePath. + if strings.HasPrefix(id, "./") || strings.HasPrefix(id, "../") || strings.HasPrefix(id, "/") { return SourceLocalPath } // If it contains a dot in the first segment (before first /), it's a domain → OCI @@ -40,6 +43,48 @@ func ClassifyID(id string) SourceType { return SourceLegacyShorthand } +// InvalidLocalFeatureError is a local Feature reference TS +// processFeatureIdentifier rejects: it logs Msg as an error and returns no +// Feature set, so the dependency graph fails the Feature as one that could not +// be processed. +type InvalidLocalFeatureError struct{ Msg string } + +func (e *InvalidLocalFeatureError) Error() string { return e.Msg } + +// LocalFeaturePath resolves a local Feature reference (./x, ../x, /x) against +// configDir, the folder of the configuration that lists it, as TS +// processFeatureIdentifier does (containerFeaturesConfiguration.ts): an +// absolute reference is refused, so is one without a configuration, and the +// resolved folder must lie under workspaceRoot's .devcontainer folder - TS +// refuses any path relative to that folder that contains "..". An empty or +// relative workspaceRoot is resolved against the working directory, as TS +// resolves its process.cwd() fallback. +func LocalFeaturePath(id, configDir, workspaceRoot string) (string, error) { + if filepath.IsAbs(id) { + return "", &InvalidLocalFeatureError{Msg: "An Absolute path to a local feature is not allowed."} + } + if configDir == "" { + return "", &InvalidLocalFeatureError{Msg: "A local feature requires a configuration path."} + } + featureFolderPath := filepath.Join(configDir, id) + parent, err := filepath.Abs(filepath.Join(workspaceRoot, ".devcontainer")) + if err != nil { + return "", err + } + child, err := filepath.Abs(featureFolderPath) + if err != nil { + return "", err + } + rel, err := filepath.Rel(parent, child) + if err != nil { + return "", err + } + if strings.Contains(rel, "..") { + return "", &InvalidLocalFeatureError{Msg: "Local file path parse error. Resolved path must be a child of the .devcontainer/ folder. Parsed: " + featureFolderPath} + } + return featureFolderPath, nil +} + // versionBackwardComp is the version tag pinned for auto-mapped legacy // features (TS getBackwardCompatibleFeatureId), so they resolve to a known // major instead of `latest`. diff --git a/internal/features/resolve_test.go b/internal/features/resolve_test.go index 6dcf264..b276fd9 100644 --- a/internal/features/resolve_test.go +++ b/internal/features/resolve_test.go @@ -1,6 +1,8 @@ package features import ( + "errors" + "path/filepath" "testing" ) @@ -17,6 +19,7 @@ func TestClassifyFeatureID(t *testing.T) { {"http://example.com/feature.tgz", SourceDirectTarball}, {"./local-feature", SourceLocalPath}, {"../sibling-feature", SourceLocalPath}, + {"/abs/feature", SourceLocalPath}, {"go", SourceLegacyShorthand}, {"node", SourceLegacyShorthand}, {"my-feature", SourceLegacyShorthand}, @@ -145,3 +148,60 @@ func TestUserFeaturesToArray_Nil(t *testing.T) { t.Errorf("expected nil, got %v", arr) } } + +// A local Feature resolves next to its configuration, and only inside the +// workspace's .devcontainer folder, as TS processFeatureIdentifier +// (containerFeaturesConfiguration.ts) requires; each refusal is the error TS +// logs. +func TestLocalFeaturePath(t *testing.T) { + ws := t.TempDir() + dc := filepath.Join(ws, ".devcontainer") + outside := "Local file path parse error. Resolved path must be a child of the .devcontainer/ folder. Parsed: " + for _, tc := range []struct { + name, id, configDir, workspaceRoot string + want, wantErr string + }{ + {name: "inside .devcontainer", id: "./feat", configDir: dc, workspaceRoot: ws, want: filepath.Join(dc, "feat")}, + {name: "nested", id: "./a/../b/feat", configDir: dc, workspaceRoot: ws, want: filepath.Join(dc, "b", "feat")}, + {name: "the .devcontainer folder itself", id: "./", configDir: dc, workspaceRoot: ws, want: dc}, + {name: "parent folder", id: "../feat", configDir: dc, workspaceRoot: ws, wantErr: outside + filepath.Join(ws, "feat")}, + {name: "configuration at the workspace root", id: "./feat", configDir: ws, workspaceRoot: ws, wantErr: outside + filepath.Join(ws, "feat")}, + {name: "configuration of another workspace", id: "./feat", configDir: dc, workspaceRoot: filepath.Join(ws, "other"), wantErr: outside + filepath.Join(dc, "feat")}, + // TS looks for ".." anywhere in the relative path, a folder name included. + {name: "dots in a folder name", id: "./a..b", configDir: dc, workspaceRoot: ws, wantErr: outside + filepath.Join(dc, "a..b")}, + {name: "absolute", id: filepath.Join(dc, "feat"), configDir: dc, workspaceRoot: ws, wantErr: "An Absolute path to a local feature is not allowed."}, + {name: "absolute without a configuration", id: "/feat", wantErr: "An Absolute path to a local feature is not allowed."}, + {name: "no configuration", id: "./feat", workspaceRoot: ws, wantErr: "A local feature requires a configuration path."}, + } { + t.Run(tc.name, func(t *testing.T) { + got, err := LocalFeaturePath(tc.id, tc.configDir, tc.workspaceRoot) + if tc.wantErr != "" { + invalid, ok := errors.AsType[*InvalidLocalFeatureError](err) + if !ok || invalid.Error() != tc.wantErr || got != "" { + t.Fatalf("LocalFeaturePath = %q, %v; want the error %q", got, err, tc.wantErr) + } + return + } + if err != nil || got != tc.want { + t.Fatalf("LocalFeaturePath = %q, %v; want %q", got, err, tc.want) + } + }) + } +} + +// Without a workspace folder the workspace root is the working directory, as +// TS's params.cwd falls back to process.cwd(). +func TestLocalFeaturePathDefaultsToTheWorkingDirectory(t *testing.T) { + ws := t.TempDir() + t.Chdir(ws) + dc := filepath.Join(ws, ".devcontainer") + if got, err := LocalFeaturePath("./feat", dc, ""); err != nil || got != filepath.Join(dc, "feat") { + t.Fatalf("LocalFeaturePath = %q, %v; want the Feature in the working directory's .devcontainer", got, err) + } + if _, err := LocalFeaturePath("./feat", filepath.Join(ws, "sub", ".devcontainer"), ""); err == nil { + t.Fatal("a configuration outside the working directory's .devcontainer resolved") + } + if got, err := LocalFeaturePath("./feat", dc, "."); err != nil || got != filepath.Join(dc, "feat") { + t.Fatalf("relative workspace root: LocalFeaturePath = %q, %v", got, err) + } +} diff --git a/internal/features/types.go b/internal/features/types.go index 7dcbec2..a17db5b 100644 --- a/internal/features/types.go +++ b/internal/features/types.go @@ -3,18 +3,21 @@ package features import ( "encoding/json" + "fmt" "maps" "slices" + "strings" "github.com/devcontainers/cli/internal/jsonc" ) // Set represents a resolved feature with its source and metadata. type Set struct { - SourceInfo SourceInfo `json:"sourceInformation"` - Features []Feature `json:"features"` - ComputedDigest string `json:"computedDigest,omitempty"` - InternalVersion string `json:"internalVersion,omitempty"` + SourceInfo SourceInfo `json:"sourceInformation"` + Features []Feature `json:"features"` + // TS applyFeatureConfigToFeature sets internalVersion, then computedDigest. + InternalVersion string `json:"internalVersion,omitempty"` + ComputedDigest string `json:"computedDigest,omitempty"` } // Feature represents a single feature within a Set. @@ -49,6 +52,186 @@ type Feature struct { PostStartCommand any `json:"postStartCommand,omitempty"` PostAttachCommand any `json:"postAttachCommand,omitempty"` Customizations map[string]any `json:"customizations,omitempty"` + // Metadata is the devcontainer-feature.json UnmarshalJSON decoded the + // Feature from, as standard JSON; File, when set, is the Feature's + // devcontainer-feature.json where it differs from Metadata (an OCI + // Feature's metadata comes from its manifest's annotation, the file from + // its layer). + Metadata json.RawMessage `json:"-"` + File json.RawMessage `json:"-"` + // SourceKeys, when set, are the properties TS processFeatureIdentifier + // gives the Feature before anything else (LocalFeatureKeys or + // OCIFeatureKeys), and the Feature is written as TS builds it (see + // MarshalJSON). + SourceKeys []string `json:"-"` +} + +// The properties TS processFeatureIdentifier starts a Feature with: a local +// or tarball Feature's {id, name, value, included}, an OCI one's +// {id, included, value} (tryGetOCIFeatureSet). +var ( + LocalFeatureKeys = []string{"id", "name", "value", "included"} + OCIFeatureKeys = []string{"id", "included", "value"} +) + +// MarshalJSON writes a Feature with SourceKeys as TS builds it for +// featuresConfiguration, and one without in the struct's order. TS starts +// from the SourceKeys properties, spreads the metadata over them when it +// reads the dependencies (buildDependencyGraph), adds cachePath and +// consecutiveId (fetchFeatures), spreads the result over the +// devcontainer-feature.json (applyFeatureConfigToFeature) and moves the +// deprecated extensions and settings into customizations +// (updateFromOldProperties). The +// metadata's own properties, values and key order are kept, those Go has no +// field for included. +func (f Feature) MarshalJSON() ([]byte, error) { + type plain Feature + if f.SourceKeys == nil { + return json.Marshal(plain(f)) + } + feature := newJSObject() + for _, k := range f.SourceKeys { + var v any + switch k { + case "id": + v = f.ID + case "name": + v = f.Name + case "value": + v = f.Value + case "included": + v = f.Included + } + js, err := toJS(v) + if err != nil { + return nil, err + } + feature.set(k, js) + } + meta, err := metadataObject(f.Metadata) + if err != nil { + return nil, err + } + feature = feature.spread(meta) + if f.CachePath != "" { + feature.set("cachePath", f.CachePath) + } + if f.ConsecutiveId != "" { + feature.set("consecutiveId", f.ConsecutiveId) + } + file := meta + if f.File != nil { + if file, err = metadataObject(f.File); err != nil { + return nil, err + } + } + out, err := updateFeatureFromOldProperties(file.spread(feature)) + if err != nil { + return nil, err + } + return out.MarshalJSON() +} + +// metadataObject is the metadata's object, empty when there is none (TS +// spreads only a metadata it read). +func metadataObject(data json.RawMessage) (*jsObject, error) { + if len(data) == 0 { + return newJSObject(), nil + } + v, err := parseJS(data) + if err != nil { + return nil, err + } + if obj, ok := v.(*jsObject); ok { + return obj, nil + } + return spreadSource(v), nil +} + +// updateFeatureFromOldProperties is TS updateFromOldProperties +// (containerFeaturesConfiguration.ts) on a Feature: its extensions and +// settings move into customizations.vscode. +func updateFeatureFromOldProperties(original *jsObject) (*jsObject, error) { + extensions, _ := original.get("extensions") + settings, _ := original.get("settings") + if !jsTruthy(extensions) && !jsTruthy(settings) { + return original, nil + } + cp := original.spread(newJSObject()) + customizations := orNewObject(cp, "customizations") + vscode := orNewObject(customizations, "vscode") + if jsTruthy(extensions) { + base, _ := vscode.get("extensions") + joined, err := jsConcat(base, extensions) + if err != nil { + return nil, err + } + vscode.set("extensions", joined) + cp.remove("extensions") + } + if jsTruthy(settings) { + vsSettings, _ := vscode.get("settings") + vscode.set("settings", spreadSource(settings).spread(spreadSource(vsSettings))) + cp.remove("settings") + } + return cp, nil +} + +// orNewObject is obj[k] || (obj[k] = {}), for an object or a new one. +func orNewObject(obj *jsObject, k string) *jsObject { + if v, _ := obj.get(k); jsTruthy(v) { + if o, ok := v.(*jsObject); ok { + return o + } + } + o := newJSObject() + obj.set(k, o) + return o +} + +// jsConcat is (base || []).concat(v): an array's elements are appended, any +// other value is; a string base concatenates as a string does. +func jsConcat(base, v any) (any, error) { + if !jsTruthy(base) { + base = []any{} + } + switch b := base.(type) { + case []any: + out := append([]any{}, b...) + if arr, ok := v.([]any); ok { + return append(out, arr...), nil + } + return append(out, v), nil + case string: + s, err := jsString(v) + return b + s, err + } + return nil, fmt.Errorf("TypeError: (vscode.extensions || []).concat is not a function") +} + +// jsString is String(v) for the values a string concat appends. +func jsString(v any) (string, error) { + switch x := v.(type) { + case string: + return x, nil + case bool, float64: + data, err := json.Marshal(x) + return string(data), err + case []any: + parts := make([]string, len(x)) + for i, e := range x { + if e == nil { + continue + } + s, err := jsString(e) + if err != nil { + return "", err + } + parts[i] = s + } + return strings.Join(parts, ","), nil + } + return "[object Object]", nil } // UnmarshalJSON decodes the Feature and records the order of its dependsOn @@ -59,6 +242,7 @@ func (f *Feature) UnmarshalJSON(data []byte) error { return err } f.DependsOnOrder = jsonc.MemberNames(data, "dependsOn") + f.Metadata = slices.Clone(data) return nil } @@ -90,13 +274,17 @@ type SourceInfo interface { } // OCISource is a feature from an OCI registry. +// Its properties are in the order of TS OCISourceInformation as +// tryGetOCIFeatureSet builds it. type OCISource struct { - Type string `json:"type"` - ManifestDigest string `json:"manifestDigest,omitempty"` - UserID string `json:"userFeatureId"` - UserFeatureIdWithoutVersion string `json:"userFeatureIdWithoutVersion,omitempty"` - FeatureRef any `json:"featureRef,omitempty"` - Manifest any `json:"manifest,omitempty"` + Type string `json:"type"` + // Manifest is the manifest as the registry served it (TS manifestObj, its + // JSON.parse): see ManifestJSON. + Manifest any `json:"manifest,omitempty"` + ManifestDigest string `json:"manifestDigest,omitempty"` + FeatureRef *FeatureRef `json:"featureRef,omitempty"` + UserID string `json:"userFeatureId"` + UserFeatureIdWithoutVersion string `json:"userFeatureIdWithoutVersion,omitempty"` // Internal fields not serialized to JSON (used for dependency resolution) Registry string `json:"-"` Namespace string `json:"-"` @@ -105,6 +293,30 @@ type OCISource struct { Tag string `json:"-"` } +// FeatureRef is TS OCIRef as getRef builds it, its properties in that order. +type FeatureRef struct { + Scheme string `json:"scheme"` + ID string `json:"id"` + Owner string `json:"owner"` + Namespace string `json:"namespace"` + Registry string `json:"registry"` + Resource string `json:"resource"` + Path string `json:"path"` + Version string `json:"version"` + Tag string `json:"tag,omitempty"` + Digest string `json:"digest,omitempty"` +} + +// ManifestJSON is a manifest as TS keeps it in OCISourceInformation: +// JSON.parse of the body the registry served, properties in its order. A +// body that does not parse is written as fallback is. +func ManifestJSON(body []byte, fallback any) any { + if v, err := parseJS(body); err == nil { + return v + } + return fallback +} + func (s *OCISource) SourceType() string { return "oci" } func (s *OCISource) UserFeatureID() string { return s.UserID } diff --git a/internal/features/types_order_test.go b/internal/features/types_order_test.go index d94efd3..1c913ce 100644 --- a/internal/features/types_order_test.go +++ b/internal/features/types_order_test.go @@ -23,3 +23,82 @@ func TestDependsOnKeysKeepsTheMetadataOrder(t *testing.T) { t.Errorf("DependsOnKeys = %v, want %v", got, want) } } + +// A Feature with SourceKeys is written as TS builds it: the metadata spread +// over the source's properties, cachePath and consecutiveId added, that +// spread over the devcontainer-feature.json, deprecated extensions and +// settings moved. The metadata's properties are its own, those Go has no +// field for and an empty options included; without SourceKeys the Feature +// keeps the struct's order. +func TestFeatureMarshalJSONBuildsTheFeatureAsTS(t *testing.T) { + var f Feature + meta := `{"version":"1.0.0","containerEnv":{"K":"v"},"id":"x","options":{},"keywords":["k"],"version":"1.0.1","10":1,"2":2}` + if err := json.Unmarshal([]byte(meta), &f); err != nil { + t.Fatal(err) + } + f.Name, f.Value, f.Included, f.CachePath, f.ConsecutiveId = "./x", map[string]any{"o": "v"}, true, "/c", "x_0" + f.CapAdd = []string{"SET_IN_CODE"} // not the metadata's: TS has no such property + for _, tc := range []struct { + name string + source []string + file string + want string + }{ + {"local", LocalFeatureKeys, "", `{"2":2,"10":1,"version":"1.0.1","containerEnv":{"K":"v"},"id":"x","options":{},"keywords":["k"],"name":"./x","value":{"o":"v"},"included":true,"cachePath":"/c","consecutiveId":"x_0"}`}, + {"oci", OCIFeatureKeys, "", `{"2":2,"10":1,"version":"1.0.1","containerEnv":{"K":"v"},"id":"x","options":{},"keywords":["k"],"included":true,"value":{"o":"v"},"cachePath":"/c","consecutiveId":"x_0"}`}, + // The file's properties come first, then the Feature's in its order; the + // metadata's values win. + {"oci with a layer file", OCIFeatureKeys, `{"id":"x","onlyInFile":true,"version":"0.9.0"}`, `{"2":2,"10":1,"id":"x","onlyInFile":true,"version":"1.0.1","included":true,"value":{"o":"v"},"containerEnv":{"K":"v"},"options":{},"keywords":["k"],"cachePath":"/c","consecutiveId":"x_0"}`}, + {"struct", nil, "", `{"id":"x","version":"1.0.1","name":"./x","included":true,"cachePath":"/c","consecutiveId":"x_0","value":{"o":"v"},"containerEnv":{"K":"v"},"capAdd":["SET_IN_CODE"]}`}, + } { + t.Run(tc.name, func(t *testing.T) { + g := f + g.SourceKeys = tc.source + if tc.file != "" { + g.File = json.RawMessage(tc.file) + } + got, err := json.Marshal(g) + if err != nil || string(got) != tc.want { + t.Errorf("got %s, %v\nwant %s", got, err, tc.want) + } + }) + } +} + +// TS's updateFromOldProperties for Features moves a truthy extensions and +// settings into customizations.vscode: extensions concatenated after vscode's +// (a string base concatenates as a string), settings spread under vscode's; +// devPort is left alone. +func TestFeatureMarshalJSONMovesDeprecatedProperties(t *testing.T) { + for _, tc := range []struct{ meta, want string }{ + {`{"id":"a","extensions":["x"],"settings":{"s":1,"t":1},"devPort":1,"customizations":{"vscode":{"extensions":["e"],"settings":{"t":2}}}}`, + `{"id":"a","devPort":1,"customizations":{"vscode":{"extensions":["e","x"],"settings":{"s":1,"t":2}}},"included":true,"value":null}`}, + {`{"id":"b","extensions":"solo","settings":[7]}`, + `{"id":"b","included":true,"value":null,"customizations":{"vscode":{"extensions":["solo"],"settings":{"0":7}}}}`}, + {`{"id":"c","extensions":["x",1],"customizations":{"vscode":{"extensions":"base"}}}`, + `{"id":"c","customizations":{"vscode":{"extensions":"basex,1"}},"included":true,"value":null}`}, + {`{"id":"d","extensions":[],"settings":"ab"}`, + `{"id":"d","included":true,"value":null,"customizations":{"vscode":{"extensions":[],"settings":{"0":"a","1":"b"}}}}`}, + {`{"id":"g","extensions":0,"settings":1}`, + `{"id":"g","extensions":0,"included":true,"value":null,"customizations":{"vscode":{"settings":{}}}}`}, + {`{"id":"e","extensions":"","settings":null,"devPort":1}`, + `{"id":"e","extensions":"","settings":null,"devPort":1,"included":true,"value":null}`}, + } { + var f Feature + if err := json.Unmarshal([]byte(tc.meta), &f); err != nil { + t.Fatal(err) + } + f.Included, f.SourceKeys = true, OCIFeatureKeys + if got, err := json.Marshal(f); err != nil || string(got) != tc.want { + t.Errorf("%s:\ngot %s, %v\nwant %s", tc.meta, got, err, tc.want) + } + } + var f Feature + if err := json.Unmarshal([]byte(`{"id":"f","extensions":["x"],"customizations":{"vscode":{"extensions":3}}}`), &f); err != nil { + t.Fatal(err) + } + f.SourceKeys = OCIFeatureKeys + if _, err := json.Marshal(f); err == nil { + t.Error("extensions concatenated onto a number, which has no concat: want an error") + } +} diff --git a/internal/log/log.go b/internal/log/log.go index ee53e35..08e6e82 100644 --- a/internal/log/log.go +++ b/internal/log/log.go @@ -3,7 +3,6 @@ package log import ( "cmp" - "encoding/json" "fmt" "io" "os" @@ -14,6 +13,8 @@ import ( "time" "golang.org/x/term" + + "github.com/devcontainers/cli/internal/stringify" ) // Level mirrors the TS enum values for compatibility. @@ -452,6 +453,8 @@ func toLogFileText(text string) string { // --- JSON handler --- +// jsonHandler is TS createJSONLog: each event as JSON.stringify writes it, on +// a line of its own. type jsonHandler struct { w io.Writer } @@ -461,7 +464,7 @@ func newJSONHandler(w io.Writer) Handler { } func (h *jsonHandler) HandleEvent(e Event) { - data, err := json.Marshal(e) + data, err := stringify.JSON(e, "") if err != nil { return // an Event is always serializable; drop rather than emit a broken line } diff --git a/internal/log/testdata/ts-oracle/log.json b/internal/log/testdata/ts-oracle/log.json index 7fc31fc..24fdb70 100644 --- a/internal/log/testdata/ts-oracle/log.json +++ b/internal/log/testdata/ts-oracle/log.json @@ -1,5 +1,5 @@ { - "comment": "spec-utils/log.ts logEventToTerminalText and createPlainLog's logEventToFileText, ported in log.go.", + "comment": "spec-utils/log.ts logEventToTerminalText, createPlainLog's logEventToFileText and createJSONLog, ported in log.go.", "cases": [ { "name": "terminal/no line end", @@ -735,6 +735,98 @@ 3 ], "result": "[1970-01-01T00:00:00.000Z] x\n" + }, + { + "name": "json/text with html characters", + "fn": "jsonLogText", + "args": [ + { + "type": "text", + "level": 3, + "timestamp": 1000, + "text": "Run: a && b 'd'" + } + ], + "result": "{\"type\":\"text\",\"level\":3,\"timestamp\":1000,\"text\":\"Run: a && b 'd'\"}\n" + }, + { + "name": "json/line and paragraph separators", + "fn": "jsonLogText", + "args": [ + { + "type": "text", + "level": 2, + "timestamp": 1000, + "text": "a\u2028b\u2029c" + } + ], + "result": "{\"type\":\"text\",\"level\":2,\"timestamp\":1000,\"text\":\"a\u2028b\u2029c\"}\n" + }, + { + "name": "json/control characters and non-ascii", + "fn": "jsonLogText", + "args": [ + { + "type": "text", + "level": 1, + "timestamp": 1000, + "text": "\"q\"\\\b\f\n\r\t\u0001\u001b[1mé😀" + } + ], + "result": "{\"type\":\"text\",\"level\":1,\"timestamp\":1000,\"text\":\"\\\"q\\\"\\\\\\b\\f\\n\\r\\t\\u0001\\u001b[1mé😀\"}\n" + }, + { + "name": "json/raw", + "fn": "jsonLogText", + "args": [ + { + "type": "raw", + "level": 3, + "timestamp": 1000, + "text": "\r\n" + } + ], + "result": "{\"type\":\"raw\",\"level\":3,\"timestamp\":1000,\"text\":\"\\r\\n\"}\n" + }, + { + "name": "json/start", + "fn": "jsonLogText", + "args": [ + { + "type": "start", + "level": 2, + "timestamp": 1000, + "text": "Run: docker compose -f a&b.yml config" + } + ], + "result": "{\"type\":\"start\",\"level\":2,\"timestamp\":1000,\"text\":\"Run: docker compose -f a&b.yml config\"}\n" + }, + { + "name": "json/stop", + "fn": "jsonLogText", + "args": [ + { + "type": "stop", + "level": 2, + "timestamp": 1250, + "text": "Run: x > y", + "startTimestamp": 1000 + } + ], + "result": "{\"type\":\"stop\",\"level\":2,\"timestamp\":1250,\"text\":\"Run: x > y\",\"startTimestamp\":1000}\n" + }, + { + "name": "json/progress", + "fn": "jsonLogText", + "args": [ + { + "type": "progress", + "name": "Build ", + "status": "running", + "stepDetail": "a & b" + } + ], + "result": "{\"type\":\"progress\",\"name\":\"Build \",\"status\":\"running\",\"stepDetail\":\"a & b\"}\n" } ] } diff --git a/internal/log/ts_oracle_test.go b/internal/log/ts_oracle_test.go index 067221f..5cee6f5 100644 --- a/internal/log/ts_oracle_test.go +++ b/internal/log/ts_oracle_test.go @@ -12,7 +12,7 @@ import ( // The goldens are the lines the reference log handlers write for one event // (go tool task testdata:ts-oracle): the terminal log when stdout is a TTY, the -// plain log otherwise. +// plain log otherwise, and the JSON log for --log-format json. func TestTSOracleLog(t *testing.T) { oracletest.Run(t, "testdata/ts-oracle/log.json", map[string]oracletest.Func{ "logEventToTerminalText": func(args []json.RawMessage) (any, error) { @@ -37,6 +37,15 @@ func TestTSOracleLog(t *testing.T) { // The goldens are recorded where Node's os.EOL is "\n". return written(strings.ReplaceAll(buf.String(), eol, "\n")), nil }, + "jsonLogText": func(args []json.RawMessage) (any, error) { + var e Event + if err := oracletest.Args(args, &e); err != nil { + return nil, err + } + var buf bytes.Buffer + newJSONHandler(&buf).HandleEvent(e) + return written(buf.String()), nil + }, }) } diff --git a/internal/stringify/stringify.go b/internal/stringify/stringify.go new file mode 100644 index 0000000..81fad7d --- /dev/null +++ b/internal/stringify/stringify.go @@ -0,0 +1,56 @@ +// Package stringify encodes values as JavaScript's JSON.stringify does, so the +// CLI's JSON (command results, --log-format json events) is byte-compatible +// with the TS CLI's. +package stringify + +import "encoding/json" + +// escapes are the escapes encoding/json writes that JSON.stringify does not: it +// writes &, <, > and U+2028/U+2029 as they are. +var escapes = map[string]string{ + "\\u0026": "&", + "\\u003c": "<", + "\\u003e": ">", + "\\u2028": "\u2028", + "\\u2029": "\u2029", +} + +// JSON encodes v as JSON.stringify(v) would. encoding/json HTML-escapes every +// string, including the ones nested json.Marshaler implementations write, so +// the escapes are undone on the encoded document. A non-empty indent indents +// it as JSON.stringify(v, undefined, indent) does. +func JSON(v any, indent string) ([]byte, error) { + var data []byte + var err error + if indent == "" { + data, err = json.Marshal(v) + } else { + data, err = json.MarshalIndent(v, "", indent) + } + if err != nil { + return nil, err + } + return unescape(data), nil +} + +// unescape rewrites, in encoded JSON, the escapes in escapes as the characters +// they stand for. A backslash only occurs in a string, always as the start of +// an escape, so taking escapes in pairs leaves an escaped backslash followed by +// "u0026" alone. +func unescape(data []byte) []byte { + out := make([]byte, 0, len(data)) + for i := 0; i < len(data); i++ { + if data[i] != '\\' { + out = append(out, data[i]) + continue + } + if s, ok := escapes[string(data[i:min(i+6, len(data))])]; ok { + out = append(out, s...) + i += 5 + continue + } + out = append(out, data[i], data[i+1]) + i++ + } + return out +} diff --git a/internal/stringify/stringify_test.go b/internal/stringify/stringify_test.go new file mode 100644 index 0000000..7f04b1e --- /dev/null +++ b/internal/stringify/stringify_test.go @@ -0,0 +1,47 @@ +package stringify + +import "testing" + +// rawMarshaler writes its JSON itself, as the CLI's ordered result objects do. +type rawMarshaler string + +func (r rawMarshaler) MarshalJSON() ([]byte, error) { return []byte(r), nil } + +// JSON.stringify writes &, <, > and U+2028/U+2029 as they are, and every +// other escape as encoding/json does. +func TestJSONWritesWhatJSONStringifyWrites(t *testing.T) { + cases := []struct { + name string + v any + indent string + want string + }{ + {"html characters", "a && b ", "", "\"a && b \""}, + {"line and paragraph separators", "\u2028\u2029", "", "\"\u2028\u2029\""}, + {"an escaped backslash before u0026", `\u0026`, "", `"\\u0026"`}, + {"an escaped quote at the end", `"`, "", `"\""`}, + {"control characters", "\b\f\n\r\t\x01", "", `"\b\f\n\r\t\u0001"`}, + {"a nested marshaler", map[string]any{"k": rawMarshaler(`{"":"&"}`)}, "", `{"k":{"":"&"}}`}, + {"indented", map[string]any{"k": ""}, " ", "{\n \"k\": \"\"\n}"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := JSON(tc.v, tc.indent) + if err != nil { + t.Fatal(err) + } + if string(got) != tc.want { + t.Errorf("JSON = %s, want %s", got, tc.want) + } + }) + } +} + +func TestJSONReportsAnUnencodableValue(t *testing.T) { + if _, err := JSON(make(chan int), ""); err == nil { + t.Error("JSON of a channel succeeded") + } + if _, err := JSON(make(chan int), " "); err == nil { + t.Error("indented JSON of a channel succeeded") + } +} diff --git a/tools/tsoracle/oracle.ts b/tools/tsoracle/oracle.ts index a6c151e..26c986f 100644 --- a/tools/tsoracle/oracle.ts +++ b/tools/tsoracle/oracle.ts @@ -19,7 +19,7 @@ import { getRef, getCollectionRef } from '../../reference/src/spec-configuration import { replaceTemplatedValues } from '../../reference/src/spec-configuration/containerTemplatesOCI'; import { readVersionPrefix, convertMountToVolume, convertMountToVolumeTopLevelElement } from '../../reference/src/spec-node/dockerCompose'; import { qualifyImageName, isBuildxCacheToInline } from '../../reference/src/spec-node/utils'; -import { logEventToTerminalText, createPlainLog } from '../../reference/src/spec-utils/log'; +import { logEventToTerminalText, createPlainLog, createJSONLog } from '../../reference/src/spec-utils/log'; import { parseCrossOriginAuthHosts, isAllowedTokenServiceRealm } from '../../reference/src/spec-configuration/httpOCIRegistry'; import { readLockfile } from '../../reference/src/spec-configuration/lockfile'; @@ -93,6 +93,12 @@ const fns: Record any> = { createPlainLog(t => { text = t; }, () => logLevel).event(e); return text; }, + // The line the JSON log (--log-format json) writes for one event. + jsonLogText: (e: any) => { + let text: string | null = null; + createJSONLog(t => { text = t; }, () => 0, new Date(0)).event(e); + return text; + }, // spec-configuration/httpOCIRegistry.ts: the registry → auth-hosts Map as an // object of sorted arrays, so the golden is order-independent. parseCrossOriginAuthHosts: (entries: string[]) => Object.fromEntries(