-
Notifications
You must be signed in to change notification settings - Fork 75
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
feat: start network docker container with cli #1107
feat: start network docker container with cli #1107
Conversation
Looks great! Just one nit about using expect vs using an error. The error lets you capture the failed command. You can use |
80d10a4
to
3983924
Compare
3983924
to
1ab08a7
Compare
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.
Solid start! Marking as "request changes" for the various doc comment stuff. The "start in detached by default with more complex health checks" thing (see comment below) would be awesome, but we could also do it in a follow-up to this one. It's already v useful to have these commands, so it might not be worth holding it up. lmk if you think we should do it at all, and if so, if we should do it now or wait for later.
I also think it would be really cool if we checked if Docker Daemon is running and, if not, rather than allowing Docker's error message to bubble through:
...we could instruct them on how to install colima. OR, we could automatically install it for them? (We could offer, and if they say I don't think we need to be all-in on colima, though. If there are other alternatives to Docker Desktop/Daemon like it, then we could check for those, too. And in any case, rather than bubbling the
Or maybe we sniff their OS and recommend colima only if they're on a Mac, podman if linux, and containerd if Windows, which are the suggestions in this blog post. I don't know. But I want to make sure we make this feel doable to total Docker & Stellar newbies who try to start a local dev environment with |
Rather than hardcoding specific recommendations into the CLI, I think we should have a message similar to what's displayed above, but link to the soroban.stellar.org docs and place a page there with links out to docker/container runtimes folks can try, with recs based on mac, linux, windows. It means we can update the list as needed. I'd also keep the original error. Never know what type of useful information might be found by passing through errors:
|
977be20
to
8c542a5
Compare
@chadoh @leighmcculloch Regarding the error case when the docker daemon can’t be found - I realized that when using bollard (and the underlying Docker API) it doesn’t seem to bubble up the original error ( So instead, I’m priting out a message similar to ℹ️ Starting testnet network
⛔️ Failed to connect to the Docker daemon at "/var/run/docker.sock". Is the docker daemon running?
Running a local Stellar network requires a Docker-compatible container runtime.
Please note that if you are using Docker Desktop, you may need to utilize the `--docker-socket-path` flag to pass in the location of the docker socket on your machine.
error: ⛔ ️Failed to start container: error trying to connect: No such file or directory (os error 2) |
84cedae
to
4e2a425
Compare
Co-authored-by: Leigh McCulloch <[email protected]> Co-authored-by: Chad Ostrowski <[email protected]>
… passed in as an env var Co-authored-by: Willem Wyndham <[email protected]> Co-authored-by: Leigh McCulloch <[email protected]>
851ed5f
to
2114d1d
Compare
Co-authored-by: Leigh McCulloch <[email protected]>
0e90ded
to
4d246d7
Compare
…er desktop default socket
94b48e5
to
9367847
Compare
c2d7779
to
70dccbd
Compare
What
This PR adds the
network start
andnetwork stop
subcommands to allow users to start and stop the stellar docker container using the CLI.Start help output
Stop help output
Output when the command is not able to connect to the docker daemon
Output when the command starts the container successfully
Why
To make it easier for users to run a stellar node for soroban development.
Known limitations
I learned that when using Docker Desktop, it starts the docker daemon on a non-standard socket 😵💫 . The Docker Desktop UI allows a user to overwrite this, in an advanced setting. But I figured allowing the user to do this all via the CLI would probably be easier, so I added the
--docker-socket-path
flag. I think that it may be helpful to allow the user to set an env var for this, instead of always passing a flag, but I'll plan to allow for this in a followup PR.resources that helped identify this:
- fussybeaver/bollard#345 (suggestion of using DOCKER_HOST env var did not work)
- https://docs.docker.com/desktop/mac/permission-requirements/
- https://devops.stackexchange.com/questions/17905/ctop-does-not-work-with-latest-version-of-docker-get-http-unix-sock-info-d#:~:text=This%20is%20probably%20because%20Docker,sock%20anymore.&text=If%20you%20want%20to%20have,used%20and%20press%20Apply%20%26%20restart%20.
Todo
To handle in a followup PR
--docker-socket-path