Skip to content
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

Mounting binderhub_config.py instead of letting it be part of the image #1165

Merged
merged 4 commits into from
Oct 19, 2020

Conversation

iwilltry42
Copy link
Contributor

@iwilltry42 iwilltry42 commented Oct 19, 2020

PR description by consideRatio

This mounts the binderhub_config.py file like done in Z2JH, which means that binderhub images with extra packages installed no longer need to keep updated with changes to binderhub_config.py files by rebuilding their images etc. But, at the same time, anyone that have made a custom change to the binderhub_config.py file embedded in the images in the past, would now need to mount it.

See jupyterhub/zero-to-jupyterhub-k8s#1407 for the Z2JH equivalent PR.

Original PR description

Revival of #1076 after screwing up branches 🙄
FYI @consideRatio

Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this work! I realize now, i think we should only set the args in k8s and let ENTRYPOINT in dockerfile, because another change we want is to start this pod with a tool called tini i think, and such change should not need to be coupled with the helm chart.

Hmmmm, or should entrypoint for dockerfile be set to just tini, or also include the call to binderhub? Not so important perhaps. Id say lets mirror z2jh for now with regards to that for now.

/ Cheers frlm my mobile during my commute

@consideRatio
Copy link
Member

Aaargh sorry, perhaps tini isnt needed, anyhow, that is another PR.

Two easy requests:

  • nitpick about new line in end of file
  • inconsistency between splitting dockerfiles cmd into entrypoint/cmd and k8s config of command/args (these sets are the same, but differently named, in k8s/dockerfile). I think id suggest to set Entrypoint in dockerfile and not setting command in k8s is, letting k8s only influence the args to binderhub, while the dockerfile can be free to install and use tini later by changing the entrypoint to wrap the call to binderhub or similar. Hmmm

@consideRatio
Copy link
Member

@iwilltry42 hey btw thank you for work on k3d :)

I think this PR is in a good enough state for merge already pretty much, i probably overthink this a bit.

@consideRatio consideRatio merged commit 7847789 into jupyterhub:master Oct 19, 2020
@welcome
Copy link

welcome bot commented Oct 19, 2020

Congrats on your first merged pull request in this project! 🎉
congrats
Thank you for contributing, we are very proud of you! ❤️

@consideRatio consideRatio added breaking maintenance Under the hood improvements and fixes enhancement and removed maintenance Under the hood improvements and fixes labels Oct 19, 2020
@consideRatio
Copy link
Member

Thank you @iwilltry42! ❤️

@iwilltry42
Copy link
Contributor Author

Thanks for your feedback and the merge @consideRatio :)
However, I agree with your arguments and I would generally add tini to the Dockerfile and then set ENTRYPOINT ["tinit", "--"] and CMD ["python3", "-m", "binderhub", "--config", "/etc/binderhub/config/binderhub_config.py"].
Then in the Kubernetes manifest we could "duplicate"/"override" the CMD with the args for the sake of completeness (i.e. people see what it does just from the manifest 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants