feat(build): improve performance and reliability of devcontainer launch (#977)#472
feat(build): improve performance and reliability of devcontainer launch (#977)#472stewartadam wants to merge 12 commits intomicrosoft:mainfrom
Conversation
|
Follow up to #119. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #472 +/- ##
=======================================
Coverage 88.01% 88.01%
=======================================
Files 45 45
Lines 7886 7886
=======================================
Hits 6941 6941
Misses 945 945
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Improves devcontainer build/attach performance and reliability by removing unneeded features, standardizing the workspace path, and adding volume mounts and git-config handling for better cross-environment behavior.
Changes:
- Standardize devcontainer workspace to
/workspaceand add volumes for user config +node_modules. - Add post-create ownership workaround for mounted volumes and a post-attach git identity reconfiguration step.
- Add repository-level ignores/attributes to support the new devcontainer flow (gitconfig exports, dockerignore, line ending rules).
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
.gitignore |
Ignores generated gitconfig export files used by the devcontainer attach workaround. |
.gitattributes |
Adds explicit checkout line-ending rules for scripts and Dockerfiles. |
.dockerignore |
Excludes .git and node_modules from Docker build contexts for speed/smaller contexts. |
.devcontainer/scripts/post-create.sh |
Adds volume ownership fix step and installs Node dependencies during container setup. |
.devcontainer/scripts/post-attach.sh |
Applies exported git identity settings inside the container after attach. |
.devcontainer/devcontainer.json |
Normalizes workspace mount/folder, adds volumes, removes unused features, and wires initialize/postAttach commands. |
.devcontainer/README.md |
Updates documented toolset and troubleshooting guidance to match the new devcontainer behavior. |
|
@stewartadam - reverting to draft for now. |
|
Hey @stewartadam — thanks for capturing these devcontainer learnings from customer projects! 🚀 We've updated the PR title scope from |
|
We've updated the PR description to align with the current pull request template. All original content has been preserved and relocated into the appropriate template sections. No action needed on your end — though you're welcome to review the updated description and fill in any remaining sections (testing details, checklist confirmations, etc.) at your convenience. |
|
Hi @stewartadam — Thanks for this contribution! A quick note on project conventions: we ask that pull requests be paired with a corresponding backlog issue for tracking purposes. I wasn't able to find an existing issue related to devcontainer performance and reliability improvements. When you get a chance, could you file an issue describing the changes this PR addresses and link it here using |
e0cc129 to
98a91ba
Compare
|
Created #977 and updated based on feedback |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
e238261 to
3f87e6d
Compare
3f87e6d to
265d54b
Compare
chaosdinosaur
left a comment
There was a problem hiding this comment.
Devcontainer Enhancement Review
Solid set of improvements for devcontainer performance and reliability. The volume mounts, workspace normalization, and git config include workaround address real pain points from customer projects.
Key items to address before merge:
- 🔴 Node.js version regression — branch has v20, main has v24. Needs rebase/merge.
- 🟡 Security:
initializeCommanddumps full git config — suggest filtering to only needed keys. - 🟡
updateContentCommandremoval — suggest keeping it for automaticnpm cion content updates.
Minor items:
- 🟢 Hardcoded
/workspacepath coupling inpost-create.sh - 🔵
sudo -nbase image dependency (well-documented) - 🔵
.dockerignorewith no Dockerfile (harmless, forward-looking)
The .gitattributes additions for line-ending enforcement across script types are a welcome improvement.
| ], | ||
| "features": { | ||
| "ghcr.io/devcontainers/features/node:1": { | ||
| "version": "20" |
There was a problem hiding this comment.
🔴 High — Node.js version regression
The PR branch specifies Node.js "version": "20", but origin/main now uses "version": "24". Merging as-is would regress the Node.js version. The branch needs a rebase or merge from main to pick up this change.
| "version": "20" | |
| "version": "24" |
| "extractGitGlobals": "(git config -l --global --include || true) > .gitconfig.global", | ||
| "extractGitLocals": "(git config -l --local --include || true) > .gitconfig.local" |
There was a problem hiding this comment.
🟡 Medium — initializeCommand dumps full git config to disk
This writes the entire global and local git config to plaintext files in the workspace root. While post-attach.sh only reads a safe allow-list (user.name, user.email, user.signingkey, commit.gpgsign) and then deletes the files, there's a window where sensitive config values (credential helpers, proxy settings, insteadOf rewrites) are exposed on disk.
Consider filtering at the source to only extract the keys actually needed:
| "extractGitGlobals": "(git config -l --global --include || true) > .gitconfig.global", | |
| "extractGitLocals": "(git config -l --local --include || true) > .gitconfig.local" | |
| "extractGitGlobals": "(git config --global --include user.name; git config --global --include user.email; git config --global --include user.signingkey; git config --global --include commit.gpgsign) > .gitconfig.global 2>/dev/null || true", | |
| "extractGitLocals": "(git config --local --include user.name; git config --local --include user.email; git config --local --include user.signingkey; git config --local --include commit.gpgsign) > .gitconfig.local 2>/dev/null || true" |
This eliminates the information disclosure window by never writing unneeded config values to disk.
| "onCreateCommand": "bash .devcontainer/scripts/on-create.sh", | ||
| "updateContentCommand": "npm ci", | ||
| "postCreateCommand": "bash .devcontainer/scripts/post-create.sh", |
There was a problem hiding this comment.
🟡 Medium — Was the updateContentCommand removal intentional?
The base branch uses updateContentCommand: "npm ci", which runs during both initial creation and content update events (branch switches, pulls while the container is running). This lifecycle hook is also parallelizable with other steps.
Moving npm ci exclusively into postCreateCommand (via post-create.sh) means it only runs on initial container creation, not on content updates. If package-lock.json changes on a branch switch inside a running container, developers will need to manually run npm ci.
I'd suggest keeping updateContentCommand: "npm ci" alongside the postCreateCommand call to maintain automatic dependency updates on content changes:
| "onCreateCommand": "bash .devcontainer/scripts/on-create.sh", | |
| "updateContentCommand": "npm ci", | |
| "postCreateCommand": "bash .devcontainer/scripts/post-create.sh", | |
| "onCreateCommand": "bash .devcontainer/scripts/on-create.sh", | |
| "updateContentCommand": "npm ci", | |
| "postCreateCommand": "bash .devcontainer/scripts/post-create.sh", |
| echo "Applying volume ownership workaround (see microsoft/vscode-remote-release#9931)..." | ||
| fix_volume_ownership "/home/${USER}/.config" | ||
| fix_volume_ownership "/workspace/node_modules" | ||
| } |
There was a problem hiding this comment.
🟢 Low — Hardcoded /workspace path
This hardcodes /workspace/node_modules which couples the script to the workspaceFolder value in devcontainer.json. If the mount path ever changes, this line silently breaks.
Consider using "$PWD/node_modules" or a variable to keep the two in sync:
| } | |
| fix_volume_ownership "$PWD/node_modules" |
|
|
||
| echo "Setting volume ownership for $volume_path" | ||
| sudo -n chown "${USER}":"${USER}" "$volume_path" | ||
| } |
There was a problem hiding this comment.
🔵 Informational — sudo -n dependency on base image
sudo -n chown requires passwordless sudo, which the current base image (mcr.microsoft.com/devcontainers/base:2-jammy) provides. The inline comment documenting this assumption is appreciated. Just flagging for awareness — if the base image ever changes, this assumption will need revalidation.
There was a problem hiding this comment.
🔵 Informational — .dockerignore has no current effect
The devcontainer uses "image:" rather than a Dockerfile build, so .dockerignore has no current effect (it only applies to Docker build contexts). The file is harmless and good practice if the setup ever transitions to a Dockerfile-based build — just noting for transparency.
Pull Request
Description
Captures learnings from recent customer projects to enhance the build speed and launch reliability of the dev container.
Related Issue(s)
Type of Change
Select all that apply:
Code & Documentation:
Infrastructure & Configuration:
AI Artifacts:
prompt-builderagent and addressed all feedback.github/instructions/*.instructions.md).github/prompts/*.prompt.md).github/agents/*.agent.md).github/skills/*/SKILL.md)Other:
.ps1,.sh,.py)Sample Prompts (for AI Artifact Contributions)
User Request:
Execution Flow:
Output Artifacts:
Success Indicators:
For detailed contribution requirements, see:
Testing
Checklist
Required Checks
AI Artifact Contributions
/prompt-analyzeto review contributionprompt-builderreviewRequired Automated Checks
The following validation commands must pass before merging:
npm run lint:mdnpm run spell-checknpm run lint:frontmatternpm run validate:skillsnpm run lint:md-linksnpm run lint:psnpm run plugin:generateSecurity Considerations
Additional Notes