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

[GeorgianaElena]

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:

• updated a test to use unlisted_choice a second time to reproduce the bug
• fixes the bug by just overriding whatever was before with the user's input, not assuming anything about the format of the existing value

Reference: 2i2c-org/infrastructure#2887 and #772

[consideRatio]

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

kubespawner/kubespawner/spawner.py

Lines 1541 to 1551 in 0e5bb46

	- `unlisted_choice`: Object to specify if there should be a free-form field if the user
	selected "Other" as a choice:
	- `enabled`: Boolean, whether the free form input should be enabled
	- `display_name`: String, label for input field
	- `validation_regex`: Optional, regex that the free form input should match - eg. ^pangeo/.*$
	- `validation_message`: Optional, validation message for the regex. Should describe the required
	input format in a human-readable way.
	- `kubespawner_override`: Object specifying what key:values should be over-ridden
	with the value of the free form input, using `{value}` for the value to be substituted with
	the user POSTed value in the `unlisted_choice` input field. eg:
	- some_config_key: some_value-with-{value}-substituted-with-what-user-wrote

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:

kubespawner/jupyterhub_config.py

Lines 46 to 55 in 0e5bb46

	'profile_options': {
	'image': {
	'display_name': 'Image',
	'unlisted_choice': {
	'enabled': True,
	'display_name': 'Image Location',
	'validation_regex': '^pangeo/.*$',
	'validation_message': 'Must be a pangeo image, matching ^pangeo/.*$',
	'kubespawner_override': {'image': '{value}'},
	},

How flexible do we want this feature to be?

• Before this PR the unlisted choice could update multiple string based configs based on string templates.
• After this PR the unlisted choice could update multiple string based configs - but only directly based on the provided value.

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]

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]

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

kubespawner/kubespawner/spawner.py

Lines 3157 to 3162 in 8f43b50

	for k, v in chosen_option_overrides.items():
	chosen_option_overrides[k] = v.format(
	value=selected_profile_user_options[
	unlisted_choice_form_key
	]
	)

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.

[consideRatio]

Nice work figuring this out @GeorgianaElena, it looks great!