-
Notifications
You must be signed in to change notification settings - Fork 313
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
Explore simplifying dev container definition with a concept of 'container features' #5442
Comments
Feedback from @Chuxel on the current implementation:
|
Update:
"capAdd": [
"SYS_ADMIN",
"NET_ADMIN"
],
"securityOpt": [
"label=user:USER",
"label=role:ROLE"
] |
//cc: @joshspicer @2percentsilk |
@chrmarti One thing I realized was missing from this list was the idea of grouping feature options - or more accurately, allowing one feature to have multiple options. One example of this is the |
To keep things simple, I'd prefer to have one version per feature. That also matches what NPM's package.json and the Linux package managers do, they have one version per package. So the If one feature depends on another feature, it might makes sense to simply install a suitable version of the dependency if none is specified. So if Gradle is to be installed, but Java is missing, we could just install the latest stable Java version. This again corresponds to what NPM and Linux package managers do and will result in a working installation. The user can later still refine the choice of the Java version. To avoid overwhelming the user with a very long list of features, we could also move less relevant features to a 'Show All' list, similar to what we do for definitions. E.g., Gradle and Maven could be shown on the initial list for the Java base image, but move to the 'Show All' list for all other base images. |
@chrmarti I don't think that works - features have multiple options. Another example is terraform. There's a TFlint version and TFGrunt version neither of which apply if you're not using Terraform. Scripts also have multiple options, like Docker-in-Docker supports using Moby or Docker, but again presenting that without first opting into installing Docker doesn't make sense. Python has options as to whether additional tools should be installed as does Go. Each of these are related to the primary feature. This seems to be driving complexity into scripting in a way that is not beneficial to customers - the complexity is still there. |
I'm not familiar with these particular tools, but if their relation to each other is similar to those I know from the JavaScript world: TFlint and TFGrunt are optional tools depending on Terraform and there might be alternatives like TFhint and TFGulp in the future. With that in mind, it makes more sense to not nest these optional features, but make them top-level and maybe have their dependencies represented in metadata. I think we need to be careful to not let the current granularity of the scripts drive the schema in the devcontainer.json. E.g.: {
"features": {
"terraform": "1.0.0",
"tflint": "0.31.0",
"tfgrunt": "0.31.8"
}
} We were also discussing using an array: {
"features": [
{
"id": "terraform",
"version": "1.0.0"
},
{
"id": "tflint",
"version": "0.31.0"
},
{
"id": "tfgrunt",
"version": "0.31.8"
}
]
} (I like the object notation better since that makes clear that the order doesn't matter and each feature can be added only once. It is also shorter.) |
If the features are split out, might there not be a case where order does matter? In which case, does the array syntax make sense? |
Also, the array syntax feels like it could open more options. E.g. for the {
"features": [
{
"id": "terraform",
"version": "1.0.0"
},
{
"id": "azure-cli",
"version": "1.0.0",
"mountHostConfig": true
},
]
} Of course, the object syntax could also be expanded: {
"features": {
"terraform": {
"version": "1.0.0"
},
"azure-cli": {
"version": "1.0.0",
"mountHostConfig": true
},
}
} |
@stuartleeks - i've been tracking what schema changes i've been proposing, and it's pretty similar to what you're saying here. https://github.com/microsoft/vssaas-planning/issues/5095 @chrmarti and I talked about it this morning. I prefer the array syntax for the main reason that it feels more flexible for the future- and given we're not really too firm on a lot of registry details, having a flexible |
@chrmarti I think that as we move forward towards Josh's idea of packages, these will be inside the same "feature". Furthermore, even if they're not, from a UI perspective, I don't want to present options that are not relevant unless they are needed. We should err on the side of user simplification over tech complexity given the goals here - which is inherently to abstract while still leaving full power available for those that want it. So, I'd want this ability in As long as we can represent this somewhere in such a way that UI implementations know how to present options, the specifics of how it ends up in devcontainer.json based on selections is less important to the end user. So really my comment here is about the features.json metadata (or whatever that evolves into). |
(re: @stuartleeks)
Where the order matters, it should be handled by our implementation. (Similar to how Debian's
Agreed, also (re: @joshspicer)
Could you give any examples of how the registry might require the array syntax? (Using an array for future use without assigning it any semantics now doesn't seem to make sense since by the time we do assign semantics, users would already have their arrays with some more or less random order.) (re: @Chuxel)
Thinking of features as packages makes me think that we should use a similar granularity as, e.g., Linux packages where each package has a single upstream version.
I have the UI in mind and think that we are not limiting ourselves. But we should discuss this here too. I think with the somewhat limiting QuickPick UI we use in VS Code, we might first show a list of features that do not require any other features (e.g., Terraform) and if the user's selection includes any dependencies for additional features, we would then show these additional features in a second list (e.g., TFlint and TFgrunt). This would also manage the length of the first list somewhat.
We can tune the metadata towards contributors while we tune the devcontainer.json towards users. E.g., multiple features in the metadata could be handled by the same script (if that helps). |
@chrmarti These packages are closer to Chocolatey which provide install options than it is to NuGet which backs Chocolatey. The version of the package here is actually the version of the installer not the thing being installed. It will be super critical that we do not expect people to create new versions of these packages every time there's an update to the underlying tool/sdk/runtime being installed. But at the same time, these scripts will have breaking changes - the analogy to a GitHub Action here isn't a bad one in this regard. Ultimately, if we have a good UI, what ends up in devcontainer.json is less critical. Fundamentally, we're invoking installer functions here. Installers provide arguments and there will be more than one. Let's stick with some examples. With the recent updates to Docker, I can see us needing to provide two options to the installer: Whether to use "Moby" rather than Docker and what version to use. Two options, same feature. There's similar things in all the scripts about whether to install supplemental tools. Once again, my concern here is passing through the complexity here in a way that does not provide us the ability to group these options in a logical way. Stuart's In each of these cases, we'd want to be able to allow someone to select the feature, and then pick options. If that ends up as flat properties in devcontainer.json, ok - though we need to discuss how features actually identify what applies to them. |
It's important to keep in mind that there are two audiences: The larger audience are the developers writing a devcontainer.json for their project. We want to keep complexity away from them as much as possible. The smaller audience are the contributors writing their own features with metadata that we currently capture in the internal features.json. We will want to give them as much flexibility as they need to implement their features and they will work with the complexity that comes with that.
I think it's important that we keep the devcontainer.json simple as that will be the file most users will work with at some point. There will be many devcontainer.json files in user repositories and we will support existing ones moving forward, so we need to make sure we pick the right amount of flexiblity. The UI we can improve and rewrite any time and we already know that the existing UI using VS Code's QuickPick comes with its advantages thanks to its simplicity, but also its limitations due to that.
My proposal is to have: "azure-cli": "1.0.0" As a shorthand for: "azure-cli": {
"version": "1.0.0"
} Allowing for additional options like: "azure-cli": {
"version": "1.0.0",
"mountHostConfig": true
} The version property would be the Azure CLI's version. We can support specifying the installer's version similarly to GitHub Actions, but that should only be needed by CI engineers requiring full control. Most developers will want to use the latest version of the installer. |
@chrmarti Totally good with that. How do we use |
I would argue for the I still feel [
"azure-cli"
] is cleaner than json
It's a bit more verbose if you do want to supply extra config to the feature, but sometimes you won't have to, in which case a single string would do. Other devcontainer fields like Re: flexibility I still also think theres a possibility this could be a valid collection of features in the future: [
{
"id": "node",
"version": "14"
},
{
"id": "node",
"version": "16"
},
"azure-cli"
] In this case, the object notation restricts us. |
@joshspicer Azure CLi is just an example of the general idea. No matter what, each script should include a "latest" version so you don't need to know (and most do already). The real question I had is how we represent scenarios where a feature has multiple options in |
(re: @joshspicer )
This opens questions like: Does the order matter? Which version will be on the PATH? How do you switch between versions? Which features support installing multiple versions? I prefer solving this with existing solutions like (re: @Chuxel )
We can add options to each feature, but I wouldn't add feature-like options. E.g., TFlint and TFGrunt should be features, not options to the Terraform feature. I'm not fully convinced we need options in a first implementation and I don't see the current UI scale to options (I can't imagine users hopping through a growing list of QuickPick steps). So if we support options in metadata and the devcontainer.json, we also want to have defaults for those options, so the UI doesn't have to present them. |
@chrmarti Re: Python, the script builds from source to enable other versions of Python. This is what |
@joshspicer I have updated the extension's code to run everything from "version": {
"type": "string",
"proposals": ["latest", "1.0", "2.0"],
"default": "latest",
"description": "Version option."
} (Use @Chuxel For prebuilt images, building from source for flexibility makes sense and the performance impact is in our CI. For features I'd imagine the default should be to install from prebuilt packages to avoid the delay for building from source on the user's side. If features are used as add-ons (as opposed to the primary technology stack used), using the latest prebuilt package might just be what users expect. |
Previously added |
@chrmarti Just to check I tried updating the docker-in-docker feature as follows and am not seeing it appear when I ran a quick check. I don't see the "moby" option presented in the UX. "id": "docker-in-docker",
"name": "Docker (Moby) support (Docker-in-Docker)",
"options": {
"moby": {
"type": "boolean",
"default": true,
"description": "Install OSS Moby build instead of Docker CE"
}
},
"entrypoint": "/usr/local/share/docker-init.sh",
"privileged": true,
"init": true,
"containerEnv": {
"DOCKER_BUILDKIT": "1"
},
// ... I also tried: //...
"options": {
"moby": {
"type": "string",
"enum": ["true", "false"],
"default": "true",
"description": "Install OSS Moby build instead of Docker CE"
}
},
// ... Similar things happened when I tried multiple properties. e.g. adding a flag as to whether node-gyp dependencies should be installed for the "id": "node",
"name": "Node.js (via nvm) and yarn",
"options": {
"version": {
"type": "string",
"proposals": [ "lts", "latest","16", "14", "12" ],
"default": "latest",
"description": "Select or enter a Node.js version to install"
},
"nodeGypDependencies": {
"type": "string",
"enum": [ "true", "false"],
"default": "true",
"description": "Install dependencies for compiling native Node.js packages (node-gyp)?"
}
} |
I made updates assuming only I made a few polish fixes and hit a bug that I resolved. Take a look @joshspicer @2percentsilk and see if there's anything else you want to tweak! |
The QuickPick UI only offers "version", only IntelliSense has all the options. Since our goal is to have the checkboxes to get users set up and running, I think we should also remove the "version" picker from the UI and just always use "latest". |
@chrmarti Got it, so currently we can add them and they'll be picked up in json?
@chrmarti I strongly disagree with this part. Our goal is actually to provide a yeoman-like experience in the UI with the ability to do advanced things in json. The problem here again is that people need the option to pin to a version. The way the UI is now with the version picker being available makes that really easy to do which is great - so I don't think we should take this away. This is a major help in discoverability. It has the same advantages that the settings UI does despite the fact that everything is instrumented in the json editor too. Also, as we add in remote features, will those work in the json editor? I'd like to expose additional options in the UI, but I'd like to hear @2percentsilk's thoughts here too. |
@Chuxel Correct, additional options will be available in JSON and suggested in IntelliSense. I see you've moved all |
I'll move these over to make sure I have a good grasp on the new syntax |
Is it required for one of the options to be I suppose the shorthand then would be broken like you're mentioning. I guess that means authors need to add in a version to their feature's features.json entry? Seems like an unnecessary restriction IMO |
Please let me know if anything else should be tweaked in the I added in a |
@chrmarti lol. As if to accentuate the importance of version selection, we've had two breaks today that could be solved with pinning to a specific version.
We're finding these in CI, which is great, but if we don't have the ability to recommend people pin to specific releases rather than "latest", this will keep happening. (We also need to recommend pre-building images since that side-steps this entirely, but that's a separate topic.) |
@joshspicer |
@joshspicer I see a few cases (e.g., PowerShell) where a version makes sense even if we currently don't offer any but the "latest". For these I recommend keeping the version, so we can add versions in the future without changing the schema from boolean to string. |
Available without experimental setting with Remote-Containers 0.200.0. Closing this issue, let's split out new issues for the remaining tasks. |
The goal of this feature is to address the complexity of adding simple tools to an existing dev container configuration. Currently this can only be done with sufficient know-how about writing a Dockerfile to get additional tools included.
The proposal to simplify adding tools to an existing dev container configuration is to have a
"features"
property listing any additional tools (from a given list of available tools) that should be installed. E.g., the following would add Node.js 14 and the GitHub CLI:A first implementation of this is adding the features by building an additional features image based on the original dev container image. The dev container is then created from this features image. The original image can be from an
"image"
,"dockerfile"
or"dockerComposeFile"
configuration.This issue continues a previously internal discussion.
The text was updated successfully, but these errors were encountered: