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
Add support for debian >=10 to bootstrap.py #800
Conversation
Thanks for submitting your first pull request! You are awesome! 🤗 |
Personally I have no objections to adding other distributions providing they're tested and easy to maintain- in this case I'd probably go for officially supporting and testing Debian 11 and not 10. However I'd like input from the other maintainers, especially @yuvipanda @consideRatio, first. I think it'd be good to sort out #724 before making any significant changes to TLJH, though we could perhaps merge this PR with tests but make clear it's not supported for now? |
Debian 10 simply works. No special treatment is required - and it is not going to change. The minimal Ubuntu is also fairly ancient (18.04). I would keep it in as "not officially supported" but not blocked (maybe with a warning?). |
The similarities between Debian / Ubuntu makes for a good case of supporting both if there is demand and adds value. I think that #724 is a critical issue to resolve that would make me more comfortable considering features overall. While this now installs okay and that is great, it adds complexity. Suddenly we have documentation that may be outdated for debian by assumptions of ubuntu and get questions about that - maybe not - I'm not sure. We have CI pipelines that take a bit longer to run because we test more variations I assume. But since there is no real change, I think I'm inclined to agree that supporting debian is acceptable if:
@GeorgianaElena @minrk @yuvipanda what do you think? |
bootstrap/bootstrap.py
Outdated
@@ -356,6 +360,7 @@ def serve_forever(server): | |||
"python3-venv", | |||
"python3-pip", | |||
"git", | |||
"sudo", |
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 assume sudo
is installed in ubuntu by default, but needs to be installed in debian - is that correct? If so, let's add an inline comment about it!
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.
Sudo is. indeed, not installed in Debian by default. But adding it here is harmless if it is installed and fixes it for Debian.
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 think this is OK, even if it's pre-installed in Ubuntu this acts as documentation that sudo is required. It's also useful if someone uses a custom Ubuntu cloud image with minimal dependencies.
On the other hand https://tljh.jupyter.org/en/latest/install/custom-server.html#step-1-installing-the-littlest-jupyterhub mentions sudo apt install python3 python3-dev git curl
is a pre-requisite so we should decide what is a prerequisite, and what should be managed by bootstrap.py
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.
What workflow were you using to run bootstrap.py
? The official instructions say curl -L https://tljh.jupyter.org/bootstrap.py | sudo -E python3 - --admin <admin-user-name>
so sudo
needs to be installed before running bootstrap.py
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.
You are right. But since I needed to modify the script and I never run scripts from the pipe to sudo I downloaded the script with wget and run it with python. If the sudo was not installed previously, the installation would fail.
Note that, there is no harm in requiring installation of already installed component.
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.
OK, presumably you're running bootstrap.py
as root
then? That's why I asked for your workflow 😄
It looks like the sudo package is only required for some config files as far as the installer is concerned:
the-littlest-jupyterhub/tljh/installer.py
Lines 147 to 153 in 0572976
with open("/etc/sudoers.d/jupyterhub-admins", "w") as f: | |
# JupyterHub admins should have full passwordless sudo access | |
f.write("%jupyterhub-admins ALL = (ALL) NOPASSWD: ALL\n") | |
# `sudo -E` should preserve the $PATH we set. This allows | |
# admins in jupyter terminals to do `sudo -E pip install <package>`, | |
# `pip` is in the $PATH we set in jupyterhub_config.py to include the user conda env. | |
f.write("Defaults exempt_group = jupyterhub-admins\n") |
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.
Yes, I was running it from root. Sudo is required in ubuntu install and probably (?) during some admin tasks. Debian is not using it for admin, but it is fully supported there.
This sounds good to me. Thanks for putting in the work, @jochym |
@yuvipanda, @consideRatio I added the requested comments, but three integration tests fail for unconnected reasons on newer ubuntus. I do not get why. My fork is synced with upstream recently and have no unconnected changes. Is this some problem with upstream? |
Merge upstream development
Sync with upstream development
@yuvipanda, @consideRatio : after syncing with the recent development, all the test run fine. Since I think I have included all your comments into the fork. Maybe the PR can be merged now? |
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.
The changes required are minimal and the similarities between debian/ubuntu are minimal overall I think. Due to that, I think this is good to go!
This is rather minimal patch to add support for the Debian 10 and above to the TLJH. If it is going to be accepted, the Debian 10 and 11 should be added to the tests (it works in my internal tests). Unfortunately, I do not know what is the testing setup here. I will be happy to cooperate on this - of course. TLJH installed with earlier version of the patch works flawlessly for my students for over a year.