Add support for "extends" keyword#311
Conversation
greggroth
force-pushed
the
gregg/extends
branch
5 times, most recently
from
d16f0c2 to
bc71633
Compare
greggroth
force-pushed
the
gregg/extends
branch
from
bc71633 to
baf7a01
Compare
greggroth
force-pushed
the
gregg/extends
branch
from
baf7a01 to
e1d2532
Compare
| 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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, hostRequirements picks the highest while for "extends" using a naive deep-merge algorithm, it would pick the one in the referenced configuration irrespective of whether it's higher or lower. As a user of devcontainer without intimate knowledge of the code, I find that more intuitive and easy to predict. Has the proposed image metadata merge algorithm been implemented yet?
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 devcontainer up on a devcontainer.json with the image build from this devcontainer.json
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
👍🏻 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.
|
The Is there a chance that this will be merged soon? Would prefer to not have to work around it by building a separate Docker container for the base dev environment, since that would break (For what it's worth, Copilot thinks that this keyword was already merged in and is suggesting it to end users, which is how I found my way here) |
|
hi @joshspicer @Chuxel @greggroth is there any update here ? I'm also looking for such feature. this will definitively help a lot to allow developers of a same project to make some slights adjustements of the devcontainer.json |
|
hi @joshspicer @Chuxel @greggroth Any updates regarding this? will be very helpful for us. |
7 participants
An "extends" keyword that allows one devcontainer file to import another devcontainer file as a base configuration was proposed in devcontainers/spec#22. This PR aims to implement support for that feature.