-
Notifications
You must be signed in to change notification settings - Fork 1k
Automatically get docker socket path (and fix socket path config in vite plugin) #10061
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
Conversation
🦋 Changeset detectedLatest commit: 95c381a The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Failed to automatically backport this PR's changes to Wrangler v3. Please manually create a PR targeting the Depending on your changes, running Notes:
|
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
wrangler
commit: |
974cf07 to
7c37a6e
Compare
vicb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM,
Added some inline comments
.changeset/full-pugs-say.md
Outdated
| "wrangler": patch | ||
| --- | ||
|
|
||
| feat: try to automatically get path of docker socket |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few questions:
Should "feat" imply a minor release?
Should "docker socket" be "container engine socket" (and use "container engine" below and in the other changeset rather than "docker" or "container tool")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
our general rule of thumb apparently is features on experimental products get releases as patches, so i'll probably keep this as is.
good point on the docker specific language, i'll update the changesets
.changeset/full-pugs-say.md
Outdated
|
|
||
| Currently, if your container tool isn't set up to listen at `unix:///var/run/docker.sock` (or isn't symlinked to that), then you have to manually set this via the `dev.containerEngine` field in your Wrangler config, or via the env vars `WRANGLER_DOCKER_HOST`. This change means that we will try and get the socket of the current context automatically. This should reduce the occurrence of opaque `internal error`s thrown by the runtime when the daemon is not listening at `unix:///var/run/docker.sock`. | ||
|
|
||
| You can still override this with `WRANGLER_DOCKER_HOST`, and we also now read `DOCKER_HOST` too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| You can still override this with `WRANGLER_DOCKER_HOST`, and we also now read `DOCKER_HOST` too. | |
| In addition to `WRANGLER_DOCKER_HOST`, `DOCKER_HOST` can now also be used to set the container engine socket address. |
| return resolve(stdout.trim()); | ||
| }); | ||
| }); | ||
| export const runDockerCmdWithOutput = (dockerPath: string, args: string[]) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity what what this changed from async to sync? (not mentioned in the PR description).
Does it deserves a comment explaining why it should not be async?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the unstable_getMiniflareWorkerOptions api needs to use this to resolve the socket path, and that function is sync (and part of wranglers public api surface) so i wanted to avoid changing that.
| return randomUUID().slice(0, 8); | ||
| } | ||
|
|
||
| type DockerContext = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this is parsed from docker context ls, maybe add a comment.
Would it be worth adding some validations (zod or whatever) on top of JSON.parse
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't really want to add zod just to validate this one thing, if it fails we have a sensible fallback. i mean, i would love to add zod to validate very many things across wrangler, but this probably isn't the place to start 😅
but comment added though :)
| dockerPath, | ||
| containerEngine: options?.experimental?.containerEngine, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the diff on the first line is only moving code around.
Is the second line diff expected ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep - i don't want to try and get the socket path unless we are sure there are containers configured and enableContainers is true, but i don't think we have read the config yet here so we can't do that. instead i set it later on in configController.
Fixes DEVX-2103.
Uses
docker context lsto get the socket path that the docker daemon is listening on, without having to manually set it via config or an env var. This is heAlso checks the env var DOCKER_HOST (in addition to WRANGLER_DOCKER_HOST), which is a standard.
Also sets this properly in the vite plugin - we were previously only picking up the value set in the wrangler config.
This has unit tests but i've also tested this manually with the vite plugin/wrangler dev and colima (which listens at a special colima socket).