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

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

Merged
merged 39 commits into from
Jul 20, 2023
Merged
Show file tree
Hide file tree
Changes from 38 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
53f98f0
Refs #699 - allow user to specify free text image location. This commit:
batpad May 18, 2023
4cfe73e
use free_form object for configuration, handle frontend validation wi…
batpad May 18, 2023
62fb537
renames: free_form -> other_choice and match_regex -> validation_matc…
batpad May 20, 2023
f76918c
slightly messy but working implementation
batpad Jun 23, 2023
08dd78d
slightly ugly hack to ensure un-needed fields are not submitted with …
batpad Jun 23, 2023
f6ee8f8
add tests for handling choices and other-choice
batpad Jun 27, 2023
b09ce98
add validation for other-choice by checking against the validation re…
batpad Jun 27, 2023
cf4ed30
add some comments to the code
batpad Jun 27, 2023
2f64ab5
rename validation_match_regex to validation_regex
batpad Jun 28, 2023
29f5df5
convert -other-choice to --other-choice in form submission to prevent…
batpad Jun 28, 2023
eabd145
change value of other to other-choice in select dropdown
batpad Jun 28, 2023
4ae4009
fix error message for POST
batpad Jun 28, 2023
bfd51d4
fix test
batpad Jun 28, 2023
ed4c9ff
Merge branch 'allow-other' of github.com:batpad/kubespawner into allo…
batpad Jun 28, 2023
16f16b5
fix validation regex in config, clarify doc
batpad Jun 28, 2023
b9c74e0
fix bug where free form input would not show if Other was selected wh…
batpad Jun 28, 2023
872ad3e
allow validation_regex to be optional, add test
batpad Jun 28, 2023
930d838
Address PR feedback from @yuvipanda:
batpad Jul 1, 2023
f55e323
make other_choice_form_key a variable
batpad Jul 1, 2023
d15e3bf
Rename other-choice to unlisted-choice
yuvipanda Jul 12, 2023
116ad0c
Move options validation code into a function
yuvipanda Jul 12, 2023
68a71f9
Format with black
yuvipanda Jul 12, 2023
07fc3a9
Fix typo
yuvipanda Jul 12, 2023
6862aa0
Explicitly populate defaults of profile_list
yuvipanda Jul 19, 2023
a47f67e
Move code to get a profile from a slug into own function
yuvipanda Jul 19, 2023
36db652
Stop using self._profile_list
yuvipanda Jul 19, 2023
59507de
Document why the options_form default is the way it is
yuvipanda Jul 19, 2023
b1dba27
Remove extra call to _populate_profile_list_defaults
yuvipanda Jul 19, 2023
1763083
Clarify that the profile_list function is idempotent
yuvipanda Jul 19, 2023
3534920
Split code that applies overrides to its own function
yuvipanda Jul 19, 2023
6eb6e8a
Remove extra comment
yuvipanda Jul 19, 2023
96067e8
Actually write comment explaining default choice selection
yuvipanda Jul 19, 2023
cea4107
Reduce extra complexity in choosing default choice
yuvipanda Jul 19, 2023
dcbd152
Fix find profile by slug tests
yuvipanda Jul 19, 2023
783f240
Run pre-commit
yuvipanda Jul 19, 2023
a88f875
Add missing await to async function call
yuvipanda Jul 19, 2023
ff4216c
Handle empty profile_list
yuvipanda Jul 19, 2023
e8e8ceb
Handle profile_list being a function in a single place
yuvipanda Jul 19, 2023
243a5d7
Handle case of slug being None
yuvipanda Jul 19, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions jupyterhub_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,13 @@
'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}'},
},
'choices': {
'pytorch': {
'display_name': 'Python 3 Training Notebook',
Expand Down
263 changes: 182 additions & 81 deletions kubespawner/spawner.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import asyncio
import ipaddress
import os
import re
import string
import sys
import warnings
Expand Down Expand Up @@ -1537,6 +1538,17 @@ def _validate_image_pull_secrets(self, proposal):
and the value is a dictionary with the following keys:

- `display_name`: Name used to identify this particular option
- `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
batpad marked this conversation as resolved.
Show resolved Hide resolved
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
- `choices`: A dictionary containing list of choices for the user to choose from
to set the value for this particular option. The key is an identifier for this
choice, and the value is a dictionary with the following possible keys:
Expand Down Expand Up @@ -1573,6 +1585,15 @@ def _validate_image_pull_secrets(self, proposal):
'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}'
}
},
'choices': {
'pytorch': {
'display_name': 'Python 3 Training Notebook',
Expand Down Expand Up @@ -2905,10 +2926,8 @@ async def stop(self, now=False):
def _env_keep_default(self):
return []

_profile_list = None

def _render_options_form(self, profile_list):
self._profile_list = self._init_profile_list(profile_list)
profile_list = self._populate_profile_list_defaults(profile_list)

loader = ChoiceLoader(
[
Expand All @@ -2924,11 +2943,10 @@ def _render_options_form(self, profile_list):
profile_form_template = env.from_string(self.profile_form_template)
else:
profile_form_template = env.get_template("form.html")
return profile_form_template.render(profile_list=self._profile_list)
return profile_form_template.render(profile_list=profile_list)

async def _render_options_form_dynamically(self, current_spawner):
profile_list = await maybe_future(self.profile_list(current_spawner))
profile_list = self._init_profile_list(profile_list)
return self._render_options_form(profile_list)

@default('options_form')
Expand All @@ -2943,8 +2961,11 @@ def _options_form_default(self):
if not self.profile_list:
return ''
if callable(self.profile_list):
# Return the function dynamically, so JupyterHub will call this when the
# form needs rendering
return self._render_options_form_dynamically
else:
# Return the rendered string, as it does not change
return self._render_options_form(self.profile_list)

@default('options_from_form')
Expand Down Expand Up @@ -2991,38 +3012,83 @@ def _options_from_form(self, formdata):

return options

async def _load_profile(self, slug, selected_profile_user_options):
"""Load a profile by name
def _validate_posted_profile_options(self, profile, selected_options):
"""
Validate posted user options against the selected profile

The default form is rendered in such a way that each option specified in
the profile *must* have a value in the POST body. Extra options in the
POST body are ignored. We only honor options that are defined
in the selected profile *and* are in the form data
posted. This prevents users who may be authorized to only use
one profile from being able to access options set for other
profiles
"""
for option_name, option in profile.get('profile_options').items():
unlisted_choice_form_key = f'{option_name}--unlisted-choice'
if option_name not in selected_options:
# unlisted_choice is enabled:
if option.get('unlisted_choice', {}).get('enabled', False):
if unlisted_choice_form_key not in selected_options:
raise ValueError(
f'Expected option {option_name} for profile {profile["slug"]} or {unlisted_choice_form_key}, not found in posted form'
)
unlisted_choice = selected_options[unlisted_choice_form_key]

# Validate value of 'unlisted_choice' against validation regex
if profile.get('profile_options')[option_name][
'unlisted_choice'
].get('validation_regex', False):
unlisted_choice_validation_regex = profile.get(
'profile_options'
)[option_name]['unlisted_choice']['validation_regex']
if not re.match(
unlisted_choice_validation_regex, unlisted_choice
):
raise ValueError(
f'Value of {unlisted_choice_form_key} does not match validation regex.'
)
# unlisted_choice is Disabled
else:
raise ValueError(
f'Expected option {option_name} for profile {profile["slug"]}, not found in posted form'
)

Called by load_user_options
def _get_profile(self, slug: str, profile_list: list):
"""
Get the configured profile for given profile slug

# find the profile
default_profile = self._profile_list[0]
for profile in self._profile_list:
if profile.get('default', False):
# explicit default, not the first
default_profile = profile
Raises an error if no profile exists for the given slug.

if profile['slug'] == slug:
break
If slug is empty string, return the default profile
profile_list should already have all its defaults set.
"""
if slug != "":
for profile in profile_list:
if profile['slug'] == slug:
return profile

# A slug is specified, but not found
raise ValueError(
"No such profile: %s. Options include: %s"
% (slug, ', '.join(p['slug'] for p in profile_list))
)
else:
if slug:
# name specified, but not found
raise ValueError(
"No such profile: %s. Options include: %s"
% (slug, ', '.join(p['slug'] for p in self._profile_list))
)
else:
# no name specified, use the default
profile = default_profile
# slug is not specified, let's find the default and return it
# default is guaranteed to be set in at least one profile
return next(p for p in profile_list if p.get('default'))

self.log.debug(
"Applying KubeSpawner override for profile '%s'", profile['display_name']
)
async def _apply_overrides(self, spawner_override: dict):
"""
Apply set of overrides onto the current spawner instance

kubespawner_override = profile.get('kubespawner_override', {})
for k, v in kubespawner_override.items():
spawner_overrides is a dict with key being the name of the traitlet
to override, and value is either a callable or the value for the
traitlet. If the value is a dictionary, it is *merged* with the
existing value (rather than replaced). Callables are called with
one parameter - the current spawner instance.
"""
for k, v in spawner_override.items():
if callable(v):
v = v(self)
self.log.debug(
Expand All @@ -3040,68 +3106,102 @@ async def _load_profile(self, slug, selected_profile_user_options):
else:
setattr(self, k, v)

if profile.get('profile_options'):
# each option specified here *must* have a value in our POST, as we
# render our HTML such that there's always something selected.

# We only honor options that are defined in the selected profile *and*
# are in the form data posted. This prevents users who may be authorized
# to only use one profile from being able to access options set for other
# profiles
for user_selected_option_name in selected_profile_user_options.keys():
if (
user_selected_option_name
not in profile.get('profile_options').keys()
):
raise ValueError(
f'Expected option {user_selected_option_name} for profile {profile["slug"]}, not found in posted form'
)
async def _load_profile(self, slug, profile_list, selected_profile_user_options):
"""Load a profile by name

Called by load_user_options
"""
profile = self._get_profile(slug, profile_list)

self.log.debug(
"Applying KubeSpawner override for profile '%s'", profile['display_name']
)

await self._apply_overrides(profile.get('kubespawner_override', {}))

if profile.get('profile_options'):
self._validate_posted_profile_options(
profile, selected_profile_user_options
)
# Get selected options or default to the first option if none is passed
for option_name, option in profile.get('profile_options').items():
unlisted_choice_form_key = f'{option_name}--unlisted-choice'
chosen_option = selected_profile_user_options.get(option_name, None)
# If none was selected get the default
# If none was selected get the default. At least one choice is
# guaranteed to have the default set
if not chosen_option:
default_option = list(option['choices'].keys())[0]
for choice_name, choice in option['choices'].items():
if choice.get('default', False):
# explicit default, not the first
default_option = choice_name
chosen_option = default_option
chosen_option = choice_name

chosen_option_overrides = option['choices'][chosen_option][
'kubespawner_override'
]
for k, v in chosen_option_overrides.items():
if callable(v):
v = await maybe_future(v(self))
self.log.debug(
f'.. overriding traitlet {k}={v} for option {option_name}={chosen_option} from callabale'
)
else:
self.log.debug(
f'.. overriding traitlet {k}={v} for option {option_name}={chosen_option}'
# Handle override for unlisted_choice free text specified by user
if (
option.get('unlisted_choice', {}).get('enabled', False)
and unlisted_choice_form_key in selected_profile_user_options
):
chosen_option_overrides = option['unlisted_choice'][
'kubespawner_override'
]
for k, v in chosen_option_overrides.items():
chosen_option_overrides[k] = v.format(
value=selected_profile_user_options[
unlisted_choice_form_key
]
)
else:
chosen_option_overrides = option['choices'][chosen_option][
'kubespawner_override'
]

# If v is a dict, *merge* it with existing values, rather than completely
# resetting it. This allows *adding* things like environment variables rather
# than completely replacing them. If value is set to None, the key
# will be removed
if isinstance(v, dict) and isinstance(getattr(self, k), dict):
recursive_update(getattr(self, k), v)
else:
setattr(self, k, v)
await self._apply_overrides(chosen_option_overrides)

# set of recognised user option keys
# used for warning about ignoring unrecognised options
_user_option_keys = {'profile'}

def _init_profile_list(self, profile_list):
# generate missing slug fields from display_name
def _populate_profile_list_defaults(self, profile_list: list):
"""
Return a fully realized profile_list

This will augment any missing fields to appropriate values.
- If 'slug' is not set for profiles, generate it automatically
from display_name
- If profile_options are present with choices, but no choice is set
as the default, set the first choice to be the default.
- If no default profile is set, the first profile is set to be the
default

The profile_list passed in is mutated and returned.

This function is *idempotent*, you can pass the same profile_list
through it as many times without any problems.
"""
if not profile_list:
# empty profile lists are just returned unmodified
return profile_list

for profile in profile_list:
# generate missing slug fields from display_name
if 'slug' not in profile:
profile['slug'] = slugify(profile['display_name'])

# If profile_options are present with choices, but no default choice
# is specified, we make the first choice the default
for option_config in profile.get('profile_options', {}).values():
if option_config.get('choices'):
# Don't do anything if choices are not present, and only unlisted_choice
# is used.
if not any(
c.get('default') for c in option_config['choices'].values()
):
# No explicit default is set
default_choice = list(option_config['choices'].keys())[0]
option_config['choices'][default_choice]["default"] = True

if not any(p.get("default") for p in profile_list):
# No profile has 'default' explicitly set, we set it for the first profile in the List
profile_list[0]["default"] = True

return profile_list

async def load_user_options(self):
Expand All @@ -3115,22 +3215,23 @@ async def load_user_options(self):
Override in subclasses to support other options.
"""

if self._profile_list is None:
if callable(self.profile_list):
profile_list = await maybe_future(self.profile_list(self))
else:
profile_list = self.profile_list
if callable(self.profile_list):
profile_list = await maybe_future(self.profile_list(self))
else:
profile_list = self.profile_list

self._profile_list = self._init_profile_list(profile_list)
profile_list = self._populate_profile_list_defaults(profile_list)

selected_profile = self.user_options.get('profile', None)
selected_profile_user_options = dict(self.user_options)
if selected_profile:
# Remove the 'profile' key so we are left with only selected profile options
del selected_profile_user_options['profile']

if self._profile_list:
await self._load_profile(selected_profile, selected_profile_user_options)
if profile_list:
await self._load_profile(
selected_profile, profile_list, selected_profile_user_options
)
elif selected_profile:
self.log.warning(
"Profile %r requested, but profiles are not enabled", selected_profile
Expand Down
Loading