-
Notifications
You must be signed in to change notification settings - Fork 391
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
DinD documentation #838
DinD documentation #838
Conversation
Wow thanks @jhamman ! That made it a lot clearer to me. I deployed a BinderHub for neurips once and that is all my BinderHub deployment experience, but at that time I didn't understand this but now I grasp this thanks to your documentation! ❤️ 🎉 LGTM |
Question for Yuvi and/or Min: are there also security benefits for running builds via docker-in-docker? In my mind one reason to run in DIND mode is that you have better control over what the |
@betatim I find this very challenging, and while working with gitlab's helm chart I've considered related security concerns about Docker overall to build and run things for users in CI pipelines. Here are some of these concerns written down by GitLab's team: |
Bumping this so we don't loose too much momentum. @betatim / @consideRatio - should we add some comments about DIND and security? I would need input from you on what to write. |
@jhamman I'm not knowledgeable enough about these aspects yet to suggest something =/ |
doc/setup-binderhub.rst
Outdated
dind: | ||
enabled: true | ||
daemonset: | ||
image: |
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 and the next two lines need to be indented. Though they are the same as the default so we probably don't need them at all?
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 fixed the indentation. I think it is helpful to show the default the image specification here though as this is the sort of thing someone may want to customize.
doc/setup-binderhub.rst
Outdated
--------------------------- | ||
|
||
The Helm chart used to install your BinderHub deployemnt has a lot of options | ||
for you to tweak. Below is a few pointers for how to configure some of the most |
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.
for you to tweak. Below is a few pointers for how to configure some of the most | |
for you to tweak. Below a few pointers for how to configure some of the most |
When PRs go stale feel free to bump them by following up on the questions/discussion points by suggesting a resolution that seems to follow the consensus. I think a frequent reason for why things go stale is that there is no immediate actionable thing and no one is trying to get to one. I think it is best if the PR proposer tries to do the driving by suggesting a next actionable item. For now I would skip the security question and maybe @jhamman can create a new PR addressing/adding thoughts on that. That way we can get this PR merged. |
@betatim - thanks for the review. I've updated things a bit and this should be good to go in.
I don't know. I'm not familiar with this issue so I'll leave this for the next contributor. |
closes #832