-
Notifications
You must be signed in to change notification settings - Fork 389
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
[WIP] Automatic HTTPS and tests #1101
Conversation
Scrolling through this to see what kind of things are being changed. I think some of it could be a separate PR that can be merged before (and we can debug it).
Will we require every dev to setup pebble locally? Can we instead keep the current way of running BinderHub as a dev w/o pebble and add a second setup with pebble? In the CI will we have one run without pebble (what we do now) and one with? |
@betatim I'd like to get back to these questions when this work has matured a bit, I share your concerns and will do my best to address what I can. Would you like to join a session with Min / Simon friday next week, with a focus on the Z2JH testing infra that I suggest we mimic overall in this subsequent PR? https://doodle.com/poll/xtbtcgx6wivpmcwz? While I would like to develop the related z2jh PR and this before we enter a review process, I felt an itch to answer your questions since you have invested your time to consider this PR already:
We only tested one k8s version in the past, and it was 1.13, currently 1.18 is available.
In z2jh, it seems like git-crypt was a stranded remnant that wasn't used any more, I think this is the case for this repo as well. It is only used from
The hub_url configuration was passed to browsers, to binder, and more. By having hub_url_local we can avoid making "hairpin" requests where not needed where we exit to the internet and go straight back. The hairpin requests exiting and directly coming back caused issues with our test setups, so two reasons exists for implementing it. I'll document the case when it is more clear to me what is really needed, what isn't, and what makes most sense.
I'm not sure what I suggest as I don't overview it fully overview it yet, I need to work more until I have a suggestion what I think make sense. I'd like there to be lightweight tests not involving k8s at all if possible for example. Right now, I think there is "main", "auth", and "helm", where "main" and "auth" both install k8s + z2jh, while "helm" installs k8s + the binderhub helm chart that reference the z2jh helm chart as a dependency.
I got tired of waiting 30 seconds for the old binderhub pod to shut down because the container isn't listening to the SIGTERM that comes 30 seconds before SIGKILL. It can perhaps be fixed by starting binderhub differently than
Because it is two version behind the Python client already which is available at version 11, and those are in turn behind the official k8s versions - this should be bumped further i think, to ensure we don't loose track of kubernetes developments entirely. This is an easy thing to have in a separate PR, and I think it will work with the old version as well still.
Agree, its needed, and I'm aiming to do that. I've got help from @GeorgianaElena on my suggested z2jh contribution instructions, this is how it currently looks, but its also a WIP still: https://github.com/jupyterhub/zero-to-jupyterhub-k8s/blob/7da1ffdf302f01bc350e9a9b0d8b66f0c917ef15/CONTRIBUTING.md
I'm not sure what I'd suggest yet, it will depend how simply the setup of pebble is I think, but BinderHub may benefit from this because you can get hub.binder.test and binder.test making CORS matters be more realistic as well.
This PR is currently in a state where it has disabled all but one test to focus on making sure that one test works. It is possible to keep testing non autohttps based setups as well of course. |
I've just realised I opened a similar PR a few months ago to fix what I thought was a problem unique to my K8s cluster, though it's basically the same issue as here but with a different underlying cause: #994 |
ac2c061
to
8abcf49
Compare
8abcf49
to
82cc5a8
Compare
I'm closing this PR as I don't intent to continue from this point but instead start fresh in another PR. I gained a lot of experience and insights that led up to the CI updates in #1188, so the new PR can be a lot less cluttered with changes. |
I'm still experimenting, but the end goal is to allow for automatic https without cert-manager etc.