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

[KubeIngressProxy] Add a long docstring to KubeIngressProxy class #568

Merged
merged 4 commits into from
Mar 11, 2022

Conversation

consideRatio
Copy link
Member

@consideRatio consideRatio commented Feb 18, 2022

For a long time I've wanted to document the KubeIngressProxy class that ships with this project even though its not needed for KubeSpawner. In the past I've often been to confused to do so, but at this point I felt ready with a better comprehension of the class, its background, and role.

This isn't perfect, and relies on my memory of conversations. @yuvipanda can probably improve this documentation further if time permits.

I think it's an important improvement not only for KubeIngressProxy curious people, but especially for anyone contributing with maintenance of KubeSpawner itself that finds this code - they will need to understand the entire code base to know they are not making breaking changes.

Closes #163 for now I think.

@consideRatio
Copy link
Member Author

consideRatio commented Feb 18, 2022

Pinged @GeorgianaElena for review for your experience with JupyteHub Proxy classes and @yuvipanda for being the author of this Proxy class.


PS: I'd like to have this merged after #563 resolves to avoid handling a merge conflict that I think could happen there otherwise. So, don't merge unless it has merged first.

Copy link
Member

@GeorgianaElena GeorgianaElena left a comment

Choose a reason for hiding this comment

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

This is an awesome thing to do @consideRatio ❤️ You're great!

From what I understand, and a proxy perspective, the docstring looks very good and accurate! I've left some comments with suggestions that either add a few details, or just reword the phrases a bit, but this should be good to go!

Thank you!

kubespawner/proxy.py Outdated Show resolved Hide resolved
kubespawner/proxy.py Outdated Show resolved Hide resolved
kubespawner/proxy.py Outdated Show resolved Hide resolved
kubespawner/proxy.py Outdated Show resolved Hide resolved
@consideRatio
Copy link
Member Author

@GeorgianaElena thank you so much for your swift, uplifting, and thorough review!!! Excellent suggestions you made!

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.

Thanks a lot for working on clarifying this, @consideRatio! I've left a couple of clarifications.

https://github.com/berkeley-dsep-infra/data8xhub was the entire config for the infrastructure I'd set up for supporting >20k active users under peak loads, and this was built for that - if you're interested in looking :)

kubespawner/proxy.py Outdated Show resolved Hide resolved
kubespawner/proxy.py Show resolved Hide resolved
@consideRatio
Copy link
Member Author

@yuvipanda thanks for your input! I've rebased and added 04457e6 with the changes made since your review.

@yuvipanda yuvipanda merged commit 648ed66 into jupyterhub:main Mar 11, 2022
@yuvipanda
Copy link
Collaborator

Awesome, thanks a lot @consideRatio and @GeorgianaElena!

@consideRatio consideRatio added the KubeIngressProxy KubeIngressProxy is a separate class from KubeSpawner! label Mar 11, 2022
@consideRatio consideRatio changed the title Add a long docstring to KubeIngressProxy class [KubeIngressProxy] Add a long docstring to KubeIngressProxy class Mar 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation KubeIngressProxy KubeIngressProxy is a separate class from KubeSpawner!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

KubeIngressProxy docs
3 participants