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 list based config volumes and volume_mounts to be assigned dictionaries #843

Merged
merged 3 commits into from
Jul 19, 2024

Conversation

sunu
Copy link
Contributor

@sunu sunu commented Jul 15, 2024

I've made the following changes:

  • Modified the trait type for volumes and volume mounts to Union(trait_types=[List(), Dict()]) so that they can be defined as dictionaries (for easy overriding) or as lists (for backwards compatibility)
  • Updated the help messages explaining both list and dictionary input options and clarifying how dictionary inputs are processed (sorted lexicographically by keys)
  • Added tests for defining volumes and volume mounts as dictionaries and test that the output is sorted lexicographically and tests for defining volumes and volume mounts as lists to make sure these changes are backwards compatible

Defining these values as dictionaries enables easy overriding. refs jupyterhub/jupyterhub#4822 and NASA-IMPACT/veda-jupyterhub#44

sunu and others added 2 commits July 15, 2024 13:11
Continue to support these values to be defined as Lists for backwards compatibility.

When defined as Dicts, the items are sorted lexicographically by the keys and the values are used as volumes / volume mounts.

This enables easy overriding or extension. refs NASA-IMPACT/veda-jupyterhub#44 (comment) jupyterhub/jupyterhub#4822
@sunu
Copy link
Contributor Author

sunu commented Jul 15, 2024

@yuvipanda Could you please take a look at these changes and let me know if they look good? If this looks ok, I'll proceed to make similar updates for other traitlets that need to support dictionaries. Thanks!

Copy link
Collaborator

@yuvipanda yuvipanda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor ask for a docstring, but otherwise this approach looks great to me.

Thank you for working on this, @sunu!

@@ -1869,6 +1891,12 @@ def _expand_all(self, src):
else:
return src

def _sorted_dict_values(self, src):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a docstring here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@sunu
Copy link
Contributor Author

sunu commented Jul 19, 2024

Thanks for the review @yuvipanda! I have added the missing docstring. I'll open a follow up PR to make similar updates for other traitlets that need to support dictionaries.

@yuvipanda yuvipanda merged commit d1c5342 into jupyterhub:main Jul 19, 2024
9 checks passed
@yuvipanda
Copy link
Collaborator

Thanks @sunu!

sunu added a commit to sunu/kubespawner that referenced this pull request Jul 26, 2024
As a continuation of jupyterhub#843,
allow more pod properties to be defined as dictionaries
for easy overriding.
@consideRatio consideRatio changed the title Allow volumes and volume mounts to be defined as Dicts Allow volumes and volume_mounts to be defined as Dicts Sep 22, 2024
@consideRatio consideRatio changed the title Allow volumes and volume_mounts to be defined as Dicts Allow list based config volumes and volume_mounts to be assigned dictionaries Sep 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants