jupyterhub / kubespawner

Kubernetes spawner for JupyterHub
https://jupyterhub-kubespawner.readthedocs.io
BSD 3-Clause "New" or "Revised" License
536 stars 301 forks source link

[bugfix] Using unlisted_choice a second time doesn't work #766

Closed GeorgianaElena closed 11 months ago

GeorgianaElena commented 11 months ago

Multiple usages of the unlisted_choice option result in the same first introduced input to be used. If another pre-selected image is used, that works.

This is ~because we're assuming to find the default {value} of the override, which is not the case after the first override was applied.~ EDIT: because we worked on an object that shouldn't be changed instead of a copy by mistake

This PR:

Reference: https://github.com/2i2c-org/infrastructure/issues/2887 and https://github.com/jupyterhub/kubespawner/issues/772

consideRatio commented 11 months ago

I'm catching up a bit with how this is implemented initially and just learned about the {value} part.

https://github.com/jupyterhub/kubespawner/blob/0e5bb4610be13865a09e16a6b4833835bce4c45a/kubespawner/spawner.py#L1541-L1551

My understanding is that this PR breaks the documented behavior of reading both keys and values in kubespawner_override, to just reading the key.

Here is a snippet from a test config that helps me think. I understand it as it won't matter what we set as a value to the image key on line 54:

https://github.com/jupyterhub/kubespawner/blob/0e5bb4610be13865a09e16a6b4833835bce4c45a/jupyterhub_config.py#L46-L55

How flexible do we want this feature to be?

I'm fine with the idea of reducing the complexity to no longer supporting the string templates as I think this PR does. If we go for that and I understood how things worked, the docs should just be updated first to not mention {value} etc I think.

yuvipanda commented 11 months ago

Thanks for digging into this, @GeorgianaElena!

I would love for us to keep the {value} if possible.

Do you know why this behavior differs on first use vs second?

GeorgianaElena commented 11 months ago

@yuvipanda, @consideRatio, I think I found the actual culprit. The part where the dict of overrides is constructed:

https://github.com/jupyterhub/kubespawner/blob/8f43b505aa33cc1cdd6e7c63109264ea76fdab27/kubespawner/spawner.py#L3157-L3162

wasn't the issue. Instead this was the place where the original self.profile_list dict was updated to hold the actual override instead of the initial {value}. This is because the function was passed a reference to the self.profile_list dict, instead of a copy of it. So chosen_option_overrides is a reference to a dict inside self.profile_list and we're actually modifying it here.