Add support of devcontainer.user.json file

[aacebedo]

This PR adds the support of an additional file named devcontainer.user.json or .devcontainer.user.json.
The latter is combined with the devcontainer.json or the .devcontainer.json before actually building the container.

It can be useful when one wants to customize the devenv of a project without modifying the file under revision (for instance when new user specific mounts shall be added to the devenv).

Note: I used an additional dep to perform the merge between the two files (mergo)

[aacebedo]

@pascalbreuninger can you have a look please?

[pascalbreuninger]

Hi @aacebedo, thanks for the PR!
I understand the desire to overwrite the base devcontainer definition and we were also thinking about this for a while.
The implemented approach here requires users to create an extra file in the source repository that then probably needs to be ignored by git to prevent users from overwriting one another

If we go down that route it should probably be any devcontainer.json you have available locally without being in the same repository. What do you think?

[aacebedo]

Hi @aacebedo, thanks for the PR!
I understand the desire to overwrite the base devcontainer definition and we were also thinking about this for a while.
The implemented approach here requires users to create an extra file in the source repository that then probably needs to be ignored by git to prevent users from overwriting one another

If we go down that route it should probably be any devcontainer.json you have available locally without being in the same repository. What do you think?

In fact I originally considered several approaches:

• add new arguments to the command line: I was not a fan because of the complexity and the number of additional args to add to handle all sections of the file
• add the automatic aggregation of a user file (solution implemented here)
• add a new argument to specify an additional json file to use while calling up command: it could be a viable and more flexible solution.

What do you think of merging the two last approaches?
By default use a specifically named file located in the same dir as the devcontainer.json file AND add an optional arg to override this.
Shall we make this arg multiple value or only one?

[aacebedo]

rebased on main and test E2E tests. It is now working

[pascalbreuninger]

@aacebedo merging the two sounds good to me!
Something like devpod up ... --extra-devcontainer-path=$YOUR_LOCAL_FILE.
One change we might want to make is instead of merging the files literally, leverage the devcontainer specs overlaying mechanism via config.MergeConfiguration. Right now it extracts the underlying devcontainer config, i.e. from features and the base image etc, from the image and then merges the repos configuration on top.

Do you want to tackle this or should we put this on our roadmap?

[aacebedo]

@pascalbreuninger I checked config.MergeConfiguration and I don't think it can be used.
The function takes ImageMetadata array and not DevContainerConfig to merge it. It means that some elements (image and dockerfile for instance) cannot be overridden by other files.

So either we say that it is not possible to override those parameters and I can modify it, or we say it is possible and then I cannot use that.

[pascalbreuninger]

Yep you're right. I was more referring to the overall mechanism of merging two devcontainer files that goes beyond overlaying two files.

We'd need to run both devcontainer through substition, then determine a priority of image, i.e. user > default and then merge the resulting devcontainer configs.
Right now the config.MergeConfiguration doesn't support merging two devcontainer definitions and only one "real" devcontainer with the devcontainer metadata found on an image. We can always change that as long as well adhere to the spec 🤷‍♂️

[aacebedo]

I hacked something by touching as less as possible to the mergeConfiguration.
I pushed it as a fixup. Tell me what you think.

[pascalbreuninger]

LGTM based on first glance 👍
Users can't overwrite everything but it's at least a good start. You've mentioned you have some people waiting for this. Do you know what they'd most likely want to overwrite/change in their devcontainer.user.json file?

[aacebedo]

LGTM based on first glance 👍 Users can't overwrite everything but it's at least a good start. You've mentioned you have some people waiting for this. Do you know what they'd most likely want to overwrite/change in their devcontainer.user.json file?

OK I'll clean up and squash the fixup then.

The thing that is most wanted is to be able tu customize mounts without having to modify the versionned devcontainer.json

[pascalbreuninger]

Okay, then that would work. One more thing we'll (you or me) have to clean up is that right now it expects paths to be present on the local fs but reads them in a place that could potentially be in a VM, leading to broken paths. I'll fix that next week

[aacebedo]

Okay, then that would work. One more thing we'll (you or me) have to clean up is that right now it expects paths to be present on the local fs but reads them in a place that could potentially be in a VM, leading to broken paths. I'll fix that next week

ok I,'ll let you do it then as I am not familiar with the VM usage. Thanks

[aacebedo]

squashed the fixup. @pascalbreuninger I let you do the checks

[pascalbreuninger]

@aacebedo just a quick update: I haven't forgotten about this, it's just been a bit hectic this week

[aacebedo]

@aacebedo just a quick update: I haven't forgotten about this, it's just been a bit hectic this week

ok no problem. Ive rebase on main

[bkneis]

@aacebedo thanks for the contribution. In general I like your implementation and think it would add value to devpod. One thing I'm not sure on though is the user file. We have --extra-devcontainer-path in addition to the .devcontainer.user.json, I think instead we should simply favour the extra files and have the user file be one of them, not a specific named file since this separates us from the .devcontainer.json spec. Also I assume the username would change for each env, if these were committed everyone can name there overrides whatever they want then simply add them to the extra files arg list.

[aacebedo]

@aacebedo thanks for the contribution. In general I like your implementation and think it would add value to devpod. One thing I'm not sure on though is the user file. We have --extra-devcontainer-path in addition to the .devcontainer.user.json, I think instead we should simply favour the extra files and have the user file be one of them, not a specific named file since this separates us from the .devcontainer.json spec. Also I assume the username would change for each env, if these were committed everyone can name there overrides whatever they want then simply add them to the extra files arg list.
Thanks for the review @bkneis !

I didn't plan to make the username change. I planned to use the file as a default non versioned file that a user could add or not depending on their preference. I liked it being a special default case taken only when it exists.

In a previous message @pascalbreuninger told me to keep the two approaches (devcontainer.user.json + extra paths). However if both of you prefer to only have the extra paths approach, I can remove the .user file.

[bkneis]

@aacebedo ah OK I can see that workflow making a lot of sense, adding the user file to a gitignore and having as a personalised override would be sweet. If @pascalbreuninger is also for it then so am I :) I think we'll have to add to the docs to make this file known to users, especially with it not being in the devcontainer spec. If it is popular with the community maybe we can look at putting upstream in the spec one day

[aacebedo]

ping?

[aacebedo]

@pascalbreuninger ?