jupyterhub / kubespawner

Kubernetes spawner for JupyterHub
https://jupyterhub-kubespawner.readthedocs.io
BSD 3-Clause "New" or "Revised" License
543 stars 304 forks source link

Rework of profile_list backend validation for readability and details #774

Closed consideRatio closed 1 year ago

consideRatio commented 1 year ago

Summary

I've failed to isolate the changes in this PR in smaller commits, but below are some highlights.

Review together with my comments

In the "Files changed" view you can see comments I've made on the changes in various sections. I think that may be the easiest way to review this PRs changes.

consideRatio commented 1 year ago

Thank you soo much for reviewing @GeorgianaElena!!!

I did a couple of passes of the changes in this PR, focusing on the changes in behavior, rather than name changes and docstrings updates, as I don't want to get into subjective change proposals given the complexity of this PR.

I appreciate that - you are most welcome to iterate on the subjective parts still though!

there was a general conclusion that would be beneficial to have all the logic related to profiles into its own module, as the spawner.py file has grown a lot and it's became harder to follow. What do you think? Do you think it would be wise to do this as part of this PR, or at least this future reaease?

I agree - I'd love to see a clearly scoped set of logic extracted from spawner.py, and I think there is good chance we clearly scope profile_list related logic somewhere else.

I think extracting code from spawner.py into another file is perfect for a dedicated PR and shouldn't go into this PR. This PR was messy and would just sully the other changed.

If you want we can hold the 6.1.0 release for such work, but I don't mind the overhead of an additional minor/patch release!