-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Install docker in the OpenHands app image #4283
Conversation
containers/app/Dockerfile
Outdated
RUN apt install -y apt-transport-https ca-certificates gnupg \ | ||
&& curl -fsSL https://download.docker.com/linux/debian/gpg | sudo gpg --dearmor -o /usr/share/keyrings/docker.gpg \ | ||
&& echo "deb [arch=$(dpkg --print-architecture) signed-by=/usr/share/keyrings/docker.gpg] https://download.docker.com/linux/debian bookworm stable" | sudo tee /etc/apt/sources.list.d/docker.list > /dev/null \ | ||
&& apt update \ |
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.
- is
apt-update
needed twice (see line 46)? - I don't recall OH is doing any of the signing operations, are these needed?
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 was following this: https://linuxiac.com/how-to-install-docker-on-debian-12-bookworm/
But I can remove those and see how it goes. The apt update
may be needed after we add that source but I'm not linux expert lol. Will try a few things
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.
@tobitege followed the official docs now: https://docs.docker.com/engine/install/debian/
update is required for sure (tried it).
I'm guessing signing is required as well.
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.
Yup after you add a new source you need to update
Have you taken a look at this (recent contribution)? Looks very similar to your steps, maybe that helps: |
That one seems to be installing the Ubuntu one: https://docs.docker.com/engine/install/ubuntu/
So I followed that one. |
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.
This is great! Actually - can we add a quick test for this workflow? e.g., try to run docker run hello-world
inside the APP image?
test would definitely be nice to have! |
@rbren and @xingyaoww added test based on Xingyao's suggestion. |
End-user friendly description of the problem this fixes or functionality that this introduces
Install docker in the OpenHands app image
Give a summary of what the PR does, explaining any non-trivial design decisions
Installing docker in the OpenHands app image allows it to build a sandbox image off of a SANDBOX_BASE_CONTAINER_IMAGE using the docker method. So users are able to provide a base image to the docker command without the development flow.
I tested this by:
myapp:1.0.0
I am unsure if this is the route we want to go. It seems to add 0.5GB to the app image. However, this is what I came up with after some guidance. See the issues for why this change is needed below. If this is fine, I can this method to the https://docs.all-hands.dev/modules/usage/how-to/custom-sandbox-guide doc
Link of any specific issues this addresses
#4220
#4230