Closed philliphoff closed 4 years ago
@philliphoff thank you for putting this together, great proposal overall. A couple of suggestions:
Consider specifying in more detail what kind of capabilities the command templates provide. E.g. are they JavaScript template literals under the covers, and can the user do some math or string manipulation when command templates are evaluated?
Regarding multiple template commands, I would separate what the user might see when prompted to select a command, from the values used by template selection heuristics.
The value used for prompting the user would be stored in a separate property (e.g. name
; mandatory if multiple templates are used).
For each heuristic match there would be a separate, optional property. For example, for docker logs
command, user could specify matchTag
and matchContainerName
properties.
This has several advantages. First, the name of the template can be made as easy to understand as necessary. Second, if the user does not desire to use the heuristics, they can do so by omitting match*
properties. And the match properties can be made regular expressions for extra flexibility.
HTH!
@karolz-ms Thanks for the feedback!
Consider specifying in more detail what kind of capabilities the command templates provide. E.g. are they JavaScript template literals under the covers, and can the user do some math or string manipulation when command templates are evaluated?
I intended the tokens to be similar to VS Code's notion of tokens in the tasks.json
and launch.json
files (e.g. ${workspaceFolder}
), which are generally just substituted values. I did not intend on allowing any sort of dynamic evaluation; I'd need to see some practical examples of how that would be useful given the added complexity.
Regarding multiple template commands, I would separate what the user might see when prompted to select a command, from the values used by template selection heuristics.
I considered this (and am not opposed to a separate match
property), but thought it might be over-complicating the 80% case. That said, such a property being an actual regular expression is a good idea that doesn't add a significant amount of complication over a simple string match. I'm a bit torn over whether each command should have a different set of match properties for each step in its heuristic (as you suggest) as opposed to just one match
property that's applied to each case until a match is made. The latter seems like a cleaner schema, and the fact that there's an order to the heuristic doesn't feel like it meshes well with peer-level properties.
Perhaps we could do a (limited) emulation of VS Code's command's when
property syntax:
{
"docker.commands.logs": [
{
"label": "Match Redis Containers",
"when": "image == redis",
},
{
"label": "Match Redis-like-named Containers",
"when": "name ~= /redis/"
}
]
}
I don't really want to go down the path of supporting complex statements like VS Code, however (e.g. boolean expressions), so I think I'd want to limit it to just one equality/match expression.
@philliphoff makes sense to do simple substitution on tokens in the command; I see the note on that in the proposal 👍
With regards to matching, the only thing that I would emphasize is the separation of the label and the match property/properties. I do believe it will save us quite a bit of grief down the line.
Having a single match
property with well-known area of application per command seems fine. We could also do the limited when
property emulation like you described, but given that we only plan to do a literal or regex match, the match
property exposes the functionality in more direct and understandable way IMO.
Coming from #1496, @philliphoff, I think that this is an interesting proposal in the good direction, but I'm afraid that it does not face the issues that force me to keep using Docker's CLI. Having some templates defined in settings.json
is good, but the proposed flexibility falls short for my requirements.
Tha main idea in the screencast shown in #1496 is that templates are first-class citizens in the GUI, just as containers, images and registries. This is because of how thoughts come to the user. Sometimes users want to run a specific container and need to guess/try which options to use. However, in other contexts, users know what options they need to use, but they need to guess/try multiple containers.
In the current proposal, it feels that templates are hidden in the GUI, as these are defined in the settings.json
. Also, because of match
and heuristics, it seems that the target is for VSC (the extension) to decide which is the better template for a given container/image. While I understand there might be a target audience for such a work flow; I'd like an interface that makes it easy for me to take decisions, not one that takes them for me.
From this point of view, I don't think the syntax for saving the templates is relevant, as long as GUI features are provided to manipulate them easily. Nonetheless, I believe that using arrays instead of a single string might make it easier to work with:
{
"label": "Build with Node 12",
"template": [
"docker",
"build",
"-f",
"${dockerfile}",
"-r",
"${tag}",
"--build-arg",
"NODE_VERSION=12.14.1",
"${context}"
]
}
BTW, it would make sense to have configurations defined declaratively:
{
"label": "Build with Node",
"template": [
"docker",
"build",
"-f",
"${dockerfile}",
"-r",
"${tag}",
"--build-arg",
"NODE_VERSION=${nodeVerion}",
"${context}"
],
"configurations": {
"12": {
"nodeVersion": "12.14.1",
},
"13": {
"nodeVersion": "13.7.0",
}
}
}
The "configurations" object can be updated with "latest used configurations" dynamically. However, this strategy might conflict with the match
strategy.
The
<dockerfile>
value is the workspace-relative path of the selected Dockerfile.
I think that absolute paths should also be supported.
The
<context>
value is derived from the docker.imageBuildContextPath setting or, if not set, defaults to the workspace-relative folder in which the Dockerfile resides.
IMHO, this (and any other parameter) should be optionally customizable from the GUI, just before executing the command.
docker run --rm -d ${exposedPorts} ${tag}
I believe that:
--rm
should be optional-d
/-t
/-it
should be a select option for a single command. I.e. docker.commands.runInteractive
should be merged as docker.commands.run('interactive')
. Other options would be docker.commands.run('daemon')
and docker.commands.run('tty')
. Naturally, it would be possible for a configuration to define a default value for this option.${shellCommand}
should be supported.By the same token, ${exposedPorts}
should be renamed to ${additionalOpts}
and it should be allowed to provide a list/array of additionalOpts, not one only. Hence, docker.commands.runAzureCli
would be an alias of a configuration with two additionalOpts: network
and userVolumes
.
Actually, I think that this is the main limiting factor for my use cases. My main requirement is to run images with exposed volumes and ports which are not defined in the Dockefile/image, and which are mostly a one-time setting. The number of shared volumes and ports is not fixed.
docker exec -it ${containerId} ${shellCommand}
As above, I think that -it
should be a select option. A the same time, ${additionalOpts}
should be supported. Additional options in this case are --privileged
, --user
and/or --workdir
. This is useful, for example, to install some missing packages as root in a container that is being executed as a non-root user.
Of course, I completely agree with your proposal being a great improvement compared to the current state. Please, take these comments as constructive criticism about where I'd like to go, not as a demand.
Another issue I forgot to comment is that the main command might not be docker
. In order to run containers with GUI apps, x11docker --runx -i [--user=0] -- [-v /$(pwd)://src] [-p 5000] -- <image_name> bash
can be used. In this case, [-v /$(pwd)://src] [-p 5000] -- <image_name> bash
is equivalent to ${userVolumes} ${exposedPorts} -- ${tag} ${shellCommand}
. The difference would be that, in the template, docker run --rm -it
is replaced with x11docker --runx -i [--user=0] --
. Note that I am ok with customisation of x11docker options not being supported.
@umarcor Thank you for your thoughts!
--rm
/ -d
/ -t
/ -i
/ etc. :smile:docker
/ docker-compose
--I agree! I don't see a good reason why we should not allow any/nearly any arbitrary command line, even if it makes no sense. For example, I don't think we should block users from making a docker run
template that is actually echo 'Hello world!'
, even though it would not be useful. But, that means you can do useful things like x11docker --runx...
and more.@umarcor Thank you for your thoughts!
Thank you for building/maintaining this piece of software! This extension was one of the main reasons that pushed me to start using VSC. It is not as good as I'd like yet, but the layout, and the UX were really good from the begining. It was easy to visualise how it can be non-intrusively extended.
- I agree that a GUI for managing these templates would be extremely helpful. I don't think that we'll have time to do that ahead of the 0.11.0 milestone, but either way I agree that that should be added. If we don't get it in 0.11.0, I'll be sure to file another work item for it.
I think this is important, and it should not be forgotten, as you say. Nevertheless, it is not prioriraty at all. Neither for v0.11.0, nor for v0.12.0 or... I'd prefer to have the GUI well thought than quickly done.
In case you didn't see it yet, this is a prototype I did: https://user-images.githubusercontent.com/38422348/70940519-cdc30400-204a-11ea-91cd-260f97054cb2.gif I didn't implement it as a PR because I could not build this extension. However, I found my limit with regard to VSC's API and internal feature set. Hence, I'd be glad to somehow contribute to have that reused, should it be reusable at all.
- Agree that anytime we ask users to pick between multiple templates, the latest used one should be selected by default (I think VSCode takes care of that internally, actually)
Note that when I said that latest configurations can be updated dynamically, I meant to have them added, not selected. So, if I select a base template/configuration and I choose to customise some field before launching it, that should/could be added to the list of known/existing configurations for that template.
Maybe this needs some more elaboration. With customise I mean two possibilities:
execute
and customise and execute
.In both cases, the interaction would happen through the dropdown menu in the center (I don't know how is it called). Actually, this text-menu based interaction can be a temporal workaround for the lack of a GUI.
- With the templating, every parameter / flag / etc. becomes optional, not just
--rm
/-d
/-t
/-i
/ etc. 😄
Glad to know!
- For example, I don't think we should block users from making a
docker run
template that is actuallyecho 'Hello world!'
, even though it would not be useful.
Agree!
Ah, I think I understand what you mean now. Would it make sense to separate "Customize" from "Execute", i.e. what users do to "Execute" doesn't really change from what there is today, but add a command for "Customize", that could open the config UI or take them to the settings.json, etc.?
I hit one speed bump during implementation with the compose commands. The extension allows for no compose file to be specified, in which case it just runs docker-compose up/down etc.
with no -f "someFile.yaml"
. This means the template needs to be able to include / exclude that -f
. I added another template variable, ${configurationFileSpecified}
, to allow for that; I also edited @philliphoff's original comment above to include that. I'm not too keen on that name so I'm open to other suggestions.
@bwateratmsft Alternatively we could have two settings/command templates, one used when compose file is specified, the other when it is not
Another option would be to put the -f
in front of the file, if needed. I'm reluctant to introduce any more settings (i.e., split into two command templates) because these configs are incredibly heavy weight.
Yeah, as I think about it that makes more sense.
Ah, I think I understand what you mean now. Would it make sense to separate "Customize" from "Execute", i.e. what users do to "Execute" doesn't really change from what there is today, but add a command for "Customize", that could open the config UI or take them to the settings.json, etc.?
Sure!
Overview
The Docker extension executes a number of Docker CLI commands on behalf of its users, such as to build images, run containers, attach to containers, and view container logs. Some of these commands have a large number of optional arguments, often used in very specific scenarios, and our users have stated a need be able to augment or override arguments passed to commands executed by the extension.
To date, the ability to customize the commands executed by the extension has been added piecemeal, with the customization often just appended to the command line generated by the extension. While this is often sufficient, it doesn't allow the user to significantly alter the command, for example, to change arguments or flags set by the extension itself, which may be necessary in a number of user scenarios.
This proposal is intended to:
Non-goals
Some Docker interaction is performed via an SDK rather than via the command lines. For example, container start/stop/restart/remove/inspect/prune. Customization of those commands may be considered in a future proposal as it would likely require a different customization scheme, if and when there is demand for it.
Related issues (not an exhaustive list)
794
807
1197
1274
1349
1505
1590
1593
General Proposal
For each Docker command (and variant), for example, build, run, attach, and view logs, the extension will:
Define a configuration setting that represents a "template" of the command line to be executed.
The default value of this setting will represent a complete command line for that Docker command (generally corresponding to that currently generated and executed by the extension).
The setting may support a set of "tokens" that represent values generated by the extension, which, if present, are substituted with their representative value at the location of the token within the template.
The set of tokens supported by a template may vary based on the Docker command.
More than one template may be defined for each command; in such cases, each
template
property may be associated with alabel
and/or amatch
property.If a
match
property is specified, those commands supporting heuristic-based selection will use that value to match against values appropriate to the command (for example, the container or image name).If multiple templates specify a
label
property and no template was otherwise selected based on theirmatch
properties, those templates will be included in the list (built from the templates'label
properties) from which the user will be prompted to select a template.If no templates were selected based on their
match
properties and fewer than 2 remaining templates have an associatedlabel
property, the first such template will be selected.If no templates were selected based on their
match
properties and no other templates have been specified, then the default template for that setting will be selected.Settings JSON Schema
VS Code will prompt the user to edit the
settings.json
file when adding or editing these settings. Customizing a command, at a minimum, is just assigning the corresponding setting name a string value that represents the Docker command line to execute. For example, to customize the Docker build command to set the build argumentNODE_VERSION
to12.14.1
:In cases where a single template is not sufficient, multiple templates can be specified by assigning the corresponding setting name an array of template objects. For example, to provide two customizations of the Docker build command, to set the
NODE_VERSION
build argument to differing values:In that scenario, upon invoking the build command, the extension will prompt the user to select a template to use from a list of the template labels.
Suppose the user wants to show all logs for Redis containers but otherwise limit logs for other containers to the last 10 lines. The user could define two templates, one matching
redis
and another template that serves as the default for all others (by not specifying amatch
orlabel
property).Docker Build
Current Behavior
When building an image, the following command is used:
The
<dockerfile>
value is the workspace-relative path of the selectedDockerfile
. The<context>
value is derived from thedocker.imageBuildContextPath
setting or, if not set, defaults to the workspace-relative folder in which theDockerfile
resides.The
<tag>
value is ultimately entered/confirmed by the user when invoking the build command. However, if previously built, it defaults to the previously-entered value for thatDockerfile
. Otherwise, it defaults to<folder>:latest
where<folder>
is the parent folder of theDockerfile
.Proposed Behavior
The new behavior will be to offer the following setting:
docker.commands.build
docker build --rm -f "${dockerfile}" -t ${tag} "${context}"
This setting will support the following tokens:
${context}
docker.imageBuildContextPath
setting. Otherwise, the workspace-relative folder in which theDockerfile
resides.${dockerfile}
Dockerfile
.${tag}
Dockerfile
. Otherwise, defaults to<folder>:latest
where<folder>
is the parent folder of theDockerfile
.Docker Run
Current Behavior
When running an image, the following commands are used:
The
<tag>
value is the full tag of the selected image. The<ports>
value is generated from the list of exposed ports in the image (i.e. ultimately from theDockerfile
), where each exposed port is mapped to the same port on the local machine. For example,"EXPOSE 5000 5001"
would generate"-p 5000:5000 -p 5001:5001"
.For the Azure CLI, the
<network>
value is--net=host
on Linux but not set on other platforms. The<volumes>
value is a mapping of volumes in the container to the user's local.azure
,.ssh
, and.kube
folders (so simplify remote connections to Azure resources). For example,"-v /Users/<user>/.azure:/root/.azure -v /Users/<user>/.ssh:/root/.ssh -v /Users/<user>/.kube:/root/.kube"
.Proposed Behavior
The new behavior will be to offer the following settings:
docker.commands.run
docker run --rm -d ${exposedPorts} ${tag}
docker.commands.runInteractive
docker run --rm -it ${exposedPorts} ${tag}
docker.commands.runAzureCli
docker run ${network} ${userVolumes} -it --rm azuresdk/azure-cli-python:latest
These settings will support the following tokens:
${exposedPorts}
Dockerfile
), where each exposed port is mapped to the same port on the local machine. For example,"EXPOSE 5000 5001"
would generate"-p 5000:5000 -p 5001:5001"
.${tag}
${network}
--net=host
. Not set for other platforms.${userVolumes}
.azure
,.ssh
, and.kube
folders (so simplify remote connections to Azure resources). For example,"-v /Users/<user>/.azure:/root/.azure -v /Users/<user>/.ssh:/root/.ssh -v /Users/<user>/.kube:/root/.kube"
.Template Matching
For non-Azure CLI run commands, if there are multiple templates associated with the command, the extension will first look for a template with a
label
property matching the tag of the image being run. If found, that template will be used. If not found, then the user will be prompted to select from the set of templates.Docker Attach
Current Behavior
When attaching to a running container, the following command is used:
The
<shellCommand>
value is derived from the value of either thedocker.attachShellCommand.windowsContainer
ordocker.attachShellCommand.linuxContainer
settings.The defaults for those settings are:
docker.attachShellCommand.linuxContainer
/bin/sh -c \"[ -e /bin/bash ] && /bin/bash || /bin/sh\"
docker.attachShellCommand.windowsContainer
powershell
Proposed Behavior
The new behavior will be to offer the following setting:
docker.commands.attach
docker exec -it ${containerId} ${shellCommand}
This setting will support the following tokens:
${containerId}
${shellCommand}
docker.attachShellCommand.linuxContainer
ordocker.attachShellCommand.windowsContainer
setting, as appropriate.Template Matching
If there are multiple templates associated with the command, the extension will first look for a template with a
label
property matching the name of the container being attached to. If found, that template will be used. If not found, the extension will then look for a template with alabel
property matching the tag of the image of the container being attached to. If not found, then the user will be prompted to select from the set of templates.Docker Logs
Current Behavior
When viewing logs of a container, the following command is used:
Proposed Behavior
The new behavior will be to offer the following setting:
docker.commands.logs
docker logs -f ${containerId}
This setting will support the following tokens:
${containerId}
Template Matching
If there are multiple templates associated with the command, the extension will first look for a template with a
label
property matching the name of the container being attached to. If found, that template will be used. If not found, the extension will then look for a template with alabel
property matching the tag of the image of the container being attached to. If not found, then the user will be prompted to select from the set of templates.Docker Compose Up
Current Behavior
When bringing up a composition, the following command is used:
The
<file>
value is the workspace-relative path to the selected Docker Compose YAML file. The<detached>
value is set to-d
if the settingdocker.dockerComposeDetached
is set totrue
. The<build>
value is set to--build
if the settingdocker.dockerComposeBuild
is set totrue
.Proposed Behavior
The new behavior will be to offer the following setting:
docker.commands.composeUp
docker-compose ${configurationFile} up ${detached} ${build}
This setting will support the following tokens:
${configurationFile}
-f
plus the workspace-relative path to the selected Docker Compose YAML file.${detached}
-d
if the settingdocker.dockerComposeDetached
is set totrue
. Otherwise, set to""
.${build}
--build
if the settingdocker.dockerComposeBuild
is set totrue
. Otherwise, set to""
.Docker Compose Down
Current Behavior
When taking down a composition, the following command is used:
Proposed Behavior
The new behavior will be to offer the following setting:
docker.commands.composeDown
docker-compose ${configurationFile} down
This setting will support the following tokens:
${configurationFile}
-f
plus the workspace-relative path to the selected Docker Compose YAML file.