search

jupyterhub / wrapspawner

Mechanism for runtime configuration of spawners for JupyterHub
BSD 3-Clause "New" or "Revised" License
60 stars 57 forks source link

Create link between WrapSpawner and its child spawner traitlets #27

Closed cmd-ntrf closed 5 years ago

cmd-ntrf commented 5 years ago

This PR addresses issues #24, #25 and #26.

It uses traitlets.directional_link[1] to make any change done on WrapSpawner's traitlets also done on the child spawner's traitlets for the traitlets that both classes share.

25 - current_port is not an attribute of WrapSpawner, but I will also submit a patch for BatchSpawner that remove that traitlet in favor of only using port.

[1] https://traitlets.readthedocs.io/en/stable/utils.html#traitlets.directional_link

mbmilligan commented 5 years ago

@cmd-ntrf or @rkdarst - since we don't have automated testing for Wrapspawner, what I would like to see is confirmation that this works with: a) both Jupyterhub 0.9 and 1.0, and b) with a couple of different spawners, not just Batchspawner.

Basically, I am nervous about the blanket forwarding of like-named traits - that's something I had specifically avoided doing previously. But if this works for those basic cases, I'm happy to accept it and we'll fix any breakage that comes along later.

rkdarst commented 5 years ago

I don't easily have other spawners to test. Does anyone else?

mbmilligan commented 5 years ago

Felix said he would try to test it with Dockerspawner.

cmd-ntrf commented 5 years ago

After testing, JupyterHub 0.9.6 and 1.0 in combination with the proposed PR work for dockerspawner.SystemUserSpawner and jupyterhub.spawner.LocalProcessSpawner.

This is my Terraform setup for test with JupyterHub 1.0 if someone would like to double check: https://gist.github.com/cmd-ntrf/95f9843a8dad1684b31a2c16e50dbef5

mbmilligan commented 5 years ago

Excellent, thanks Felix! Merging now.