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

Fix conflicting key with kubespawner #59

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ph0tonic
Copy link
Contributor

@Ph0tonic Ph0tonic commented Jan 8, 2024

https://github.com/jupyterhub/kubespawner 6.x is conflicting with this project. Version 6 now includes a way of configuring profiles a bit like wrapspawner but limited to kubernetes.
When they introduced this feature, they used the same key to get the right profile from the form. When trying to spawn a process with kubespawner from wrapspawner, it crashes with the following error:

Exception in Future <Task finished name='Task-37' coro=<BaseHandler.spawn_single_user.<locals>.finish_user_spawn() done, defined at
    Traceback (most recent call last):
      File "/usr/local/lib/python3.11/site-packages/tornado/gen.py", line 625, in error_callback
        future.result()
      File "/usr/local/lib/python3.11/site-packages/jupyterhub/handlers/base.py", line 988, in finish_user_spawn
        await spawn_future
      File "/usr/local/lib/python3.11/site-packages/jupyterhub/user.py", line 902, in spawn
        raise e
      File "/usr/local/lib/python3.11/site-packages/jupyterhub/user.py", line 798, in spawn
        url = await gen.with_timeout(timedelta(seconds=spawner.start_timeout), f)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/usr/local/lib/python3.11/site-packages/kubespawner/spawner.py", line 2667, in _start
        await self.load_user_options()
      File "/usr/local/lib/python3.11/site-packages/kubespawner/spawner.py", line 3327, in load_user_options
        self._validate_user_options(profile_list
      File "/usr/local/lib/python3.11/site-packages/kubespawner/spawner.py", line 3132, in _validate_user_options
        profile = self._get_profile(profile_slug, profile_list
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/usr/local/lib/python3.11/site-packages/kubespawner/spawner.py", line 3176, in _get_profile
        raise ValueError
    ValueError: No such profile: kubespawner. Options include: 

This PR fixes this conflict by using a different key name, wrap_profile. I am currently using this fix in production and it's now working fine.

\cc @mbmilligan

Copy link

welcome bot commented Jan 8, 2024

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

If you haven't done so already, check out Jupyter's Code of Conduct.
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! 🎉

@Ph0tonic
Copy link
Contributor Author

Ph0tonic commented Feb 6, 2024

Hi, just willing to bump this up @mbmilligan 👍

@Ph0tonic Ph0tonic mentioned this pull request Mar 21, 2024
@Ph0tonic
Copy link
Contributor Author

Ph0tonic commented Apr 3, 2024

Hello, any feedback on this issue ? This issue is really blocking any use with KubeSpawner.
\cc @minrk

@minrk
Copy link
Member

minrk commented Apr 4, 2024

Sorry for the slow response. Changing this key is a breaking change, since launch API calls may use this. Perhaps the right fix is to not pass consumed user_options field(s) to the child spawner? Or namespace them upon passing?

@Ph0tonic
Copy link
Contributor Author

Ph0tonic commented Apr 9, 2024

No problem, thank you very much for your feedback. I understand the need to avoid breaking changes but I am wondering whether the cost to avoid this situation is higher thank just releasing a 2.x version. Namespacing and filtering options could add some code not required making it more complicating to maintain. I am also not sure whether not passing consumed fields or namespacing them could also be considered a breaking change in itself as we change the list of arguments provided to spawners ?

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

Successfully merging this pull request may close these issues.

2 participants