feat(config): resolve variable substitution in extends paths#254
feat(config): resolve variable substitution in extends paths#254skevetter wants to merge 1 commit into
Conversation
The devcontainer spec allows variable substitution (e.g., ${localEnv:X},
${localWorkspaceFolder}) in all string properties including extends paths.
Previously, extends resolution happened before variable substitution, so
variable references in extends paths were used as literal strings.
Apply variable substitution to extends ref strings before the extends
resolver attempts to open the referenced files. This handles both the
top-level ParseDevContainerJSONFile entry point and the recursive
parseDevContainerJSONFileWithVisited path.
❌ Deploy Preview for devsydev failed.
|
📝 WalkthroughWalkthroughThis PR adds variable substitution support to devcontainer ChangesVariable Substitution in Extends Resolution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/devcontainer/config/parse.go`:
- Around line 294-295: The computed localWorkspaceFolder in
substituteExtendsRefs is wrong for non-.devcontainer paths; pass the workspace
folder through the call chain instead of deriving it with
filepath.Dir(filepath.Dir(configFilePath)). Add a workspaceFolder string
parameter to ParseDevContainerJSONFile, parseDevContainerJSONFileWithVisited,
and substituteExtendsRefs (and update callers in extends.go) so the correct
workspace root is used; alternatively, as a minimal interim fix implement the
suggested heuristic inside substituteExtendsRefs to detect root-level and nested
.devcontainer layouts rather than always using Dir(Dir(configFilePath)). Ensure
all function signatures and recursive calls are updated to accept and forward
workspaceFolder.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dec43bd6-76a4-422a-b7f9-176ccf5481c0
📒 Files selected for processing (3)
pkg/devcontainer/config/extends.gopkg/devcontainer/config/extends_test.gopkg/devcontainer/config/parse.go
| func substituteExtendsRefs(refs ExtendsRef, configFilePath string) ExtendsRef { | ||
| localWorkspaceFolder := filepath.Dir(filepath.Dir(configFilePath)) |
There was a problem hiding this comment.
localWorkspaceFolder is computed incorrectly for non-.devcontainer/ config paths.
filepath.Dir(filepath.Dir(configFilePath)) only yields the correct workspace root when configFilePath lives inside a .devcontainer/ subdirectory (the canonical .devcontainer/devcontainer.json layout). For the other two standard layouts it is wrong:
| Config path | Dir(Dir(path)) |
Correct workspace |
|---|---|---|
/ws/.devcontainer/devcontainer.json |
/ws ✓ |
/ws |
/ws/.devcontainer.json (root-level) |
parent of /ws ✗ |
/ws |
/ws/.devcontainer/myproj/devcontainer.json |
/ws/.devcontainer ✗ |
/ws |
And when substituteExtendsRefs is called recursively from extends.go with a parent file that lives outside a .devcontainer/ directory (e.g., a shared base at /shared/base.json), the computed workspace is again wrong.
The proper fix is to thread the workspace folder through the call chain as a parameter. A lower-effort intermediate heuristic that handles the two most common layouts:
🛠️ Suggested heuristic fix
func substituteExtendsRefs(refs ExtendsRef, configFilePath string) ExtendsRef {
- localWorkspaceFolder := filepath.Dir(filepath.Dir(configFilePath))
+ // localWorkspaceFolder: parent of the .devcontainer/ dir when the config
+ // is inside one, otherwise the directory that contains the config file.
+ configDir := filepath.Dir(configFilePath)
+ localWorkspaceFolder := configDir
+ if filepath.Base(configDir) == ".devcontainer" {
+ localWorkspaceFolder = filepath.Dir(configDir)
+ }The long-term fix is to accept workspaceFolder string as a parameter on ParseDevContainerJSONFile, parseDevContainerJSONFileWithVisited, and substituteExtendsRefs, matching the devcontainer spec where this value is provided by the host.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func substituteExtendsRefs(refs ExtendsRef, configFilePath string) ExtendsRef { | |
| localWorkspaceFolder := filepath.Dir(filepath.Dir(configFilePath)) | |
| func substituteExtendsRefs(refs ExtendsRef, configFilePath string) ExtendsRef { | |
| // localWorkspaceFolder: parent of the .devcontainer/ dir when the config | |
| // is inside one, otherwise the directory that contains the config file. | |
| configDir := filepath.Dir(configFilePath) | |
| localWorkspaceFolder := configDir | |
| if filepath.Base(configDir) == ".devcontainer" { | |
| localWorkspaceFolder = filepath.Dir(configDir) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/devcontainer/config/parse.go` around lines 294 - 295, The computed
localWorkspaceFolder in substituteExtendsRefs is wrong for non-.devcontainer
paths; pass the workspace folder through the call chain instead of deriving it
with filepath.Dir(filepath.Dir(configFilePath)). Add a workspaceFolder string
parameter to ParseDevContainerJSONFile, parseDevContainerJSONFileWithVisited,
and substituteExtendsRefs (and update callers in extends.go) so the correct
workspace root is used; alternatively, as a minimal interim fix implement the
suggested heuristic inside substituteExtendsRefs to detect root-level and nested
.devcontainer layouts rather than always using Dir(Dir(configFilePath)). Ensure
all function signatures and recursive calls are updated to accept and forward
workspaceFolder.
|
Closing — after investigation, this fix is not warranted:
|
1 participant
Summary
${localEnv:X},${localWorkspaceFolder}, etc.) in devcontainer.jsonextendspathsSpec reference: https://containers.dev/implementors/reference/
Summary by CodeRabbit
Bug Fixes
Tests