From 53f98f07cc3c6e07225a84f34483fec44e9d0b88 Mon Sep 17 00:00:00 2001 From: Sanjay Bhangar Date: Thu, 18 May 2023 13:42:38 +0530 Subject: [PATCH 01/38] Refs #699 - allow user to specify free text image location. This commit: - Creates an option called 'allow_other' - If allow_other, show an extra option for Other in the dropdown - If Other is selected, show an input field for user to type free text. TODO: handle POSTing of "other" input on the backend --- jupyterhub_config.py | 1 + kubespawner/spawner.py | 4 ++++ kubespawner/templates/form.html | 39 +++++++++++++++++++++++++-------- kubespawner/templates/style.css | 4 ++++ 4 files changed, 39 insertions(+), 9 deletions(-) diff --git a/jupyterhub_config.py b/jupyterhub_config.py index 0f960c85..b149b6c5 100644 --- a/jupyterhub_config.py +++ b/jupyterhub_config.py @@ -46,6 +46,7 @@ 'profile_options': { 'image': { 'display_name': 'Image', + 'allow_other': True, 'choices': { 'pytorch': { 'display_name': 'Python 3 Training Notebook', diff --git a/kubespawner/spawner.py b/kubespawner/spawner.py index 9e88ee0c..e4919192 100644 --- a/kubespawner/spawner.py +++ b/kubespawner/spawner.py @@ -1537,6 +1537,9 @@ 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 + - `allow_other`: Boolean, defines whether the select drop-down showing choices will show + an "Other" value. If "Other" is selected, the user will be given an input box. Defaults + to False. - `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: @@ -1573,6 +1576,7 @@ def _validate_image_pull_secrets(self, proposal): 'profile_options': { 'image': { 'display_name': 'Image', + 'allow_other': True, 'choices': { 'pytorch': { 'display_name': 'Python 3 Training Notebook', diff --git a/kubespawner/templates/form.html b/kubespawner/templates/form.html index 69464514..dc8c0004 100644 --- a/kubespawner/templates/form.html +++ b/kubespawner/templates/form.html @@ -17,15 +17,24 @@

{{ profile.display_name }}

{%- if profile.profile_options %}
{%- for k, option in profile.profile_options.items() %} -
- - +
+
+ + +
+ {%- if option['allow_other'] %} +
+ + +
+ {%- endif %}
{%- endfor %}
@@ -41,4 +50,16 @@

{{ profile.display_name }}

.find('input[type=radio]') .prop('checked', true); }); + + // If the user selects "Other" in the dropdown, show the free text field + $('.js-profile-option-select').change(function() { + const selectionValue = $(this).val(); + const $otherInputContainer = $(this).parents('.js-options-container').find('.js-other-input-container'); + if (selectionValue === 'other') { + $otherInputContainer.removeClass('hide'); + } else { + $otherInputContainer.addClass('hide'); + } + }); + diff --git a/kubespawner/templates/style.css b/kubespawner/templates/style.css index 5538e9fe..aab4fe4e 100644 --- a/kubespawner/templates/style.css +++ b/kubespawner/templates/style.css @@ -27,3 +27,7 @@ margin-right: 8px; min-width: 96px; } + +.hide { + display: none; +} From 4cfe73e5e30269b6afb22da952a1ef903fa415c0 Mon Sep 17 00:00:00 2001 From: Sanjay Bhangar Date: Thu, 18 May 2023 18:27:55 +0530 Subject: [PATCH 02/38] use free_form object for configuration, handle frontend validation with configurable regex --- jupyterhub_config.py | 8 +++++++- kubespawner/spawner.py | 23 +++++++++++++++++++---- kubespawner/templates/form.html | 11 +++++++---- 3 files changed, 33 insertions(+), 9 deletions(-) diff --git a/jupyterhub_config.py b/jupyterhub_config.py index b149b6c5..78fcd4c0 100644 --- a/jupyterhub_config.py +++ b/jupyterhub_config.py @@ -46,7 +46,13 @@ 'profile_options': { 'image': { 'display_name': 'Image', - 'allow_other': True, + 'free_form': { + 'enabled': True, + 'display_name': 'Image Location', + 'match_regex': '^pangeo/.*$', + 'validation_message': 'Must be a pangeo image, matching ^pangeo/.*$', + 'kubespawner_override': {'image': '{value}'}, + }, 'choices': { 'pytorch': { 'display_name': 'Python 3 Training Notebook', diff --git a/kubespawner/spawner.py b/kubespawner/spawner.py index e4919192..8b0d1b9a 100644 --- a/kubespawner/spawner.py +++ b/kubespawner/spawner.py @@ -1537,9 +1537,16 @@ 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 - - `allow_other`: Boolean, defines whether the select drop-down showing choices will show - an "Other" value. If "Other" is selected, the user will be given an input box. Defaults - to False. + - `free_form`: Object to specify if there should be a free-form field if the user + select "Other" as a choice: + - `enabled`: Boolean, whether the free form input should be enabled + - `display_name`: String, label for input field + - `match_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 + - 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: @@ -1576,7 +1583,15 @@ def _validate_image_pull_secrets(self, proposal): 'profile_options': { 'image': { 'display_name': 'Image', - 'allow_other': True, + 'free_form': { + 'enabled': true, + 'display_name': 'Image Location', + 'match_regex': '^pangeo/.*$', + 'validation_message': 'Must be a pangeo image, matching ^pangeo/.*$', + 'kubespawner_override': { + 'image': '{value}' + } + }, 'choices': { 'pytorch': { 'display_name': 'Python 3 Training Notebook', diff --git a/kubespawner/templates/form.html b/kubespawner/templates/form.html index dc8c0004..6e3791b9 100644 --- a/kubespawner/templates/form.html +++ b/kubespawner/templates/form.html @@ -26,13 +26,16 @@

{{ profile.display_name }}

