Closed batpad closed 1 year ago
Ooooh great to see you work towards this @batpad!!!
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)?
@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 :-/ .
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 otherchoice.
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.
Thanks for your comments, @consideRatio!
I left a comment in https://github.com/batpad/kubespawner/commit/cc0037b9753d6905cd1d98a0a2acaded905fd84d#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}"
https://github.com/jupyterhub/kubespawner/blob/ff8dcf15205208fb25a7255563b59f53f07aca88/kubespawner/spawner.py#L3016 is what gets the form data, converts them into 'user options' that are then passed on to https://github.com/jupyterhub/kubespawner/blob/ff8dcf15205208fb25a7255563b59f53f07aca88/kubespawner/spawner.py#L3091, which interprets them and applies the overrides.
With respect to nested vs flat, @consideRatio, I think nested is better here! Because we already will have three things:
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.Given this, I'd suggest the nested config structure.
To add:
validation_message
to show a human-readable error message / describe the input format required, like the human readable value of the regex
field.Re: nested config, I also opened https://github.com/jupyterhub/kubespawner/issues/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.
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:
@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 :-)
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?
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:
https://github.com/jupyterhub/kubespawner/blob/ff8dcf15205208fb25a7255563b59f53f07aca88/kubespawner/spawner.py#L3016 is what gets the form data, converts them into 'user options' that are then passed on to https://github.com/jupyterhub/kubespawner/blob/ff8dcf15205208fb25a7255563b59f53f07aca88/kubespawner/spawner.py#L3091, 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.
@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:
other_choice
option which handles showing a free text input for that choicechoices
parameter which did not have a testOverall, 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)
- 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.
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?
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.
Apologies, I mis-spoke in my comment above. Two different things needed to be done:
name
field, affecting the key for the value being POSTed, made the --other-choice
instead of -other-choice
value
in the drop-down for the Other
option. Changed that currently to just other-choice
from other
.Just wanted to quickly drop a big thanks to everyone workin on this! Having this option will be a tremendous boost in useability/reproducibility for science communities! 🚀
How will this be used in the z2jh helm chart? Looking at https://z2jh.jupyter.org/en/stable/resources/reference.html#singleuser-profilelist I don't see a direct/obvious relation/impact, so wanted to ask.
@echarles once this is merged, released, and bumped in z2jh, someone would probably add a documentation PR there so it can be used.
@echarles once this is merged, released, and bumped in z2jh, someone would probably add a documentation PR there so it can be used.
Makes sense, I have opened https://github.com/jupyterhub/zero-to-jupyterhub-k8s/issues/3152 to follow that.
Hey @batpad I'm going on vacation for three weeks! Below are two documentation related review points, but besides those lean on other maintainers guidance - I'd love to see this approved and merged! Thank you for your work on this @batpad, I think it will be a very appreciated feature!!!
help
string documenting the functionality well enough via the configuration reference and reflecting the state to be merged?
.. versionchanged:: 6.1
to clarify new functionality was added in the profile_list
config was added at that point in time.help
string is rST formatted, not MyST markdown.pip install -r docs/requirements.txt
, cd docs
, make devenv
. I'm not sure if it auto-refreshes correctly and re-generates built documentation from the source code, maybe. Otherwise do rm -rf _build
and run again.Unfortunately, @batpad is a little sick, so I'm taking this on and trying to push it forward to completion now.
@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...
@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! 🎉
Fixes #699 .
This should eventually:
allow_other
, but I think @yuvipanda had some ideas to make that a bit nicer.allow_other
is true, show an additional option called "Other" in the dropdown and handle showing a text input when Other is selectedCurrently, 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