Skip to content
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

Use the per-user Docker socket as the Unix default, if present #157

Merged
merged 2 commits into from
Sep 23, 2023

Conversation

pimterry
Copy link
Contributor

This fixes #156. On Mac and Linux, if $HOME/.docker/run/docker.sock is present, that will be used by default. If not, the previous /var/run/docker.sock will be used instead.

This needs to change a few things, because the check for this is async. To make that work, socketPath can now be a function that returns a promise, in which case this will be run and awaited during dial.

This shouldn't be a breaking change, but it probably is because (just searching around GitHub) there's quite a few projects that look at modem.socketPath directly to see which path was detected. Fortunately it looks like you're already on track for a v4 breaking release anyway, so we can sneak this in there too 😄.

To make that change easier for downstream users, and to simplify the internals too, this adds a new getSocketPath method, which always returns either undefined (if no socket path is used - e.g. for remote hosts & SSH) or a promise that should resolve to the path.

Anybody who wants the path can use socketPath = await modem.getSocketPath() and they'll always get either a string or undefined.

Sound reasonable?

There is another way to implement this, by changing the API completely to replace the simple new Modem constructor with an async buildModem method that returns a promise. In that case, we could calculate the socketPath during the method, and then everything after that resolved would just be synchronous. That's a bit cleaner in some ways (no new async steps during dial, socketPath is always just a string or undefined) but it's a bigger breaking change so I've avoided it here. Let me know if you'd prefer that.

This is installed by Docker Desktop, and appears to be the preferred
socket path in the latest versions.
@pimterry
Copy link
Contributor Author

Ah one other downside to doing this within dial: we'll re-evaluate this for every dial (which means for every command, with Dockerode). That should be very quick I think (<1ms) but it's a bit wasteful, and I can imagine there might be edge cases where hitting the disk unnecessarily like that could be expensive.

Doing the buildModem breaking change instead would avoid that, evaluating this just once at construction time instead, if that's important here.

@apocas apocas merged commit 669e227 into apocas:master Sep 23, 2023
4 checks passed
@apocas
Copy link
Owner

apocas commented Sep 23, 2023

I believe we could also look for this inside the constructor without implementing a breaking change, will look into it later today :)
Will ping when published.

@pimterry
Copy link
Contributor Author

Hi @apocas, any updates on this? It'd be great to get this change released so I can start using it!

@apocas
Copy link
Owner

apocas commented Oct 10, 2023

Will publish it today.
Going to cache the value after first execution, to avoid promise execution on every dial call.
This way we avoid a breaking change right now and we get the best of both worlds :)

@pimterry
Copy link
Contributor Author

This way we avoid a breaking change right now and we get the best of both worlds :)

Not quite sure what's happened here. The changes look good (thanks!) but it's been published as v4.0.1, so that is actually a major breaking bump from the v3 version Dockerode uses, and it looks like v4.0.0 was never published at all (no release either here or on npm). Is that right?

Not a huge problem at all, just checking! For my purposes that means I'll just need to wait for a matching Dockerode release so I can use them together though.

@apocas
Copy link
Owner

apocas commented Oct 11, 2023

Yes the major bump was due to #149 (comment)
It was actually a "bug" but didn't want to take chances on someone using the bug as a feature :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use the new Mac Docker socket default path, if present
2 participants