-
Notifications
You must be signed in to change notification settings - Fork 382
Add support for "extends" keyword #311
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,9 +3,9 @@ | |
| * Licensed under the MIT License. See License.txt in the project root for license information. | ||
| *--------------------------------------------------------------------------------------------*/ | ||
|
|
||
| import * as path from 'path'; | ||
|
|
||
| import * as deepmerge from 'deepmerge-ts'; | ||
| import * as jsonc from 'jsonc-parser'; | ||
| import * as path from 'path'; | ||
|
|
||
| import { openDockerfileDevContainer } from './singleContainer'; | ||
| import { openDockerComposeDevContainer } from './dockerCompose'; | ||
|
|
@@ -71,26 +71,41 @@ async function resolveWithLocalFolder(params: DockerResolverParameters, parsedAu | |
| return result; | ||
| } | ||
|
|
||
| export async function readDevContainerConfigFile(cliHost: CLIHost, workspace: Workspace | undefined, configFile: URI, mountWorkspaceGitRoot: boolean, output: Log, consistency?: BindMountConsistency, overrideConfigFile?: URI) { | ||
| async function readFile(cliHost: CLIHost, path: URI) { | ||
| const documents = createDocuments(cliHost); | ||
| const content = await documents.readDocument(overrideConfigFile ?? configFile); | ||
| const content = await documents.readDocument(path); | ||
| if (!content) { | ||
| return undefined; | ||
| } | ||
| const raw = jsonc.parse(content) as DevContainerConfig | undefined; | ||
| const updated = raw && updateFromOldProperties(raw); | ||
| if (!updated || typeof updated !== 'object' || Array.isArray(updated)) { | ||
| throw new ContainerError({ description: `Dev container config (${uriToFsPath(configFile, cliHost.platform)}) must contain a JSON object literal.` }); | ||
| throw new ContainerError({ description: `Dev container config (${uriToFsPath(path, cliHost.platform)}) must contain a JSON object literal.` }); | ||
| } | ||
| const workspaceConfig = await getWorkspaceConfiguration(cliHost, workspace, updated, mountWorkspaceGitRoot, output, consistency); | ||
|
|
||
| return updated; | ||
| } | ||
|
|
||
| export async function readDevContainerConfigFile(cliHost: CLIHost, workspace: Workspace | undefined, configFile: URI, mountWorkspaceGitRoot: boolean, output: Log, consistency?: BindMountConsistency, overrideConfigFile?: URI) { | ||
| const confPath = overrideConfigFile ?? configFile; | ||
| let content = await readFile(cliHost, confPath) as DevContainerConfig; | ||
|
|
||
| if (content.extends) { | ||
| const extendsConfPath = path.resolve(path.dirname(confPath.path), content.extends); | ||
| const referencedContent = await readFile(cliHost, URI.file(extendsConfPath)) as DevContainerConfig; | ||
| delete content.extends; | ||
| content = deepmerge.deepmerge(referencedContent, content); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have merge logic specified for the image metadata, should we reuse the same here? https://github.com/devcontainers/spec/blob/main/proposals/image-metadata.md#merge-logic Plain deep merge might not always have the expected outcome and could result in invalid configuration more easily.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the "extends" proposal, we discussed a using a simple deep-merge where the behavior is based on the type and there isn't specific per-key behavior like the image metadata merge algorithm requires, similar to how docker-compose handles adding and overriding configuration. For example, in the image metadata merge algorithm,
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is implemented here: https://github.com/devcontainers/cli/blob/ddd743e7f04bbb5f90793b29506931cec65b0922/src/spec-node/imageMetadata.ts It covers having a devcontainer.json to build a base image that is then reused by a project's devcontainer.json. This might turn the simplicity argument around because it will be simpler for users to deal with a single merge logic than with multiple ones. What do you think? (We might have to add to the current merge logic, but that's fine if it keeps the UX simple/r.)
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't have strong feelings either way aside from ensuring we meet the technical needs of allowing users to override settings and that we do that in as intuitive and user-friendly way as possible. If the image-metadata merge algorithm is solidified and there's an expectation that users would be familiar with it, then it makes sense to be consistent and expect the same thing when using "extends". @Chuxel, I know you had good intuition and feedback when we were working on the proposal. Do you have any feelings either way about using a docker-compose-style merge versus the nuanced image-metadata merge?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One consideration I forgot to mention is that whatever merging behavior we adopt, we will more than likely need to duplicate it in GitHub.com in order to properly read the configuration for Codespaces. Has that already been discussed for image metadata support?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It makes sense to me to keep the two merge logics identical. For example, I'd think manually extending from a prebuilt image's devcontainer.json (https://github.com/devcontainers/images/blob/main/src/go/.devcontainer/devcontainer.json), should behave the same as doing
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I think we said that compose was the obvious baseline implementation, but this was back before we had the features and image metadata merge logic. Now that we have a more refined view, I agree consistency would probably make sense. We doc'd the merge logic in the spec as well.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍🏻 That sounds good. I can update this PR and mirror the merge implementation to dotcom so (in theory) both the devcontainer-cli and dotcom resolve the configuration the same. We'll need to remember to keep it in mind if additional special cases are added since it could create a confusing situation if they begin to diverge. |
||
| } | ||
|
|
||
| const workspaceConfig = await getWorkspaceConfiguration(cliHost, workspace, content, mountWorkspaceGitRoot, output, consistency); | ||
| const substitute0: SubstituteConfig = value => substitute({ | ||
| platform: cliHost.platform, | ||
| localWorkspaceFolder: workspace?.rootFolderPath, | ||
| containerWorkspaceFolder: workspaceConfig.workspaceFolder, | ||
| configFile, | ||
| env: cliHost.env, | ||
| }, value); | ||
| const config: DevContainerConfig = substitute0(updated); | ||
| const config: DevContainerConfig = substitute0(content); | ||
| if (typeof config.workspaceFolder === 'string') { | ||
| workspaceConfig.workspaceFolder = config.workspaceFolder; | ||
| } | ||
|
|
@@ -101,7 +116,7 @@ export async function readDevContainerConfigFile(cliHost: CLIHost, workspace: Wo | |
| return { | ||
| config: { | ||
| config, | ||
| raw: updated, | ||
| raw: content, | ||
| substitute: substitute0, | ||
| }, | ||
| workspaceConfig, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| { | ||
| "name": "example configuration", | ||
| "image": "mcr.microsoft.com/devcontainers/base:latest", | ||
| "forwardPorts": [80], | ||
| "features": { | ||
| "ghcr.io/devcontainers/features/go:1": { | ||
| "version": "latest" | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| { | ||
| "extends": "./.devcontainer.base.json", | ||
| "name": "Overrides", | ||
| "forwardPorts": [443], | ||
| "features": { | ||
| "ghcr.io/devcontainers/features/docker-in-docker:1": { | ||
| "version": "latest", | ||
| "moby": true | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| import path from 'path'; | ||
| import { getCLIHost } from '../../spec-common/cliHost'; | ||
| import { loadNativeModule } from '../../spec-common/commonUtils'; | ||
| import { readDevContainerConfigFile } from '../../spec-node/configContainer'; | ||
| import { nullLog } from '../../spec-utils/log'; | ||
| import { URI } from 'vscode-uri'; | ||
| import { assert } from 'chai'; | ||
|
|
||
| describe('readDevContainerConfigFile', async function () { | ||
| it('can read a basic configuration file', async function () { | ||
| const cwd = process.cwd(); | ||
| const cliHost = await getCLIHost(cwd, loadNativeModule); | ||
| const workspace = { | ||
| isWorkspaceFile: true, | ||
| workspaceOrFolderPath: '/foo/bar', | ||
| rootFolderPath: '/foo/bar', | ||
| configFolderPath: '/foo/bar', | ||
| }; | ||
| const configFile = URI.file(path.resolve('./src/test/configs/example/.devcontainer.json')); | ||
|
|
||
| const configs = await readDevContainerConfigFile(cliHost, workspace, configFile, false, nullLog); | ||
| assert.isNotNull(configs); | ||
| assert.property(configs, 'config'); | ||
| assert.isNotNull(configs?.config.config); | ||
|
|
||
| const features = configs?.config.config.features as Record<string, string | boolean | Record<string, string | boolean>>; | ||
| assert.hasAllKeys(features, ['ghcr.io/devcontainers/features/go:1']); | ||
| }); | ||
|
|
||
| it('can resolve an "extends" file reference', async function () { | ||
| const cwd = process.cwd(); | ||
| const cliHost = await getCLIHost(cwd, loadNativeModule); | ||
| const workspace = { | ||
| isWorkspaceFile: true, | ||
| workspaceOrFolderPath: '/foo/bar', | ||
| rootFolderPath: '/foo/bar', | ||
| configFolderPath: '/foo/bar', | ||
| }; | ||
| const configFile = URI.file(path.resolve('./src/test/configs/extends/.devcontainer.json')); | ||
|
|
||
| const configs = await readDevContainerConfigFile(cliHost, workspace, configFile, false, nullLog); | ||
| assert.isNotNull(configs); | ||
| assert.property(configs, 'config'); | ||
| assert.isNotNull(configs?.config.config); | ||
| assert.isNotNull(configs?.config.raw); | ||
| const expectedConfig = { | ||
| name: 'Overrides', | ||
| image: 'mcr.microsoft.com/devcontainers/base:latest', | ||
| forwardPorts: [ 80, 443 ], | ||
| features: { | ||
| 'ghcr.io/devcontainers/features/docker-in-docker:1': { | ||
| 'version': 'latest', | ||
| 'moby': true | ||
| }, | ||
| 'ghcr.io/devcontainers/features/go:1': { | ||
| 'version': 'latest' | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| assert.deepEqual(configs?.config.raw as any, expectedConfig); | ||
| }); | ||
| }); |
Uh oh!
There was an error while loading. Please reload this page.