Allow end user to select a choice different from list of available choices in profile_list

[batpad]

Fixes #699 .

This should eventually:

• Allow specifying an option on profiles to allow a users to specify free-form text for an image (currently a single boolean called allow_other, but I think @yuvipanda had some ideas to make that a bit nicer.
• If allow_other is true, show an additional option called "Other" in the dropdown and handle showing a text input when Other is selected
• Handle the text input value on the backend to actually pull the image onto the cluster, etc.

Currently, have added a simple allow_other option and handling it just on the frontend.

@yuvipanda hopefully this is a good starting point for us to work together on implementing handling the value on the backend and improve the semantics of the options.

cc @consideRatio

[consideRatio]

Ooooh great to see you work towards this @batpad!!!

Reflection on config naming

I think allow_other could be renamed to be a bit more self-explanatory, as something related to the choices config.

With a free-form input field, front-end validation of the input is probably going to end up being another feature. I don't think we need to think about implementing that as part of this PR, but I'm considering it already when thinking on what configuration to provide. For example include_other_choice as a boolean may need to be augmented with other_choice_validation or similar, which makes me consider if we want other_choice as a dictionary with included under it or similar, allowing other_choice.included and other_choice.some_validation_related_config.

Hmmm hmmm, I'm currently leaning towards a flat structure to configure this, like other_choice_included, to be complemented with other_choice_some_validation_related_config. There are pros and cons of using a flat config structure (a_b, a_c) vs a nested structure (a["b"], a["c"]), but I think staying flat is fine here but its good if we can find a common prefix in the name such as other_choice_.

Wdyt about renaming allow_other (boolean) to other_choice_included (boolean)?

[batpad]

@consideRatio I think @yuvipanda had a good idea for how this config should be structured, making it an object, that also contained a regex to use to validate the user input. Let's wait for him to weigh in on the structure for options - sorry, he had pasted that to me somewhere, but I can't find it :-/ .

[batpad]

There are pros and cons of using a flat config structure (a_b, a_c) vs a nested structure (a["b"], a["c"]), but I think staying flat is fine here but its good if we can find a common prefix in the name such as other_choice_.

Hmm, this is an interesting one - yea, I do think this should allow for multiple "sub-options" like the regex to validate input, so either prefixing with other_choice_ or making it a nested structure sounds good to me - I don't really have a preference on nested vs flat - happy to go with whatever @yuvipanda and you feel is best here.

[yuvipanda]

Thanks for your comments, @consideRatio!

I left a comment in batpad@cc0037b#r111675499 earlier, which I will repro here.

Let's expand this a little bit. How about something like:

freeform:
   enabled: true
   match_regex: (optional regex to match)
   kubespawner_override:
       some_config_key: some_value-with-{value}-substituted-with-what-user-wrote

So for an image where we only want to allow pangeo images, it would be something like

freeform:
   enabled: true
   match_regex: ^pangeo/.*$
   kubespawner_override:
      image: "{value}"

kubespawner/kubespawner/spawner.py

Line 3016 in ff8dcf1

option_formdata_prefix = f'profile-option-{profile}-'

is what gets the form data, converts them into 'user options' that are then passed on to

kubespawner/kubespawner/spawner.py

Line 3091 in ff8dcf1

for option_name, option in profile.get('profile_options').items():

, which interprets them and applies the overrides.

[yuvipanda]

With respect to nested vs flat, @consideRatio, I think nested is better here! Because we already will have three things:

1. Wether to enable this or not
2. How it should actually override the kubespawner traitlet config. Currently, with choices, we set a 'key' and override it with with specific config that is set in the jupyterhub_config.py. With free-form text, we should allow admins the option to interpolate the value entered into an override. We also need to store the raw user entered value in user_options, and re-setup the kubespawner_override whenever the user server is stopped / started again.
3. Simple regex based frontend validation, that @batpad tells me can be done with HTML5 form validation built into browsers, no extras needed.

Given this, I'd suggest the nested config structure.

[batpad]

To add:

• We would also want something like a validation_message to show a human-readable error message / describe the input format required, like the human readable value of the regex field.

[yuvipanda]

Re: nested config, I also opened #736 to help us mitigate the complexity of validation in the nested config in the future. Does not need to block this PR though. I would like us to make sure we get frontend validation in here with this though.

[batpad]

Pushed a commit using your suggestion for the config @yuvipanda (not a problem if we want to change it, just wanted to get a sense of if the validation, etc. works, which it seems to).

So now, instead of allow_other as a boolean, there is an object free_form with the properties Yuvi mentioned, with two additional properties: display_name and validation_message, which control the text for the label of the input, and the message to be shown if the user input does not match the validation regex.

In the frontend form, just using html form validation with a pattern attribute for the regex - this seems to work good enough - and we can use the title message to add a custom message to the error. Will trigger when you try and submit the form.

On the frontend part of this work:

• Need to test and handle different cases of the config - i.e. when no regex match is specified, etc. just making sure the frontend handles all this. @yuvipanda this would link to the config validation work you're talking about - would be good to know how many edge cases I need to handle in the template output, versus how much will be done as part of config validation.
• Currently, the value for the "other" option in the drop-down is hard-coded as "other". This means you can't have a different option in the drop-down that has a value "other". Hopefully this is fine, but just a heads-up - we can change this hard-coded "other" to something else if this seems like it might be a problem.

@consideRatio would be great to get your thoughts on the config structure and naming here, happy to make any changes.

This should now POST a field with a name like profile-option-training-python-image-other with the text that's in the "Other" field to the backend.

I'll start to take a look at the backend integration for this, though may have limited availability after this week for the next ~2 weeks, so if someone else is able to take on the backend integration, please do :-)

[ranchodeluxe]

Hey folks, just checking in here. There's a demo on June 29th that Aimee will be doing and I'm wondering if maybe this work could be wrapped up and exposed on nasa-veda cluster by then? No worries if it's a stretch. Is there anyway I can help?

[batpad]

Starting to look at the backend integration for this.

Just pasting over @yuvipanda's comments from elsewhere about where this needs to happen to have it easy to refer to:

kubespawner/kubespawner/spawner.py

Line 3016 in ff8dcf1

option_formdata_prefix = f'profile-option-{profile}-'

is what gets the form data, converts them into 'user options' that are then passed on to

kubespawner/kubespawner/spawner.py

Line 3091 in ff8dcf1

for option_name, option in profile.get('profile_options').items():

, which interprets them and applies the overrides.

I'll start familiarizing myself here and then lean on you @yuvipanda to get through the actual implementation.

[batpad]

@yuvipanda @consideRatio there's probably some discussion to be had around details of the implementation here, but this is now "complete" and ready for review:

• Adds an other_choice option which handles showing a free text input for that choice
• Handles validating and overriding the appropriate profile value on the backend
• Has basic tests for the above, including a test to handle the choices parameter which did not have a test

Overall, I think there's some ideas on how we could do this whole thing a bit better, but that might be a bit of a larger refactor, that hopefully we can discuss separately.

I'll give this whole thing another good look tomorrow, but do let me know anything that pops out / that should definitely be fixed.

(thanks again @yuvipanda for helping me figure out most of the backend integration code)

[consideRatio]

• Currently, the value for the "other" option in the drop-down is hard-coded as "other". This means you can't have a different option in the drop-down that has a value "other". Hopefully this is fine, but just a heads-up - we can change this hard-coded "other" to something else if this seems like it might be a problem.

I think its the right call to hardcode this specific choice for a given profile, and "other" seems like a fine choice but also one that could risk collide with other choices. I think either we should make the name a bit more obscure, like other-choice / other_choice, or force the other choices slugs to not overlap with the help of validation logic.

[batpad]

I think either we should make the name a bit more obscure, like other-choice / other_choice, or force the other choices slugs to not overlap with the help of validation logic.

Speaking to @yuvipanda a bit here -- have gone with naming the form field with --other-choice i.e. adding an additional - to prevent accidental naming conflicts.

It would also be good to have comprehensive validation of the config via JSON schema or similar, and then we can just prohibit choices with -- in them. We can do that as a separate PR?

[GeorgianaElena]

I believed this should be merged, after https://github.com/batpad/kubespawner/pull/2 gets merged as well! 🚀

[yuvipanda]

@GeorgianaElena ok this is ready to go now i think!

After this, I think it'd be useful to do a refactor to move all the profile handling code into a separate file. But I don't want to make this PR too much longer...

[GeorgianaElena]

@yuvipanda, I agree! But for now, let's merge this! 🚀

Many thanks to everyone for all the thought and effort that went into this PR! 🎉