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

hub image: add sqlalchemy-cocroachdb dependency #2262

Merged
merged 1 commit into from
Jun 21, 2021

Conversation

weisdd
Copy link
Contributor

@weisdd weisdd commented Jun 18, 2021

Based on jupyterhub's docs, it should be generally compatible with any sqlalchemy provider.

In our environment, we're relying on cocroachdb due to easier clusterization and scalability.
Aside from a tiny defect in jupyterhub (jupyterhub/jupyterhub#3484, it was not part of 1.4.1), which might not even be specific to cocroachdb (rather to the dummy authenticator), everything seems to work well.

Currently, we have to dynamically install sqlalchemy-cockroachdb upon container start. It'd be great to have it included in the base image.

P.S. If I got it right, I had to run the dependencies freeze --upgrade script to make sure all dependencies are added to the requirements.txt. That's why you can see two files updated instead of just one (requirements.in).

@welcome
Copy link

welcome bot commented Jun 18, 2021

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@consideRatio consideRatio changed the title Add sqlalchemy-cocroachdb dependency hub image: add sqlalchemy-cocroachdb dependency Jun 18, 2021
@consideRatio consideRatio requested a review from minrk June 18, 2021 16:43
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.

@minrk I think it is reasonable to add this python library to the hub image while still considering that we want to avoid increasing complexity and such, do you agree?

@minrk minrk merged commit 44aab41 into jupyterhub:main Jun 21, 2021
@welcome
Copy link

welcome bot commented Jun 21, 2021

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

consideRatio pushed a commit to jupyterhub/helm-chart that referenced this pull request Jun 21, 2021
@minrk
Copy link
Member

minrk commented Jun 21, 2021

I think this is fine to do for db backends that folks use, since there can't be many. Authenticators are more of a slippery slope, so I opened #2265 so we can have a better story for less-used dependencies than always submitting patches to the base image.

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.

3 participants