fix: set default base image in Dockerfiles and improve syntax handling - #1271
fix: set default base image in Dockerfiles and improve syntax handling#1271Kaniska (Kaniska244) wants to merge 7 commits into
Conversation
| }); | ||
|
|
||
| // returns the names of any ARGs used in a `FROM` that lack a default declared before them. | ||
| function findFromArgsWithoutDefault(dockerfile: string): string[] { |
There was a problem hiding this comment.
Do we not also want to use this for other features?
There was a problem hiding this comment.
Hi Abdurrahmaan Iqbal (@abdurriq)
Thank you for pointing this out. It's done now. Kindly let me know in case of any concern.
|
Hi Kaniska (@Kaniska244), The current behaviour is breaking existing workflows. |
There was a problem hiding this comment.
Pull request overview
This PR addresses Docker/BuildKit’s InvalidDefaultArgInFrom warning by ensuring Dockerfiles (generated by the CLI and scripts/updateUID.Dockerfile) declare a default value for any ARG referenced by a FROM, using syntax the linter recognizes.
Changes:
- Set
_DEV_CONTAINERS_BASE_IMAGEdefault toscratchin generated Dockerfile prefixes and in the feature base Dockerfile template. - Set
BASE_IMAGE=placeholderinscripts/updateUID.Dockerfileto avoidFROM $BASE_IMAGEdefault warnings. - Add unit tests to regression-check generated Dockerfiles for
ARGdefaults beforeFROM, plus a helper to detect violations.
Show a summary per file
| File | Description |
|---|---|
src/test/testUtils.ts |
Adds a helper to detect FROM-referenced args lacking defaults (used by new tests). |
src/test/container-features/generateFeaturesConfig.test.ts |
Adds regression tests ensuring generated Dockerfiles avoid InvalidDefaultArgInFrom. |
src/spec-node/containerFeatures.ts |
Changes generated Dockerfile prefix default _DEV_CONTAINERS_BASE_IMAGE from placeholder to scratch. |
src/spec-configuration/containerFeaturesConfiguration.ts |
Adds ARG _DEV_CONTAINERS_BASE_IMAGE=scratch before template FROM stages. |
scripts/updateUID.Dockerfile |
Adds default BASE_IMAGE=placeholder to satisfy Dockerfile parsing/linting. |
.devcontainer/devcontainer-lock.json |
Trailing newline / formatting-only change. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 5/6 changed files
- Comments generated: 1
- Review effort level: Lite
| export function findFromArgsWithoutDefault(dockerfile: string): string[] { | ||
| const argsWithDefault = new Set<string>(); | ||
| const offenders: string[] = []; | ||
|
|
||
| for (const rawLine of dockerfile.split('\n')) { | ||
| const line = rawLine.trim(); | ||
| const argWithDefault = /^ARG\s+([A-Za-z0-9_]+)\s*=\s*\S+/.exec(line); | ||
| if (argWithDefault) { | ||
| argsWithDefault.add(argWithDefault[1]); | ||
| continue; | ||
| } | ||
| if (/^ARG\s+[A-Za-z0-9_]+\s*$/.test(line)) { | ||
| continue; | ||
| } | ||
| const fromMatch = /^FROM\s+\$\{?([A-Za-z0-9_]+)/.exec(line); | ||
| if (fromMatch && !argsWithDefault.has(fromMatch[1])) { | ||
| offenders.push(fromMatch[1]); | ||
| } | ||
| } | ||
| return offenders; | ||
| } |
What this PR does
Related #1229. Fixes the
InvalidDefaultArgInFrombuild warning emitted by Docker/BuildKit when building dev containers. The warning is raised whenever aFROMinstruction references a buildARGthat has no in-scope default value at parse time.This warning does not come from the user's project Dockerfile — it originates from the internal Dockerfiles the CLI generates (and the static
updateUID.Dockerfile), whereFROMreferenced_DEV_CONTAINERS_BASE_IMAGE/BASE_IMAGEwithout a declared default.Root cause
Two patterns were triggering the linter:
scripts/updateUID.DockerfiledeclaredARG BASE_IMAGE(no default) and then usedFROM $BASE_IMAGE.FROM ${_DEV_CONTAINERS_BASE_IMAGE:-scratch}. The${VAR:-scratch}form is a runtime shell expansion; it is not recognized as anARGdefault by the static linter, so the warning persisted even when a globalARG ... = placeholderwas declared.The fix
Declare the base-image
ARGwith a real default (scratchfor the generated files,placeholderfor the UID script) immediately before it's used, and reference it plainly as$VAR— the form the linter recognizes as having a valid default.src/spec-configuration/containerFeaturesConfiguration.ts— addARG _DEV_CONTAINERS_BASE_IMAGE=scratchbefore theFROMstages in the generated feature base Dockerfile (theFROMlines already reference the ARG directly).src/spec-node/containerFeatures.ts— change the prefixARGdefault fromplaceholdertoscratchin bothgetImageBuildOptions(no-features path) andgetFeaturesBuildOptions(features path).scripts/updateUID.Dockerfile— changeARG BASE_IMAGE→ARG BASE_IMAGE=placeholder.Behavior is unchanged at build time
The CLI always passes the real image via
--build-arg(_DEV_CONTAINERS_BASE_IMAGE=<image>, andBASE_IMAGE=<image>for the UID Dockerfile), so thescratch/placeholderdefaults only ever exist to satisfy the static linter and never affect the actual build.Tests
Added a daemon-free unit test suite in
src/test/container-features/generateFeaturesConfig.test.ts(validate generated Dockerfiles avoid InvalidDefaultArgInFrom) that:findFromArgsWithoutDefault()helper mimicking theInvalidDefaultArgInFromrule (flags anyARGused in aFROMwithout a default declared before it)._DEV_CONTAINERS_BASE_IMAGEwith a default, references it directly, and does not use the${VAR:-default}shell fallback.scripts/updateUID.DockerfiledeclaresBASE_IMAGEwith a default beforeFROM.These run without Docker, so they execute in CI as part of
npm test.Files changed
src/spec-configuration/containerFeaturesConfiguration.tsARG _DEV_CONTAINERS_BASE_IMAGE=scratchbeforeFROMsrc/spec-node/containerFeatures.tsplaceholder→scratchin both build-option pathsscripts/updateUID.DockerfileARG BASE_IMAGE→ARG BASE_IMAGE=placeholdersrc/test/container-features/generateFeaturesConfig.test.tsInvalidDefaultArgInFromregression testsNotes / follow-ups
getImageBuildOptionsno-features path is covered indirectly (its ARG default was corrected), but its Dockerfile string is built inline and isn't directly unit-tested. A small follow-up could extract that string into an exported helper to unit-test it the same way..devcontainer/devcontainer-lock.jsonshows a trailing-newline-only change; consider reverting that if it's unintentional.