Preserve Dockerfile devcontainer.metadata during image build#1225
Open
RossBugginsNHS wants to merge 3 commits into
Open
Preserve Dockerfile devcontainer.metadata during image build#1225RossBugginsNHS wants to merge 3 commits into
RossBugginsNHS wants to merge 3 commits into
Conversation
## Summary This PR fixes a metadata regression where `devcontainer build` could produce a final effective `devcontainer.metadata` label that excluded user-declared metadata from the Dockerfile. Target for this PR is **my fork's `main` branch** so it can be used as a fully documented staging PR before opening upstream. ## Problem Statement ### Expected User-provided Dockerfile metadata (`LABEL devcontainer.metadata=...`) should be preserved and merged with feature/runtime metadata in the final effective label. ### Actual In affected build paths (especially feature-enabled paths), metadata assembly used base-image metadata but did not reliably include user Dockerfile label metadata in the final computed metadata payload. ### Impact Runtime-critical config encoded in metadata (for example mounts and lifecycle hooks) can be silently dropped from effective container configuration. ## Root Cause (Code Path) 1. Dockerfile build flow computes metadata via `internalGetImageBuildInfoFromDockerfile(...)`. 2. That path used `findBaseImage(...)` + `inspectDockerImage(baseImage)` and parsed image label metadata from the resolved base image. 3. User metadata from Dockerfile label instructions was not merged into the computed metadata set used downstream for wrapper/feature label generation. ## What Changed ### 1) Preserve Dockerfile label metadata during image build info computation File: `src/spec-node/imageMetadata.ts` - In `internalGetImageBuildInfoFromDockerfile(...)`: - Keep existing base image metadata extraction. - Parse `LABEL devcontainer.metadata=...` entries from Dockerfile text. - Merge parsed Dockerfile metadata entries with base metadata (`base + dockerfile`) before downstream merge/composition. ### 2) Reuse shared parsing logic File: `src/spec-node/imageMetadata.ts` - Extracted shared metadata JSON parse helper used by both: - image-label parsing (`internalGetImageMetadata0`), and - Dockerfile label parsing. ### 3) Add regression tests File: `src/test/dockerfileUtils.test.ts` Added tests under `getImageBuildInfo`: - `preserves metadata declared in Dockerfile labels` - `parses metadata declared with a quoted label key` These assert user Dockerfile metadata is present in `info.metadata.raw` (and substituted metadata in `info.metadata.config`) after `internalGetImageBuildInfoFromDockerfile(...)`. ## Validation Performed - Targeted tests: - `mocha -r ts-node/register --exit src/test/dockerfileUtils.test.ts` - Result: **71 passing, 0 failing** - Lint: - `eslint src/spec-node/imageMetadata.ts src/test/dockerfileUtils.test.ts` - Result: clean - Type-check: - `npm run type-check` - Result: success ## Why This Is Safe - Scope is limited to metadata extraction/merge in Dockerfile build-info path. - No changes to public CLI arguments or command behavior outside metadata assembly. - Regression tests added for the new behavior and edge case label key quoting. ## Upstream Follow-Up Notes When opening upstream PR to `devcontainers/cli`: 1. Reuse this PR body sections directly (Problem, Root Cause, Changes, Validation). 2. Include your externally observed reproduction matrix (feature + metadata-bearing base, feature + plain base, etc.). 3. Keep focus on spec-intended composition behavior: preserve user metadata, append/merge feature/runtime metadata. 4. Optional follow-up tests (integration-level) can assert final built image inspect output includes both user marker metadata and feature metadata. ## Commit - `b34a404` - Preserve Dockerfile devcontainer.metadata during image build
|
@RossBugginsNHS please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Attribution
Submitted by Ross Buggins after interactive investigation and drafting assistance from GitHub Copilot. The changes and PR content were reviewed and submitted by the author, not posted by an unattended bot.
Summary
This PR preserves user-declared Dockerfile
devcontainer.metadataduring CLI image-build metadata computation so that final effective metadata composition does not drop user image metadata entries.Related Discussion
Spec/proposal issue:
Fork issue used during preparation:
Problem
Expected
If a Dockerfile includes
LABEL devcontainer.metadata=..., those entries should survive CLI build composition and be merged with base image, feature, and config-derived metadata.Actual
In affected Dockerfile build paths, especially feature-enabled ones, the final effective
devcontainer.metadatacan exclude metadata declared directly in the user Dockerfile.Impact
This can silently remove runtime-relevant configuration encoded in metadata, including examples like:
postCreateCommandRoot Cause
internalGetImageBuildInfoFromDockerfile(...)derived metadata from the resolved base image, but did not reliably merge metadata declared directly in DockerfileLABEL devcontainer.metadata=...instructions into the metadata set later used for final wrapper/feature label generation.What This PR Changes
1. Preserve Dockerfile label metadata in Dockerfile build-info extraction
File:
src/spec-node/imageMetadata.tsChanges:
devcontainer.metadatavalues from DockerfileLABELinstructions.internalGetImageBuildInfoFromDockerfile(...).2. Reuse shared metadata JSON parsing
File:
src/spec-node/imageMetadata.tsChanges:
3. Add regression tests
File:
src/test/dockerfileUtils.test.tsAdded tests:
preserves metadata declared in Dockerfile labelsparses metadata declared with a quoted label keyThese verify that Dockerfile-declared metadata is present in both raw and substituted metadata returned by
internalGetImageBuildInfoFromDockerfile(...).Validation
Executed locally:
mocha -r ts-node/register --exit src/test/dockerfileUtils.test.tseslint src/spec-node/imageMetadata.ts src/test/dockerfileUtils.test.tsnpm run type-checkWhy This Is Safe
Notes
This PR is intended to align implementation with the expected metadata-composition behavior. If maintainers would prefer to settle semantics first in the spec discussion, this PR can still serve as a concrete implementation reference and regression test basis.