{%- for k, choice in option['choices'].items() %} {%- endfor %} - {%- if option['allow_other'] %}{%- endif %} + {%- if option['free_form'] and option['free_form']['enabled'] %}{%- endif %}
- {%- if option['allow_other'] %} + {%- if option['free_form'] and option['free_form']['enabled'] %}
- - + +
{%- endif %} From 62fb537de5f1df9d8a49a9af7803159ffea67fe8 Mon Sep 17 00:00:00 2001 From: Sanjay Bhangar Date: Sat, 20 May 2023 11:35:24 +0530 Subject: [PATCH 03/38] renames: free_form -> other_choice and match_regex -> validation_match_regex --- jupyterhub_config.py | 4 ++-- kubespawner/spawner.py | 8 ++++---- kubespawner/templates/form.html | 16 ++++++++++------ 3 files changed, 16 insertions(+), 12 deletions(-) diff --git a/jupyterhub_config.py b/jupyterhub_config.py index 78fcd4c0..bc87fcc1 100644 --- a/jupyterhub_config.py +++ b/jupyterhub_config.py @@ -46,10 +46,10 @@ 'profile_options': { 'image': { 'display_name': 'Image', - 'free_form': { + 'other_choice': { 'enabled': True, 'display_name': 'Image Location', - 'match_regex': '^pangeo/.*$', + 'validation_match_regex': '^pangeo/.*$', 'validation_message': 'Must be a pangeo image, matching ^pangeo/.*$', 'kubespawner_override': {'image': '{value}'}, }, diff --git a/kubespawner/spawner.py b/kubespawner/spawner.py index 8b0d1b9a..b3c2c328 100644 --- a/kubespawner/spawner.py +++ b/kubespawner/spawner.py @@ -1537,11 +1537,11 @@ 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 - - `free_form`: Object to specify if there should be a free-form field if the user + - `other_choice`: Object to specify if there should be a free-form field if the user select "Other" as a choice: - `enabled`: Boolean, whether the free form input should be enabled - `display_name`: String, label for input field - - `match_regex`: Optional, regex that the free form input should match - eg. ^pangeo/.*$ + - `validation_match_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 @@ -1583,10 +1583,10 @@ def _validate_image_pull_secrets(self, proposal): 'profile_options': { 'image': { 'display_name': 'Image', - 'free_form': { + 'other_choice': { 'enabled': true, 'display_name': 'Image Location', - 'match_regex': '^pangeo/.*$', + 'validation_match_regex': '^pangeo/.*$', 'validation_message': 'Must be a pangeo image, matching ^pangeo/.*$', 'kubespawner_override': { 'image': '{value}' diff --git a/kubespawner/templates/form.html b/kubespawner/templates/form.html index 6e3791b9..c200fd14 100644 --- a/kubespawner/templates/form.html +++ b/kubespawner/templates/form.html @@ -26,15 +26,19 @@

{{ profile.display_name }}

{%- for k, choice in option['choices'].items() %} {%- endfor %} - {%- if option['free_form'] and option['free_form']['enabled'] %}{%- endif %} + {%- if option['other_choice'] and option['other_choice']['enabled'] %} + + {%- endif %} - {%- if option['free_form'] and option['free_form']['enabled'] %} + {%- if option['other_choice'] and option['other_choice']['enabled'] %}
- - + {{ option['other_choice']['display_name'] }} + +
{%- endif %} From f76918cc4fb8aba391ee7b5313ae3e8e7c0c9647 Mon Sep 17 00:00:00 2001 From: Sanjay Bhangar Date: Thu, 22 Jun 2023 18:41:43 -0700 Subject: [PATCH 04/38] slightly messy but working implementation --- kubespawner/spawner.py | 46 ++++++++++++++++++++++++++++++++---------- 1 file changed, 35 insertions(+), 11 deletions(-) diff --git a/kubespawner/spawner.py b/kubespawner/spawner.py index b3c2c328..2d0664c9 100644 --- a/kubespawner/spawner.py +++ b/kubespawner/spawner.py @@ -3067,14 +3067,23 @@ async def _load_profile(self, slug, selected_profile_user_options): # 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' - ) + + for option_name, option in profile.get('profile_options').items(): + if option_name not in selected_profile_user_options: + # other_choice in Enabled: + if option.get('other_choice', {}).get('enabled', False): + if ( + f'{option_name}-other-choice' + not in selected_profile_user_options + ): + raise ValueError( + f'Expected option {option_name} for profile {profile["slug"]} or -other-choice, not found in posted form' + ) + # other_choice is Disabled + else: + raise ValueError( + f'Expected option {option_name} for profile {profile["slug"]}, not found in posted form' + ) # Get selected options or default to the first option if none is passed for option_name, option in profile.get('profile_options').items(): @@ -3088,9 +3097,24 @@ async def _load_profile(self, slug, selected_profile_user_options): default_option = choice_name chosen_option = default_option - chosen_option_overrides = option['choices'][chosen_option][ - 'kubespawner_override' - ] + if ( + option.get('other_choice', {}).get('enabled', False) + and f'{option_name}-other-choice' in selected_profile_user_options + ): + chosen_option_overrides = option['other_choice'][ + 'kubespawner_override' + ] + for k, v in chosen_option_overrides.items(): + chosen_option_overrides[k] = v.format( + value=selected_profile_user_options[ + f'{option_name}-other-choice' + ] + ) + else: + 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)) From 08dd78d92c6cbc9b5e2fbca33a8516a356f95e07 Mon Sep 17 00:00:00 2001 From: Sanjay Bhangar Date: Fri, 23 Jun 2023 12:11:46 -0700 Subject: [PATCH 05/38] slightly ugly hack to ensure un-needed fields are not submitted with the POST, based on if the user has selected Other --- kubespawner/templates/form.html | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/kubespawner/templates/form.html b/kubespawner/templates/form.html index c200fd14..39bb0edc 100644 --- a/kubespawner/templates/form.html +++ b/kubespawner/templates/form.html @@ -36,10 +36,10 @@

{{ profile.display_name }}

- + class="form-control js-other-input" /> {%- endif %} @@ -62,11 +62,18 @@

{{ profile.display_name }}

$('.js-profile-option-select').change(function() { const selectionValue = $(this).val(); const $otherInputContainer = $(this).parents('.js-options-container').find('.js-other-input-container'); + const $otherInput = $(this).parents('.js-options-container').find('.js-other-input'); if (selectionValue === 'other') { + $(this).data('name', $(this).attr('name')); + $(this).removeAttr('name'); + $otherInput.attr('name', $otherInput.data('name')); $otherInputContainer.removeClass('hide'); } else { + $otherInput.removeAttr('name'); + $(this).attr('name', $(this).data('name')); $otherInputContainer.addClass('hide'); } }); + From f6ee8f80864111de18ae7930db537409c3d4d44f Mon Sep 17 00:00:00 2001 From: Sanjay Bhangar Date: Mon, 26 Jun 2023 18:09:07 -0700 Subject: [PATCH 06/38] add tests for handling choices and other-choice --- tests/test_spawner.py | 67 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/tests/test_spawner.py b/tests/test_spawner.py index 3a9b09e2..2d306c4a 100644 --- a/tests/test_spawner.py +++ b/tests/test_spawner.py @@ -881,6 +881,35 @@ async def test_get_pod_manifest_tolerates_mixed_input(): 'environment': {'override': 'override-value', "to-remove": None}, }, }, + { + 'display_name': 'Test choices', + 'slug': 'test-choices', + 'profile_options': { + 'image': { + 'display_name': 'Image', + 'other_choice': { + 'enabled': True, + 'display_name': 'Image Location', + 'validation_match_regex': '^pangeo/.*$', + 'validation_message': 'Must be a pangeo image, matching ^pangeo/.*$', + 'kubespawner_override': {'image': '{value}'}, + }, + 'choices': { + 'pytorch': { + 'display_name': 'Python 3 Training Notebook', + 'kubespawner_override': { + 'image': 'pangeo/pytorch-notebook:master' + }, + }, + 'tf': { + 'display_name': 'R 4.2 Training Notebook', + 'default': True, + 'kubespawner_override': {'image': 'training/r:label'}, + }, + }, + }, + }, + }, ] @@ -902,6 +931,44 @@ async def test_user_options_set_from_form(): assert getattr(spawner, key) == value +async def test_user_options_set_from_form_choices(): + spawner = KubeSpawner(_mock=True) + spawner.profile_list = _test_profiles + await spawner.get_options_form() + spawner.user_options = spawner.options_from_form( + { + 'profile': [_test_profiles[3]['slug']], + 'profile-option-test-choices-image': ['pytorch'], + } + ) + assert spawner.user_options == { + 'image': 'pytorch', + 'profile': _test_profiles[3]['slug'], + } + assert spawner.cpu_limit is None + await spawner.load_user_options() + assert getattr(spawner, 'image') == 'pangeo/pytorch-notebook:master' + + +async def test_user_options_set_from_form_other_choice(): + spawner = KubeSpawner(_mock=True) + spawner.profile_list = _test_profiles + await spawner.get_options_form() + spawner.user_options = spawner.options_from_form( + { + 'profile': [_test_profiles[3]['slug']], + 'profile-option-test-choices-image-other-choice': ['pangeo/test:latest'], + } + ) + assert spawner.user_options == { + 'image-other-choice': 'pangeo/test:latest', + 'profile': _test_profiles[3]['slug'], + } + assert spawner.cpu_limit is None + await spawner.load_user_options() + assert getattr(spawner, 'image') == 'pangeo/test:latest' + + async def test_kubespawner_override(): spawner = KubeSpawner(_mock=True) spawner.profile_list = _test_profiles From b09ce984776068b8c7e7fdd103c69f874ce3b7ec Mon Sep 17 00:00:00 2001 From: Sanjay Bhangar Date: Mon, 26 Jun 2023 18:34:41 -0700 Subject: [PATCH 07/38] add validation for other-choice by checking against the validation regex in the backend + add test --- kubespawner/spawner.py | 12 ++++++++++++ tests/test_spawner.py | 20 ++++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/kubespawner/spawner.py b/kubespawner/spawner.py index 2d0664c9..e2fa372b 100644 --- a/kubespawner/spawner.py +++ b/kubespawner/spawner.py @@ -7,6 +7,7 @@ import asyncio import ipaddress import os +import re import string import sys import warnings @@ -3079,6 +3080,17 @@ async def _load_profile(self, slug, selected_profile_user_options): raise ValueError( f'Expected option {option_name} for profile {profile["slug"]} or -other-choice, not found in posted form' ) + other_choice = selected_profile_user_options[ + f'{option_name}-other-choice' + ] + other_choice_validation_regex = profile.get('profile_options')[ + option_name + ]['other_choice']['validation_match_regex'] + regex = re.compile(other_choice_validation_regex) + if not regex.match(other_choice): + raise ValueError( + f'Value of {option_name}-other-choice does not match validation regex.' + ) # other_choice is Disabled else: raise ValueError( diff --git a/tests/test_spawner.py b/tests/test_spawner.py index 2d306c4a..9c5b2e15 100644 --- a/tests/test_spawner.py +++ b/tests/test_spawner.py @@ -969,6 +969,26 @@ async def test_user_options_set_from_form_other_choice(): assert getattr(spawner, 'image') == 'pangeo/test:latest' +async def test_user_options_set_from_form_invalid_regex(): + spawner = KubeSpawner(_mock=True) + spawner.profile_list = _test_profiles + await spawner.get_options_form() + spawner.user_options = spawner.options_from_form( + { + 'profile': [_test_profiles[3]['slug']], + 'profile-option-test-choices-image-other-choice': ['invalid/foo:latest'], + } + ) + assert spawner.user_options == { + 'image-other-choice': 'invalid/foo:latest', + 'profile': _test_profiles[3]['slug'], + } + assert spawner.cpu_limit is None + + with pytest.raises(ValueError): + await spawner.load_user_options() + + async def test_kubespawner_override(): spawner = KubeSpawner(_mock=True) spawner.profile_list = _test_profiles From cf4ed30bb42144eaed6376d7492b836cf2d16f18 Mon Sep 17 00:00:00 2001 From: Sanjay Bhangar Date: Mon, 26 Jun 2023 18:38:44 -0700 Subject: [PATCH 08/38] add some comments to the code --- kubespawner/spawner.py | 3 +++ kubespawner/templates/form.html | 9 +++++++++ 2 files changed, 12 insertions(+) diff --git a/kubespawner/spawner.py b/kubespawner/spawner.py index e2fa372b..d2716a44 100644 --- a/kubespawner/spawner.py +++ b/kubespawner/spawner.py @@ -3083,6 +3083,8 @@ async def _load_profile(self, slug, selected_profile_user_options): other_choice = selected_profile_user_options[ f'{option_name}-other-choice' ] + + # Validate value of 'other-choice' against validation regex other_choice_validation_regex = profile.get('profile_options')[ option_name ]['other_choice']['validation_match_regex'] @@ -3109,6 +3111,7 @@ async def _load_profile(self, slug, selected_profile_user_options): default_option = choice_name chosen_option = default_option + # Handle override for other-choice free text specified by user if ( option.get('other_choice', {}).get('enabled', False) and f'{option_name}-other-choice' in selected_profile_user_options diff --git a/kubespawner/templates/form.html b/kubespawner/templates/form.html index 39bb0edc..9da10bea 100644 --- a/kubespawner/templates/form.html +++ b/kubespawner/templates/form.html @@ -64,6 +64,15 @@

{{ profile.display_name }}

const $otherInputContainer = $(this).parents('.js-options-container').find('.js-other-input-container'); const $otherInput = $(this).parents('.js-options-container').find('.js-other-input'); if (selectionValue === 'other') { + + // if the Select box is selected with a value that is not 'other', + // we remove the "name" attribute for the other-choice input + // and vice-versa, so that we only submit valid form values + // and don't leave the complexity of figuring out which value + // to use to the back-end. + + // This can probably be done cleaner by completely refactoring + // how we send values from the frontend. $(this).data('name', $(this).attr('name')); $(this).removeAttr('name'); $otherInput.attr('name', $otherInput.data('name')); From 2f64ab5e9f8e161d23d0cf5cfddc9514fa3c77a6 Mon Sep 17 00:00:00 2001 From: Sanjay Bhangar Date: Tue, 27 Jun 2023 18:08:19 -0700 Subject: [PATCH 09/38] rename validation_match_regex to validation_regex --- kubespawner/templates/form.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kubespawner/templates/form.html b/kubespawner/templates/form.html index 9da10bea..6ac57644 100644 --- a/kubespawner/templates/form.html +++ b/kubespawner/templates/form.html @@ -37,7 +37,7 @@

{{ profile.display_name }}

{{ option['other_choice']['display_name'] }} From 29f5df5c93806c081eea747691c8841f2d320be0 Mon Sep 17 00:00:00 2001 From: Sanjay Bhangar Date: Tue, 27 Jun 2023 18:16:51 -0700 Subject: [PATCH 10/38] convert -other-choice to --other-choice in form submission to prevent conflicting names --- kubespawner/spawner.py | 18 +++++++++--------- kubespawner/templates/form.html | 4 ++-- tests/test_spawner.py | 8 ++++---- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/kubespawner/spawner.py b/kubespawner/spawner.py index d2716a44..193dd8d8 100644 --- a/kubespawner/spawner.py +++ b/kubespawner/spawner.py @@ -1542,7 +1542,7 @@ def _validate_image_pull_secrets(self, proposal): select "Other" as a choice: - `enabled`: Boolean, whether the free form input should be enabled - `display_name`: String, label for input field - - `validation_match_regex`: Optional, regex that the free form input should match - eg. ^pangeo/.*$ + - `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 @@ -1587,7 +1587,7 @@ def _validate_image_pull_secrets(self, proposal): 'other_choice': { 'enabled': true, 'display_name': 'Image Location', - 'validation_match_regex': '^pangeo/.*$', + 'validation_regex': '^pangeo/.*$', 'validation_message': 'Must be a pangeo image, matching ^pangeo/.*$', 'kubespawner_override': { 'image': '{value}' @@ -3074,24 +3074,24 @@ async def _load_profile(self, slug, selected_profile_user_options): # other_choice in Enabled: if option.get('other_choice', {}).get('enabled', False): if ( - f'{option_name}-other-choice' + f'{option_name}--other-choice' not in selected_profile_user_options ): raise ValueError( - f'Expected option {option_name} for profile {profile["slug"]} or -other-choice, not found in posted form' + f'Expected option {option_name} for profile {profile["slug"]} or --other-choice, not found in posted form' ) other_choice = selected_profile_user_options[ - f'{option_name}-other-choice' + f'{option_name}--other-choice' ] # Validate value of 'other-choice' against validation regex other_choice_validation_regex = profile.get('profile_options')[ option_name - ]['other_choice']['validation_match_regex'] + ]['other_choice']['validation_regex'] regex = re.compile(other_choice_validation_regex) if not regex.match(other_choice): raise ValueError( - f'Value of {option_name}-other-choice does not match validation regex.' + f'Value of {option_name}--other-choice does not match validation regex.' ) # other_choice is Disabled else: @@ -3114,7 +3114,7 @@ async def _load_profile(self, slug, selected_profile_user_options): # Handle override for other-choice free text specified by user if ( option.get('other_choice', {}).get('enabled', False) - and f'{option_name}-other-choice' in selected_profile_user_options + and f'{option_name}--other-choice' in selected_profile_user_options ): chosen_option_overrides = option['other_choice'][ 'kubespawner_override' @@ -3122,7 +3122,7 @@ async def _load_profile(self, slug, selected_profile_user_options): for k, v in chosen_option_overrides.items(): chosen_option_overrides[k] = v.format( value=selected_profile_user_options[ - f'{option_name}-other-choice' + f'{option_name}--other-choice' ] ) else: diff --git a/kubespawner/templates/form.html b/kubespawner/templates/form.html index 6ac57644..1e69982a 100644 --- a/kubespawner/templates/form.html +++ b/kubespawner/templates/form.html @@ -33,10 +33,10 @@

{{ profile.display_name }}

{%- if option['other_choice'] and option['other_choice']['enabled'] %}
-
@@ -63,7 +63,7 @@

{{ profile.display_name }}

const selectionValue = $(this).val(); const $otherInputContainer = $(this).parents('.js-options-container').find('.js-other-input-container'); const $otherInput = $(this).parents('.js-options-container').find('.js-other-input'); - if (selectionValue === 'other') { + if (selectionValue === 'other-choice') { // if the Select box is selected with a value that is not 'other', // we remove the "name" attribute for the other-choice input From 4ae4009aef52266937cfa3cfd37c4cca6fa241de Mon Sep 17 00:00:00 2001 From: Sanjay Bhangar Date: Tue, 27 Jun 2023 18:22:35 -0700 Subject: [PATCH 12/38] fix error message for POST Co-authored-by: Yuvi Panda --- kubespawner/spawner.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kubespawner/spawner.py b/kubespawner/spawner.py index 193dd8d8..b3b4e634 100644 --- a/kubespawner/spawner.py +++ b/kubespawner/spawner.py @@ -3078,7 +3078,7 @@ async def _load_profile(self, slug, selected_profile_user_options): not in selected_profile_user_options ): raise ValueError( - f'Expected option {option_name} for profile {profile["slug"]} or --other-choice, not found in posted form' + f'Expected option {option_name} for profile {profile["slug"]} or {option_name}--other-choice, not found in posted form' ) other_choice = selected_profile_user_options[ f'{option_name}--other-choice' From bfd51d47e7101d8828507fb6e7386de9c9ddb27b Mon Sep 17 00:00:00 2001 From: Sanjay Bhangar Date: Tue, 27 Jun 2023 18:27:44 -0700 Subject: [PATCH 13/38] fix test --- tests/test_spawner.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_spawner.py b/tests/test_spawner.py index ee5fc553..de489c27 100644 --- a/tests/test_spawner.py +++ b/tests/test_spawner.py @@ -980,7 +980,7 @@ async def test_user_options_set_from_form_invalid_regex(): } ) assert spawner.user_options == { - 'image-other-choice': 'invalid/foo:latest', + 'image--other-choice': 'invalid/foo:latest', 'profile': _test_profiles[3]['slug'], } assert spawner.cpu_limit is None From 16f16b54b59be91c4f17690b12cb59d956a68930 Mon Sep 17 00:00:00 2001 From: Sanjay Bhangar Date: Tue, 27 Jun 2023 18:56:20 -0700 Subject: [PATCH 14/38] fix validation regex in config, clarify doc --- jupyterhub_config.py | 2 +- kubespawner/spawner.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/jupyterhub_config.py b/jupyterhub_config.py index bc87fcc1..8f58be49 100644 --- a/jupyterhub_config.py +++ b/jupyterhub_config.py @@ -49,7 +49,7 @@ 'other_choice': { 'enabled': True, 'display_name': 'Image Location', - 'validation_match_regex': '^pangeo/.*$', + 'validation_regex': '^pangeo/.*$', 'validation_message': 'Must be a pangeo image, matching ^pangeo/.*$', 'kubespawner_override': {'image': '{value}'}, }, diff --git a/kubespawner/spawner.py b/kubespawner/spawner.py index b3b4e634..e34f2c68 100644 --- a/kubespawner/spawner.py +++ b/kubespawner/spawner.py @@ -1542,7 +1542,7 @@ def _validate_image_pull_secrets(self, proposal): select "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_regex`: Required if enabled is True, 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 From b9c74e016b67fa6b7be8ce9bb97dc9965b12133a Mon Sep 17 00:00:00 2001 From: Sanjay Bhangar Date: Tue, 27 Jun 2023 20:08:58 -0700 Subject: [PATCH 15/38] fix bug where free form input would not show if Other was selected when page loaded --- kubespawner/templates/form.html | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/kubespawner/templates/form.html b/kubespawner/templates/form.html index 93516ce7..4f953405 100644 --- a/kubespawner/templates/form.html +++ b/kubespawner/templates/form.html @@ -65,7 +65,7 @@

{{ profile.display_name }}

const $otherInput = $(this).parents('.js-options-container').find('.js-other-input'); if (selectionValue === 'other-choice') { - // if the Select box is selected with a value that is not 'other', + // if the Select box is selected with a value that is not 'other-choice', // we remove the "name" attribute for the other-choice input // and vice-versa, so that we only submit valid form values // and don't leave the complexity of figuring out which value @@ -84,5 +84,13 @@

{{ profile.display_name }}

} }); + // wrapping in a document.ready to make clear this is executed once + // on page load + $(document).ready(() => { + // trigger the `change` event on the select inputs, + // so that if "Other" is selected on page load, the corresponding + // free-text input shows + $('.js-profile-option-select').trigger('change'); + }); From 872ad3e190330c2925c915ab61a52c0c5aea2c88 Mon Sep 17 00:00:00 2001 From: Sanjay Bhangar Date: Tue, 27 Jun 2023 20:54:24 -0700 Subject: [PATCH 16/38] allow validation_regex to be optional, add test --- kubespawner/spawner.py | 21 ++++++++------- kubespawner/templates/form.html | 2 ++ tests/test_spawner.py | 47 +++++++++++++++++++++++++++++++++ 3 files changed, 61 insertions(+), 9 deletions(-) diff --git a/kubespawner/spawner.py b/kubespawner/spawner.py index e34f2c68..878941ef 100644 --- a/kubespawner/spawner.py +++ b/kubespawner/spawner.py @@ -1542,7 +1542,7 @@ def _validate_image_pull_secrets(self, proposal): select "Other" as a choice: - `enabled`: Boolean, whether the free form input should be enabled - `display_name`: String, label for input field - - `validation_regex`: Required if enabled is True, regex that the free form input should match - eg. ^pangeo/.*$ + - `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 @@ -3085,14 +3085,17 @@ async def _load_profile(self, slug, selected_profile_user_options): ] # Validate value of 'other-choice' against validation regex - other_choice_validation_regex = profile.get('profile_options')[ - option_name - ]['other_choice']['validation_regex'] - regex = re.compile(other_choice_validation_regex) - if not regex.match(other_choice): - raise ValueError( - f'Value of {option_name}--other-choice does not match validation regex.' - ) + if profile.get('profile_options')[option_name][ + 'other_choice' + ].get('validation_regex', False): + other_choice_validation_regex = profile.get( + 'profile_options' + )[option_name]['other_choice']['validation_regex'] + regex = re.compile(other_choice_validation_regex) + if not regex.match(other_choice): + raise ValueError( + f'Value of {option_name}--other-choice does not match validation regex.' + ) # other_choice is Disabled else: raise ValueError( diff --git a/kubespawner/templates/form.html b/kubespawner/templates/form.html index 4f953405..88783cdf 100644 --- a/kubespawner/templates/form.html +++ b/kubespawner/templates/form.html @@ -37,7 +37,9 @@

{{ profile.display_name }}

{{ option['other_choice']['display_name'] }} diff --git a/tests/test_spawner.py b/tests/test_spawner.py index de489c27..fce77d6e 100644 --- a/tests/test_spawner.py +++ b/tests/test_spawner.py @@ -910,6 +910,33 @@ async def test_get_pod_manifest_tolerates_mixed_input(): }, }, }, + { + 'display_name': 'Test choices no regex', + 'slug': 'no-regex', + 'profile_options': { + 'image': { + 'display_name': 'Image', + 'other_choice': { + 'enabled': True, + 'display_name': 'Image Location', + 'kubespawner_override': {'image': '{value}'}, + }, + 'choices': { + 'pytorch': { + 'display_name': 'Python 3 Training Notebook', + 'kubespawner_override': { + 'image': 'pangeo/pytorch-notebook:master' + }, + }, + 'tf': { + 'display_name': 'R 4.2 Training Notebook', + 'default': True, + 'kubespawner_override': {'image': 'training/r:label'}, + }, + }, + }, + }, + }, ] @@ -989,6 +1016,26 @@ async def test_user_options_set_from_form_invalid_regex(): await spawner.load_user_options() +async def test_user_options_set_from_form_no_regex(): + spawner = KubeSpawner(_mock=True) + spawner.profile_list = _test_profiles + await spawner.get_options_form() + # print(_test_profiles[4]) + spawner.user_options = spawner.options_from_form( + { + 'profile': [_test_profiles[4]['slug']], + 'profile-option-no-regex-image--other-choice': ['invalid/foo:latest'], + } + ) + assert spawner.user_options == { + 'image--other-choice': 'invalid/foo:latest', + 'profile': _test_profiles[4]['slug'], + } + assert spawner.cpu_limit is None + await spawner.load_user_options() + assert getattr(spawner, 'image') == 'invalid/foo:latest' + + async def test_kubespawner_override(): spawner = KubeSpawner(_mock=True) spawner.profile_list = _test_profiles From 930d8381fafb83a970e1af5fa73dbda69bcc272b Mon Sep 17 00:00:00 2001 From: Sanjay Bhangar Date: Fri, 30 Jun 2023 20:06:36 -0700 Subject: [PATCH 17/38] Address PR feedback from @yuvipanda: - Make docs for kubespawner_override in other_choice better - fix typo in docs - `in Enabled` -> `is enabled` - Fix re.match usage - Use bootstrap hidden class - Add docstrings to tests --- kubespawner/spawner.py | 10 ++++++---- kubespawner/templates/form.html | 6 +++--- kubespawner/templates/style.css | 4 ---- tests/test_spawner.py | 17 +++++++++++++++++ 4 files changed, 26 insertions(+), 11 deletions(-) diff --git a/kubespawner/spawner.py b/kubespawner/spawner.py index 878941ef..5dc57719 100644 --- a/kubespawner/spawner.py +++ b/kubespawner/spawner.py @@ -1546,7 +1546,8 @@ def _validate_image_pull_secrets(self, proposal): - `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 + with the value of the free form input, using `{value}` for the value to be substituted with + the user POSTed value in the `other-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 @@ -3071,7 +3072,7 @@ async def _load_profile(self, slug, selected_profile_user_options): for option_name, option in profile.get('profile_options').items(): if option_name not in selected_profile_user_options: - # other_choice in Enabled: + # other_choice is enabled: if option.get('other_choice', {}).get('enabled', False): if ( f'{option_name}--other-choice' @@ -3091,8 +3092,9 @@ async def _load_profile(self, slug, selected_profile_user_options): other_choice_validation_regex = profile.get( 'profile_options' )[option_name]['other_choice']['validation_regex'] - regex = re.compile(other_choice_validation_regex) - if not regex.match(other_choice): + if not re.match( + other_choice_validation_regex, other_choice + ): raise ValueError( f'Value of {option_name}--other-choice does not match validation regex.' ) diff --git a/kubespawner/templates/form.html b/kubespawner/templates/form.html index 88783cdf..987e5ade 100644 --- a/kubespawner/templates/form.html +++ b/kubespawner/templates/form.html @@ -32,7 +32,7 @@

{{ profile.display_name }}

{%- if option['other_choice'] and option['other_choice']['enabled'] %} -
+ - {%- if option['other_choice'] and option['other_choice']['enabled'] %} + {%- if option['unlisted_choice'] and option['unlisted_choice']['enabled'] %} {%- endif %} @@ -65,10 +65,10 @@

{{ profile.display_name }}

const selectionValue = $(this).val(); const $otherInputContainer = $(this).parents('.js-options-container').find('.js-other-input-container'); const $otherInput = $(this).parents('.js-options-container').find('.js-other-input'); - if (selectionValue === 'other-choice') { + if (selectionValue === 'unlisted-choice') { - // if the Select box is selected with a value that is not 'other-choice', - // we remove the "name" attribute for the other-choice input + // if the Select box is selected with a value that is not 'unlisted-choice', + // we remove the "name" attribute for the unlisted-choice input // and vice-versa, so that we only submit valid form values // and don't leave the complexity of figuring out which value // to use to the back-end. diff --git a/tests/test_spawner.py b/tests/test_spawner.py index b404c61e..a35876bb 100644 --- a/tests/test_spawner.py +++ b/tests/test_spawner.py @@ -887,7 +887,7 @@ async def test_get_pod_manifest_tolerates_mixed_input(): 'profile_options': { 'image': { 'display_name': 'Image', - 'other_choice': { + 'unlisted_choice': { 'enabled': True, 'display_name': 'Image Location', 'validation_regex': '^pangeo/.*$', @@ -916,7 +916,7 @@ async def test_get_pod_manifest_tolerates_mixed_input(): 'profile_options': { 'image': { 'display_name': 'Image', - 'other_choice': { + 'unlisted_choice': { 'enabled': True, 'display_name': 'Image Location', 'kubespawner_override': {'image': '{value}'}, @@ -982,9 +982,9 @@ async def test_user_options_set_from_form_choices(): assert getattr(spawner, 'image') == 'pangeo/pytorch-notebook:master' -async def test_user_options_set_from_form_other_choice(): +async def test_user_options_set_from_form_unlisted_choice(): """ - Test that when user sends an arbitrary text input in the `other_choice` field, + Test that when user sends an arbitrary text input in the `unlisted_choice` field, it is process correctly and the correct attribute over-ridden on the spawner. """ spawner = KubeSpawner(_mock=True) @@ -993,11 +993,11 @@ async def test_user_options_set_from_form_other_choice(): spawner.user_options = spawner.options_from_form( { 'profile': [_test_profiles[3]['slug']], - 'profile-option-test-choices-image--other-choice': ['pangeo/test:latest'], + 'profile-option-test-choices-image--unlisted-choice': ['pangeo/test:latest'], } ) assert spawner.user_options == { - 'image--other-choice': 'pangeo/test:latest', + 'image--unlisted-choice': 'pangeo/test:latest', 'profile': _test_profiles[3]['slug'], } assert spawner.cpu_limit is None @@ -1007,8 +1007,8 @@ async def test_user_options_set_from_form_other_choice(): async def test_user_options_set_from_form_invalid_regex(): """ - Test that if the user input for the `other-choice` field does not match the regex - specified in the `validation_match_regex` option for the `other_choice`, a ValueError is raised. + Test that if the user input for the `unlisted-choice` field does not match the regex + specified in the `validation_match_regex` option for the `unlisted_choice`, a ValueError is raised. """ spawner = KubeSpawner(_mock=True) spawner.profile_list = _test_profiles @@ -1016,11 +1016,11 @@ async def test_user_options_set_from_form_invalid_regex(): spawner.user_options = spawner.options_from_form( { 'profile': [_test_profiles[3]['slug']], - 'profile-option-test-choices-image--other-choice': ['invalid/foo:latest'], + 'profile-option-test-choices-image--unlisted-choice': ['invalid/foo:latest'], } ) assert spawner.user_options == { - 'image--other-choice': 'invalid/foo:latest', + 'image--unlisted-choice': 'invalid/foo:latest', 'profile': _test_profiles[3]['slug'], } assert spawner.cpu_limit is None @@ -1031,7 +1031,7 @@ async def test_user_options_set_from_form_invalid_regex(): async def test_user_options_set_from_form_no_regex(): """ - Test that if the `other_choice` object in the profile_options does not contain + Test that if the `unlisted_choice` object in the profile_options does not contain a `validation_regex` key, no validation is done and the input is correctly processed - i.e. validation_regex is optional. """ spawner = KubeSpawner(_mock=True) @@ -1041,11 +1041,11 @@ async def test_user_options_set_from_form_no_regex(): spawner.user_options = spawner.options_from_form( { 'profile': [_test_profiles[4]['slug']], - 'profile-option-no-regex-image--other-choice': ['invalid/foo:latest'], + 'profile-option-no-regex-image--unlisted-choice': ['invalid/foo:latest'], } ) assert spawner.user_options == { - 'image--other-choice': 'invalid/foo:latest', + 'image--unlisted-choice': 'invalid/foo:latest', 'profile': _test_profiles[4]['slug'], } assert spawner.cpu_limit is None From 116ad0c67cd6780ec42b6e486ebfa2255e45b6d9 Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Tue, 11 Jul 2023 20:43:32 -0700 Subject: [PATCH 20/38] Move options validation code into a function --- kubespawner/spawner.py | 84 ++++++++++++++++++++++-------------------- 1 file changed, 44 insertions(+), 40 deletions(-) diff --git a/kubespawner/spawner.py b/kubespawner/spawner.py index b04034c7..df6ed344 100644 --- a/kubespawner/spawner.py +++ b/kubespawner/spawner.py @@ -3012,6 +3012,49 @@ def _options_from_form(self, formdata): return options + 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' + ) async def _load_profile(self, slug, selected_profile_user_options): """Load a profile by name @@ -3062,46 +3105,7 @@ async def _load_profile(self, slug, selected_profile_user_options): 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 option_name, option in profile.get('profile_options').items(): - unlisted_choice_form_key = f'{option_name}--unlisted-choice' - if option_name not in selected_profile_user_options: - # unlisted_choice is enabled: - if option.get('unlisted_choice', {}).get('enabled', False): - if unlisted_choice_form_key not in selected_profile_user_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_profile_user_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' - ) - + 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' From 68a71f939e73325469b5e664a43b9b0b440b7b69 Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Tue, 11 Jul 2023 20:45:55 -0700 Subject: [PATCH 21/38] Format with black --- kubespawner/spawner.py | 13 ++++++++----- tests/test_spawner.py | 8 ++++++-- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/kubespawner/spawner.py b/kubespawner/spawner.py index df6ed344..2d378bcd 100644 --- a/kubespawner/spawner.py +++ b/kubespawner/spawner.py @@ -3033,9 +3033,7 @@ def _validate_posted_profile_options(self, profile, 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 - ] + unlisted_choice = selected_options[unlisted_choice_form_key] # Validate value of 'unlisted_choice' against validation regex if profile.get('profile_options')[option_name][ @@ -3055,6 +3053,7 @@ def _validate_posted_profile_options(self, profile, selected_options): raise ValueError( f'Expected option {option_name} for profile {profile["slug"]}, not found in posted form' ) + async def _load_profile(self, slug, selected_profile_user_options): """Load a profile by name @@ -3105,7 +3104,9 @@ async def _load_profile(self, slug, selected_profile_user_options): setattr(self, k, v) if profile.get('profile_options'): - self._validate_posted_profile_options(profile, selected_profile_user_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' @@ -3129,7 +3130,9 @@ async def _load_profile(self, slug, selected_profile_user_options): ] for k, v in chosen_option_overrides.items(): chosen_option_overrides[k] = v.format( - value=selected_profile_user_options[unlisted_choice_form_key] + value=selected_profile_user_options[ + unlisted_choice_form_key + ] ) else: chosen_option_overrides = option['choices'][chosen_option][ diff --git a/tests/test_spawner.py b/tests/test_spawner.py index a35876bb..9859f206 100644 --- a/tests/test_spawner.py +++ b/tests/test_spawner.py @@ -993,7 +993,9 @@ async def test_user_options_set_from_form_unlisted_choice(): spawner.user_options = spawner.options_from_form( { 'profile': [_test_profiles[3]['slug']], - 'profile-option-test-choices-image--unlisted-choice': ['pangeo/test:latest'], + 'profile-option-test-choices-image--unlisted-choice': [ + 'pangeo/test:latest' + ], } ) assert spawner.user_options == { @@ -1016,7 +1018,9 @@ async def test_user_options_set_from_form_invalid_regex(): spawner.user_options = spawner.options_from_form( { 'profile': [_test_profiles[3]['slug']], - 'profile-option-test-choices-image--unlisted-choice': ['invalid/foo:latest'], + 'profile-option-test-choices-image--unlisted-choice': [ + 'invalid/foo:latest' + ], } ) assert spawner.user_options == { From 07fc3a93c83ba85e1049c2f03a77d0f2175b380e Mon Sep 17 00:00:00 2001 From: Yuvi Panda Date: Wed, 12 Jul 2023 12:46:44 -0700 Subject: [PATCH 22/38] Fix typo Co-authored-by: Georgiana --- kubespawner/spawner.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kubespawner/spawner.py b/kubespawner/spawner.py index 2d378bcd..0dc26582 100644 --- a/kubespawner/spawner.py +++ b/kubespawner/spawner.py @@ -1539,7 +1539,7 @@ def _validate_image_pull_secrets(self, proposal): - `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 - select "Other" as a choice: + 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/.*$ From 6862aa0c94c25208cc4419859272ff235d9e767e Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Tue, 18 Jul 2023 20:39:12 -0700 Subject: [PATCH 23/38] Explicitly populate defaults of profile_list Co-authored-by: Georgiana --- kubespawner/spawner.py | 32 ++++++++-- tests/test_profile.py | 140 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 167 insertions(+), 5 deletions(-) create mode 100644 tests/test_profile.py diff --git a/kubespawner/spawner.py b/kubespawner/spawner.py index 0dc26582..1741346b 100644 --- a/kubespawner/spawner.py +++ b/kubespawner/spawner.py @@ -2929,7 +2929,7 @@ def _env_keep_default(self): _profile_list = None def _render_options_form(self, profile_list): - self._profile_list = self._init_profile_list(profile_list) + self._profile_list = self._populate_profile_list_defaults(profile_list) loader = ChoiceLoader( [ @@ -2949,7 +2949,7 @@ def _render_options_form(self, 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) + profile_list = self._populate_profile_list_defaults(profile_list) return self._render_options_form(profile_list) @default('options_form') @@ -3163,12 +3163,34 @@ async def _load_profile(self, slug, selected_profile_user_options): # 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. + + The profile_list passed in is mutated and returned. + """ 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 + 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 + return profile_list async def load_user_options(self): @@ -3188,7 +3210,7 @@ async def load_user_options(self): else: profile_list = self.profile_list - self._profile_list = self._init_profile_list(profile_list) + self._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) diff --git a/tests/test_profile.py b/tests/test_profile.py new file mode 100644 index 00000000..b8495ac6 --- /dev/null +++ b/tests/test_profile.py @@ -0,0 +1,140 @@ +from kubespawner import KubeSpawner +import pytest + + +@pytest.mark.parametrize( + "unfilled_profile_list,filled_profile_list", + [ + ( + [ + { + 'display_name': 'Something without a slug', + 'kubespawner_override': {}, + }, + { + 'display_name': 'Something with a slug', + 'slug': 'sluggity-slug', + 'kubespawner_override': {}, + }, + ], + [ + { + 'display_name': 'Something without a slug', + 'slug': 'something-without-a-slug', + 'kubespawner_override': {}, + }, + { + 'display_name': 'Something with a slug', + 'slug': 'sluggity-slug', + 'kubespawner_override': {}, + }, + ], + ), + ( + [ + { + 'display_name': 'Something without choices', + 'kubespawner_override': {}, + }, + { + 'display_name': 'Something with choices', + 'kubespawner_override': {}, + 'profile_options': { + 'no-defaults': { + 'display_name': 'Some choice without a default set', + 'choices': { + 'option-1': { + 'display_name': 'Option 1', + 'kubespawner_override': {} + }, + 'option-2': { + 'display_name': 'Option 2', + 'kubespawner_override': {} + } + } + }, + 'only-unlisted': { + 'display_name': 'Some option without any choices set', + 'unlisted_choice': { + 'enabled': True + } + }, + 'explicit-defaults': { + 'display_name': 'Some choice with a default set', + 'choices': { + 'option-1': { + 'display_name': 'Option 1', + 'kubespawner_override': {} + }, + 'option-2': { + 'display_name': 'Option 2', + 'default': True, + 'kubespawner_override': {} + } + } + } + } + }, + ], + [ + { + 'display_name': 'Something without choices', + 'slug': 'something-without-choices', + 'kubespawner_override': {}, + }, + { + 'display_name': 'Something with choices', + 'slug': 'something-with-choices', + 'kubespawner_override': {}, + 'profile_options': { + 'no-defaults': { + 'display_name': 'Some choice without a default set', + 'choices': { + 'option-1': { + 'display_name': 'Option 1', + 'default': True, + 'kubespawner_override': {} + }, + 'option-2': { + 'display_name': 'Option 2', + 'kubespawner_override': {} + } + } + }, + 'only-unlisted': { + 'display_name': 'Some option without any choices set', + 'unlisted_choice': { + 'enabled': True + } + }, + 'explicit-defaults': { + 'display_name': 'Some choice with a default set', + 'choices': { + 'option-1': { + 'display_name': 'Option 1', + 'kubespawner_override': {} + }, + 'option-2': { + 'display_name': 'Option 2', + 'default': True, + 'kubespawner_override': {} + } + } + } + } + }, + ], + ), + ], +) +async def test_profile_missing_defaults_populated( + unfilled_profile_list, filled_profile_list +): + """ + Tests that missing profileList values are populated + """ + spawner = KubeSpawner(_mock=True) + assert ( + spawner._populate_profile_list_defaults(unfilled_profile_list) + == filled_profile_list + ) From a47f67e2224541d90b2c62c20de05cf4fa348b87 Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Tue, 18 Jul 2023 23:00:20 -0700 Subject: [PATCH 24/38] Move code to get a profile from a slug into own function Co-authored-by: Georgiana --- kubespawner/spawner.py | 52 +++++++++++++-------- tests/test_profile.py | 100 ++++++++++++++++++++++++++++++----------- 2 files changed, 107 insertions(+), 45 deletions(-) diff --git a/kubespawner/spawner.py b/kubespawner/spawner.py index 1741346b..b172ee83 100644 --- a/kubespawner/spawner.py +++ b/kubespawner/spawner.py @@ -3054,31 +3054,38 @@ def _validate_posted_profile_options(self, profile, selected_options): f'Expected option {option_name} for profile {profile["slug"]}, not found in posted form' ) + def _get_profile(self, slug: str): + """ + Get the configured profile for given profile slug + + Raises an error if no profile exists for the given slug. + + If slug is empty string, return the default profile + """ + profile_list = self._populate_profile_list_defaults(self.profile_list) + if slug != "": + for profile in profile_list: + if profile['slug'] == slug: + return profile + + # A slug is specified, but not found + # name specified, but not found + raise ValueError( + "No such profile: %s. Options include: %s" + % (slug, ', '.join(p['slug'] for p in profile_list)) + ) + else: + # 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')) + async def _load_profile(self, slug, selected_profile_user_options): """Load a profile by name Called by load_user_options """ - # 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 - - if profile['slug'] == slug: - break - 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 + profile = self._get_profile(slug) self.log.debug( "Applying KubeSpawner override for profile '%s'", profile['display_name'] @@ -3173,6 +3180,8 @@ def _populate_profile_list_defaults(self, profile_list: list): 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. """ @@ -3191,6 +3200,11 @@ def _populate_profile_list_defaults(self, profile_list: list): 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): diff --git a/tests/test_profile.py b/tests/test_profile.py index b8495ac6..ad03c793 100644 --- a/tests/test_profile.py +++ b/tests/test_profile.py @@ -21,6 +21,7 @@ { 'display_name': 'Something without a slug', 'slug': 'something-without-a-slug', + 'default': True, 'kubespawner_override': {}, }, { @@ -39,41 +40,40 @@ { 'display_name': 'Something with choices', 'kubespawner_override': {}, + 'default': True, 'profile_options': { 'no-defaults': { 'display_name': 'Some choice without a default set', 'choices': { 'option-1': { 'display_name': 'Option 1', - 'kubespawner_override': {} + 'kubespawner_override': {}, }, 'option-2': { 'display_name': 'Option 2', - 'kubespawner_override': {} - } - } + 'kubespawner_override': {}, + }, + }, }, 'only-unlisted': { 'display_name': 'Some option without any choices set', - 'unlisted_choice': { - 'enabled': True - } + 'unlisted_choice': {'enabled': True}, }, 'explicit-defaults': { 'display_name': 'Some choice with a default set', 'choices': { 'option-1': { 'display_name': 'Option 1', - 'kubespawner_override': {} + 'kubespawner_override': {}, }, 'option-2': { 'display_name': 'Option 2', 'default': True, - 'kubespawner_override': {} - } - } - } - } + 'kubespawner_override': {}, + }, + }, + }, + }, }, ], [ @@ -85,6 +85,7 @@ { 'display_name': 'Something with choices', 'slug': 'something-with-choices', + 'default': True, 'kubespawner_override': {}, 'profile_options': { 'no-defaults': { @@ -93,35 +94,33 @@ 'option-1': { 'display_name': 'Option 1', 'default': True, - 'kubespawner_override': {} + 'kubespawner_override': {}, }, 'option-2': { 'display_name': 'Option 2', - 'kubespawner_override': {} - } - } + 'kubespawner_override': {}, + }, + }, }, 'only-unlisted': { 'display_name': 'Some option without any choices set', - 'unlisted_choice': { - 'enabled': True - } + 'unlisted_choice': {'enabled': True}, }, 'explicit-defaults': { 'display_name': 'Some choice with a default set', 'choices': { 'option-1': { 'display_name': 'Option 1', - 'kubespawner_override': {} + 'kubespawner_override': {}, }, 'option-2': { 'display_name': 'Option 2', 'default': True, - 'kubespawner_override': {} - } - } - } - } + 'kubespawner_override': {}, + }, + }, + }, + }, }, ], ), @@ -138,3 +137,52 @@ async def test_profile_missing_defaults_populated( spawner._populate_profile_list_defaults(unfilled_profile_list) == filled_profile_list ) + + +@pytest.mark.parametrize( + "profile_list,slug,selected_profile,exception", + [ + ( + [ + { + 'display_name': 'profile 1', + 'kubespawner_override': {}, + }, + { + 'display_name': 'profile 2', + 'kubespawner_override': {}, + }, + ], + 'profile-2', + { + 'display_name': 'profile 2', + 'slug': 'profile-2', + 'kubespawner_override': {}, + }, + ), + ( + [ + { + 'display_name': 'profile 1', + 'kubespawner_override': {}, + }, + { + 'display_name': 'profile 2', + 'default': True, + 'kubespawner_override': {}, + }, + ], + '', + { + 'display_name': 'profile 2', + 'slug': 'profile-2', + 'default': True, + 'kubespawner_override': {}, + }, + ), + ], +) +async def test_find_slug(profile_list, slug, selected_profile): + spawner = KubeSpawner(_mock=True) + spawner.profile_list = profile_list + assert spawner._get_profile(slug) == selected_profile From 36db652125c38cdb77398c9e6520d3d17dde5de7 Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Tue, 18 Jul 2023 23:12:40 -0700 Subject: [PATCH 25/38] Stop using self._profile_list This was slightly hidden state, making reasoning about what was happening to profiles a little more difficult. Instead, we just call the function to set defaults as necessary wherever it is used. --- kubespawner/spawner.py | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/kubespawner/spawner.py b/kubespawner/spawner.py index b172ee83..fc207d61 100644 --- a/kubespawner/spawner.py +++ b/kubespawner/spawner.py @@ -2926,10 +2926,9 @@ 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._populate_profile_list_defaults(profile_list) + profile_list = self._populate_profile_list_defaults(profile_list) loader = ChoiceLoader( [ @@ -2945,7 +2944,7 @@ 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)) @@ -3218,13 +3217,12 @@ 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._populate_profile_list_defaults(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) @@ -3232,7 +3230,7 @@ async def load_user_options(self): # Remove the 'profile' key so we are left with only selected profile options del selected_profile_user_options['profile'] - if self._profile_list: + if profile_list: await self._load_profile(selected_profile, selected_profile_user_options) elif selected_profile: self.log.warning( From 59507de7d6c228879854617338a131cfca434c7b Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Tue, 18 Jul 2023 23:13:49 -0700 Subject: [PATCH 26/38] Document why the options_form default is the way it is --- kubespawner/spawner.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/kubespawner/spawner.py b/kubespawner/spawner.py index fc207d61..74d0947a 100644 --- a/kubespawner/spawner.py +++ b/kubespawner/spawner.py @@ -2963,8 +2963,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') From b1dba2785befacc55195bfcdd5a09c2d5a2117e0 Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Tue, 18 Jul 2023 23:14:00 -0700 Subject: [PATCH 27/38] Remove extra call to _populate_profile_list_defaults --- kubespawner/spawner.py | 1 - 1 file changed, 1 deletion(-) diff --git a/kubespawner/spawner.py b/kubespawner/spawner.py index 74d0947a..b22a01ca 100644 --- a/kubespawner/spawner.py +++ b/kubespawner/spawner.py @@ -2948,7 +2948,6 @@ def _render_options_form(self, profile_list): async def _render_options_form_dynamically(self, current_spawner): profile_list = await maybe_future(self.profile_list(current_spawner)) - profile_list = self._populate_profile_list_defaults(profile_list) return self._render_options_form(profile_list) @default('options_form') From 1763083927a353477479b3a8e5842eada7c1b8f5 Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Tue, 18 Jul 2023 23:14:43 -0700 Subject: [PATCH 28/38] Clarify that the profile_list function is idempotent --- kubespawner/spawner.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/kubespawner/spawner.py b/kubespawner/spawner.py index b22a01ca..97ffe808 100644 --- a/kubespawner/spawner.py +++ b/kubespawner/spawner.py @@ -3185,6 +3185,9 @@ def _populate_profile_list_defaults(self, profile_list: list): 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. """ for profile in profile_list: # generate missing slug fields from display_name From 3534920142acff548a69ec2596d5592971621b11 Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Tue, 18 Jul 2023 23:32:29 -0700 Subject: [PATCH 29/38] Split code that applies overrides to its own function Co-authored-by: Georgiana --- kubespawner/spawner.py | 56 +++++++++++++++++++----------------------- 1 file changed, 25 insertions(+), 31 deletions(-) diff --git a/kubespawner/spawner.py b/kubespawner/spawner.py index 97ffe808..26976cbc 100644 --- a/kubespawner/spawner.py +++ b/kubespawner/spawner.py @@ -3080,20 +3080,17 @@ def _get_profile(self, slug: str): # default is guaranteed to be set in at least one profile return next(p for p in profile_list if p.get('default')) - async def _load_profile(self, slug, selected_profile_user_options): - """Load a profile by name - - Called by load_user_options + async def _apply_overrides(self, spawner_override: dict): """ - # find the profile - profile = self._get_profile(slug) - - self.log.debug( - "Applying KubeSpawner override for profile '%s'", profile['display_name'] - ) + 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( @@ -3111,6 +3108,21 @@ async def _load_profile(self, slug, selected_profile_user_options): else: setattr(self, k, v) + + async def _load_profile(self, slug, selected_profile_user_options): + """Load a profile by name + + Called by load_user_options + """ + # find the profile + profile = self._get_profile(slug) + + self.log.debug( + "Applying KubeSpawner override for profile '%s'", profile['display_name'] + ) + + self._apply_overrides(profile.get('kubespawner_override', {})) + if profile.get('profile_options'): self._validate_posted_profile_options( profile, selected_profile_user_options @@ -3147,25 +3159,7 @@ async def _load_profile(self, slug, selected_profile_user_options): '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}' - ) - - # 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) + self._apply_overrides(chosen_option_overrides) # set of recognised user option keys # used for warning about ignoring unrecognised options From 6eb6e8ab8948b392503294fb3742e25334fc4942 Mon Sep 17 00:00:00 2001 From: Yuvi Panda Date: Wed, 19 Jul 2023 10:25:45 -0700 Subject: [PATCH 30/38] Remove extra comment Co-authored-by: Georgiana --- kubespawner/spawner.py | 1 - 1 file changed, 1 deletion(-) diff --git a/kubespawner/spawner.py b/kubespawner/spawner.py index 26976cbc..8b65a0b6 100644 --- a/kubespawner/spawner.py +++ b/kubespawner/spawner.py @@ -3070,7 +3070,6 @@ def _get_profile(self, slug: str): return profile # A slug is specified, but not found - # name specified, but not found raise ValueError( "No such profile: %s. Options include: %s" % (slug, ', '.join(p['slug'] for p in profile_list)) From 96067e8903c9a60c394aec4bcc044c257fc61f79 Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Wed, 19 Jul 2023 10:27:16 -0700 Subject: [PATCH 31/38] Actually write comment explaining default choice selection Co-authored-by: Georgiana --- kubespawner/spawner.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/kubespawner/spawner.py b/kubespawner/spawner.py index 8b65a0b6..10475ac0 100644 --- a/kubespawner/spawner.py +++ b/kubespawner/spawner.py @@ -3187,7 +3187,8 @@ def _populate_profile_list_defaults(self, profile_list: list): if 'slug' not in profile: profile['slug'] = slugify(profile['display_name']) - # If profile_options are present with choices, but no + # 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 From cea4107304b759902b1040e1c7d60b1b5546f56f Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Wed, 19 Jul 2023 10:30:55 -0700 Subject: [PATCH 32/38] Reduce extra complexity in choosing default choice --- kubespawner/spawner.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/kubespawner/spawner.py b/kubespawner/spawner.py index 10475ac0..05778b62 100644 --- a/kubespawner/spawner.py +++ b/kubespawner/spawner.py @@ -3130,14 +3130,12 @@ async def _load_profile(self, slug, selected_profile_user_options): 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 # Handle override for unlisted_choice free text specified by user if ( From dcbd1527684f63263189cfed452cc20ca3bb63a0 Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Wed, 19 Jul 2023 10:38:03 -0700 Subject: [PATCH 33/38] Fix find profile by slug tests --- tests/test_profile.py | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/tests/test_profile.py b/tests/test_profile.py index ad03c793..20c1ae36 100644 --- a/tests/test_profile.py +++ b/tests/test_profile.py @@ -140,7 +140,7 @@ async def test_profile_missing_defaults_populated( @pytest.mark.parametrize( - "profile_list,slug,selected_profile,exception", + "profile_list,slug,selected_profile", [ ( [ @@ -183,6 +183,27 @@ async def test_profile_missing_defaults_populated( ], ) async def test_find_slug(profile_list, slug, selected_profile): + """ + Test that we can find the profile we expect given slugs + """ spawner = KubeSpawner(_mock=True) spawner.profile_list = profile_list assert spawner._get_profile(slug) == selected_profile + +async def test_find_slug_exception(): + """ + Test that looking for a slug that doesn't exist gives us an exception + """ + spawner = KubeSpawner(_mock=True) + spawner.profile_list = [ + { + 'display_name': 'profile 1', + 'kubespawner_override': {}, + }, + { + 'display_name': 'profile 2', + 'kubespawner_override': {}, + }, + ] + with pytest.raises(ValueError): + spawner._get_profile('does-not-exist') From 783f2407f6bc0d7c5252782f52ee0115bad8e39f Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Wed, 19 Jul 2023 10:38:49 -0700 Subject: [PATCH 34/38] Run pre-commit --- kubespawner/spawner.py | 8 +++----- tests/test_profile.py | 22 ++++++++++++---------- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/kubespawner/spawner.py b/kubespawner/spawner.py index 05778b62..9fc6c05f 100644 --- a/kubespawner/spawner.py +++ b/kubespawner/spawner.py @@ -2926,7 +2926,6 @@ async def stop(self, now=False): def _env_keep_default(self): return [] - def _render_options_form(self, profile_list): profile_list = self._populate_profile_list_defaults(profile_list) @@ -3107,7 +3106,6 @@ async def _apply_overrides(self, spawner_override: dict): else: setattr(self, k, v) - async def _load_profile(self, slug, selected_profile_user_options): """Load a profile by name @@ -3162,7 +3160,6 @@ async def _load_profile(self, slug, selected_profile_user_options): # used for warning about ignoring unrecognised options _user_option_keys = {'profile'} - def _populate_profile_list_defaults(self, profile_list: list): """ Return a fully realized profile_list @@ -3191,12 +3188,13 @@ def _populate_profile_list_defaults(self, profile_list: list): 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()): + 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 diff --git a/tests/test_profile.py b/tests/test_profile.py index 20c1ae36..3f5ee631 100644 --- a/tests/test_profile.py +++ b/tests/test_profile.py @@ -1,6 +1,7 @@ -from kubespawner import KubeSpawner import pytest +from kubespawner import KubeSpawner + @pytest.mark.parametrize( "unfilled_profile_list,filled_profile_list", @@ -190,20 +191,21 @@ async def test_find_slug(profile_list, slug, selected_profile): spawner.profile_list = profile_list assert spawner._get_profile(slug) == selected_profile + async def test_find_slug_exception(): """ Test that looking for a slug that doesn't exist gives us an exception """ spawner = KubeSpawner(_mock=True) spawner.profile_list = [ - { - 'display_name': 'profile 1', - 'kubespawner_override': {}, - }, - { - 'display_name': 'profile 2', - 'kubespawner_override': {}, - }, - ] + { + 'display_name': 'profile 1', + 'kubespawner_override': {}, + }, + { + 'display_name': 'profile 2', + 'kubespawner_override': {}, + }, + ] with pytest.raises(ValueError): spawner._get_profile('does-not-exist') From a88f875b7d760056f50dacf9208e9cea24bd7546 Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Wed, 19 Jul 2023 10:45:52 -0700 Subject: [PATCH 35/38] Add missing await to async function call --- kubespawner/spawner.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/kubespawner/spawner.py b/kubespawner/spawner.py index 9fc6c05f..5ce77b16 100644 --- a/kubespawner/spawner.py +++ b/kubespawner/spawner.py @@ -3118,7 +3118,7 @@ async def _load_profile(self, slug, selected_profile_user_options): "Applying KubeSpawner override for profile '%s'", profile['display_name'] ) - self._apply_overrides(profile.get('kubespawner_override', {})) + await self._apply_overrides(profile.get('kubespawner_override', {})) if profile.get('profile_options'): self._validate_posted_profile_options( @@ -3154,7 +3154,7 @@ async def _load_profile(self, slug, selected_profile_user_options): 'kubespawner_override' ] - self._apply_overrides(chosen_option_overrides) + await self._apply_overrides(chosen_option_overrides) # set of recognised user option keys # used for warning about ignoring unrecognised options From ff4216c40322e7d05df42c4f312777fc0cb13800 Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Wed, 19 Jul 2023 10:54:30 -0700 Subject: [PATCH 36/38] Handle empty profile_list --- kubespawner/spawner.py | 4 ++++ tests/test_profile.py | 1 + 2 files changed, 5 insertions(+) diff --git a/kubespawner/spawner.py b/kubespawner/spawner.py index 5ce77b16..20d9c8f2 100644 --- a/kubespawner/spawner.py +++ b/kubespawner/spawner.py @@ -3177,6 +3177,10 @@ def _populate_profile_list_defaults(self, profile_list: list): 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: diff --git a/tests/test_profile.py b/tests/test_profile.py index 3f5ee631..b8dc40f5 100644 --- a/tests/test_profile.py +++ b/tests/test_profile.py @@ -125,6 +125,7 @@ }, ], ), + ([], []), ], ) async def test_profile_missing_defaults_populated( From e8e8ceb40e1613507a7dcb9b8e4c909ceed94326 Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Wed, 19 Jul 2023 11:05:02 -0700 Subject: [PATCH 37/38] Handle profile_list being a function in a single place --- kubespawner/spawner.py | 13 +++++++------ tests/test_profile.py | 9 +++++---- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/kubespawner/spawner.py b/kubespawner/spawner.py index 20d9c8f2..16fa7e5a 100644 --- a/kubespawner/spawner.py +++ b/kubespawner/spawner.py @@ -3054,15 +3054,15 @@ def _validate_posted_profile_options(self, profile, selected_options): f'Expected option {option_name} for profile {profile["slug"]}, not found in posted form' ) - def _get_profile(self, slug: str): + def _get_profile(self, slug: str, profile_list: list): """ Get the configured profile for given profile slug Raises an error if no profile exists for the given slug. If slug is empty string, return the default profile + profile_list should already have all its defaults set. """ - profile_list = self._populate_profile_list_defaults(self.profile_list) if slug != "": for profile in profile_list: if profile['slug'] == slug: @@ -3106,13 +3106,12 @@ async def _apply_overrides(self, spawner_override: dict): else: setattr(self, k, v) - async def _load_profile(self, slug, selected_profile_user_options): + async def _load_profile(self, slug, profile_list, selected_profile_user_options): """Load a profile by name Called by load_user_options """ - # find the profile - profile = self._get_profile(slug) + profile = self._get_profile(slug, profile_list) self.log.debug( "Applying KubeSpawner override for profile '%s'", profile['display_name'] @@ -3230,7 +3229,9 @@ async def load_user_options(self): del selected_profile_user_options['profile'] if profile_list: - await self._load_profile(selected_profile, selected_profile_user_options) + 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 diff --git a/tests/test_profile.py b/tests/test_profile.py index b8dc40f5..a5ed08a9 100644 --- a/tests/test_profile.py +++ b/tests/test_profile.py @@ -189,8 +189,8 @@ async def test_find_slug(profile_list, slug, selected_profile): Test that we can find the profile we expect given slugs """ spawner = KubeSpawner(_mock=True) - spawner.profile_list = profile_list - assert spawner._get_profile(slug) == selected_profile + profile_list = spawner._populate_profile_list_defaults(profile_list) + assert spawner._get_profile(slug, profile_list) == selected_profile async def test_find_slug_exception(): @@ -198,7 +198,7 @@ async def test_find_slug_exception(): Test that looking for a slug that doesn't exist gives us an exception """ spawner = KubeSpawner(_mock=True) - spawner.profile_list = [ + profile_list = [ { 'display_name': 'profile 1', 'kubespawner_override': {}, @@ -208,5 +208,6 @@ async def test_find_slug_exception(): 'kubespawner_override': {}, }, ] + profile_list = spawner._populate_profile_list_defaults(profile_list) with pytest.raises(ValueError): - spawner._get_profile('does-not-exist') + spawner._get_profile('does-not-exist', profile_list) From 243a5d7fbafd1e8e32cd6200dc589d65d7384a6c Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Wed, 19 Jul 2023 11:13:31 -0700 Subject: [PATCH 38/38] Handle case of slug being None --- kubespawner/spawner.py | 6 +++--- tests/test_profile.py | 20 ++++++++++++++++++++ 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/kubespawner/spawner.py b/kubespawner/spawner.py index 16fa7e5a..d3b00f51 100644 --- a/kubespawner/spawner.py +++ b/kubespawner/spawner.py @@ -3054,16 +3054,16 @@ def _validate_posted_profile_options(self, profile, selected_options): f'Expected option {option_name} for profile {profile["slug"]}, not found in posted form' ) - def _get_profile(self, slug: str, profile_list: list): + def _get_profile(self, slug: Optional[str], profile_list: list): """ Get the configured profile for given profile slug Raises an error if no profile exists for the given slug. - If slug is empty string, return the default profile + If slug is empty string or None, return the default profile profile_list should already have all its defaults set. """ - if slug != "": + if slug: for profile in profile_list: if profile['slug'] == slug: return profile diff --git a/tests/test_profile.py b/tests/test_profile.py index a5ed08a9..2de9a1b8 100644 --- a/tests/test_profile.py +++ b/tests/test_profile.py @@ -162,6 +162,26 @@ async def test_profile_missing_defaults_populated( 'kubespawner_override': {}, }, ), + ( + [ + { + 'display_name': 'profile 1', + 'kubespawner_override': {}, + }, + { + 'display_name': 'profile 2', + 'default': True, + 'kubespawner_override': {}, + }, + ], + None, + { + 'display_name': 'profile 2', + 'slug': 'profile-2', + 'default': True, + 'kubespawner_override': {}, + }, + ), ( [ {