-
Notifications
You must be signed in to change notification settings - Fork 202
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 a persistent container for ov
#4491
Conversation
Note: This is missing support for aliases, to be restored in the next commit
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.
Works for me 👍
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'm not getting into a formal review because I don't have enough concentration/time but I have a small suggestion.
docker/dev_env/Dockerfile
Outdated
COPY entrypoint.sh /entrypoint.sh | ||
# Infinite sleep loop so that we create a persistent environment | ||
# Everything runs in the same container, rather than new one-off containers for each command | ||
ENTRYPOINT /bin/bash -c 'while true; do sleep 1; done;' | ||
|
||
ENTRYPOINT ["/entrypoint.sh"] | ||
|
||
CMD ["/bin/sh"] | ||
CMD ["/bin/bash"] |
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.
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.
Oh, good to know! I will remove those entries for sure 👍
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.
Actually, I think I'd rather leave these explicit, both entrypoint and command, because it documents the behaviour we expect without relying on the base image "incidentally" behaving that way. I'd rather explicitly instruct our image to loop and sleep. What do you think?
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'll leave that up to you then. I personally would like to avoid instructions that can be omitted without altering the end experience but I can also get behind the value of being explicit.
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've pushed it. Part of why I was reluctant is because I felt the need to explain that we wanted that behaviour, so it was documented in the Dockerfile. It's just as many lines as the explicit implementation, so I don't see a difference, really, but I suppose it's nice to use the default configuration as you said. It just doesn't make anything more concise or easier to understand overall, at least not to me.
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, looking forward to faster ov
commands!
Fixes
Fixes #4490 by @sarayourfriend
Description
Implements the solution described in the issue for using a persistent container to run
ov
. I've also added support on Linux for running commands as root (macOS already always runs commands in the container as root).Anecdotally,
ov
commands do run noticeably faster for me. It's a matter of maybe 50-100ms at most, which is less so faster and moreso just plain responsive feeling. If you happen to notice a change here one way or the other, please let me know. I'm curious to know how fastdocker ps
is on macOS hosts that have the virtual machine to contend with (my impression is they should be plenty fast?).Testing Instructions
To test this change:
ov clean && ov init
to get a brand new environment set upov just api/up
,ov just api/test
,ov nuxt
, etc. Just pick some things you'd normally do with the development environment and try them out.ov sudo dnf update --downloadonly
.dnf
should list some updates (hopefully some exist) and ask if you want to download them. Respond yes (typey
, then return), and let them download. Now run the same command again, and respond yes again. dnf should report that all the packages are already downloaded. If you try the same process onmain
, the second time dnf will download the packages again, because the container environment was not persisted.I will make sure the pinged reviewers include at least one macOS user and one Linux user. In doing so, the changes to accommodate
sudo
exercised in step 3 will be sufficiently tested cross platform.Checklist
Update index.md
).main
) or a parent feature branch.just catalog/generate-docs
for catalogPRs) or the media properties generator (
just catalog/generate-docs media-props
for the catalog or
just api/generate-docs
for the API) where applicable.Developer Certificate of Origin
Developer Certificate of Origin