-
Notifications
You must be signed in to change notification settings - Fork 8.3k
feat(server): Add a new configuration variable, runtime_extra_volumes #7552
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
4bbeb00 to
2f5a423
Compare
2f5a423 to
ca7b0b9
Compare
tofarr
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.
Aside from the introduced type error, this looks like a solid change. It should be noted that we have moved away from a mounted volume by default.
|
@tofarr Thanks for your review. Fix the typing issue.
I am confused. It means the runtime container does not use mounted volume as workspace, isn't it? |
|
There's no issue to 👍 , but I want to chime in and say this would solve many issues for us. Beyond docker in docker, we also would mount a dependency cache volume so we can re-use the outputs of things like |
enyst
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.
LTGM 🍰
@tofarr I think your concern has been addressed, could you take a look?
|
This PR is stale because it has been open for 30 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
|
This PR was closed because it has been stalled for over 30 days with no activity. |
|
@ROCKTAKEY I apologize for us dropping the ball here. We could have handled this differently, and I promise I'll try to fix our process. In the meantime, another PR implemented this slightly differently, in the sandbox section, and this feature is released. Thank you for the contribution, and I'm really sorry for the miss! |
|
@enyst Never mind. I'm glad it was implemented. I will show the functions implemented for someone who needs the feature: |
End-user friendly description of the problem this fixes or functionality that this introduces.
Add a new configuration variable,
runtime_extra_volumeswhich means extra volumes for bind mounts.We can bind-mount the host directory in the sandbox container by setting it.
It is useful for Docker outside of Docker since it let us mount
/var/run/docker.sockon the sandbox.Give a summary of what the PR does, explaining any non-trivial design decisions.
Add a new configuration variable,
runtime_extra_volumesand add document for it.Link of any specific issues this addresses.
None