Support all DOCKER_* environment variables as VS Code settings
#998
Conversation
as VS Code settings
src/tree/LocalRootTreeItemBase.ts
Outdated
| @@ -293,8 +294,12 @@ export abstract class LocalRootTreeItemBase<TItem extends ILocalItem, TProperty | |||
| } | |||
|
|
|||
| private async getSortedItems(): Promise<TItem[]> { | |||
| const items: TItem[] = await this.getItems() || []; | |||
| return items.sort((a, b) => a.treeId.localeCompare(b.treeId)); | |||
| if (ext.dockerodeError === undefined) { | |||
There was a problem hiding this comment.
Why have this attached to the extensionVariables rather than just bubbling the error up from refreshDockerode?
There was a problem hiding this comment.
Because refreshDockerode gets called in two places:
- On extension activation
- Every time you update a docker setting
I feel like showing an error in either of those cases is too aggressive and would get annoying. I would rather delay the error and show it in a place that's actually relevant (aka the tree).
There was a problem hiding this comment.
I just don't like that it has to be manually reset to undefined. I could see a case where the error is stale or not attached to the right tree node. What happens if you update a bunch of docker settings at once??
There was a problem hiding this comment.
This is the current experience
This is what you might get if I showed the error in refreshDockerode:
I'm open to suggestions for refactoring the code, but I definitely don't want to change the user experience.
I could see a case where the error is stale or not attached to the right tree node.
Worst case the user has to hit refresh and it won't be stale any more. I don't think it can be attached to the wrong tree node. This logic is unique to LocalRootTreeItemBase, which is the root node for all views (containers, images, volumes, etc.)
What happens if you update a bunch of docker settings at once??
It works? I'm not entirely sure what your concern is.
There was a problem hiding this comment.
I was wondering if you could have errors from one treeItem and have it affect another valid treeItem since it's only checking of the ext.dockerodeError is equal to undefined. But I'm not familiar with the code flow for this, so if it doesn't seem like a realistic scenario, I'm totally fine with dropping it. Just wanted to get the reason since it's not a pattern I've seen you use before.
src/utils/addDockerSettingsToEnv.ts
Outdated
| addDockerSettingToEnv("machineName", 'DOCKER_MACHINE_NAME', env); | ||
| } | ||
|
|
||
| function addDockerSettingToEnv(settingKey: string, envVar: string, env: {}): void { |
There was a problem hiding this comment.
So VS Code settings are respected above env settings that may currently exist? For instance, DOCKER_HOST would get overridden if I had the host setting in my project?
There was a problem hiding this comment.
Yeah, that's what users requested. They might have the environment variable set on their machine, but they want VS Code settings to override that on a per-workspace level
There was a problem hiding this comment.
It definitely makes sense, but I was wondering if we should alert users in the output that their env variable is getting overwritten. I could totally see myself forgetting I had a setting and wondering why my environment variables weren't being used 😅
Previously we only supported "docker.host" (aka "DOCKER_HOST") as a VS Code setting. I added support for the 4 main environment variables used in docker machine. Users want these as VS Code settings so that they can have per-workspace setups and so that they can modify a setting without reloading VS Code or changing their environment variables.
Also we were parsing "docker.host" ourselves in the past, which led to a lot of problems because our parsing logic was very simplistic. Instead, I will let docker_modem (a dependency of Dockerode) do all the parsing for us. See here for more info.
Fixes #962
Fixes #914
Fixes #649
Fixes #580
Fixes #216
Related to #646