feat: 🩺 allow customized health check endpoint - #442

Hi @dee-me-tree-or-love,
May I get your approval to run the tests workflow?

Hey @Mason-Lin sure! The CI's on for the PR :) Thanks for sending in the idea! I will try to leave a proper review a little later today. 👍

Hey @Mason-Lin sure! The CI's on for the PR :) Thanks for sending in the idea! I will try to leave a proper review a little later today. 👍

Thank you so much!

Hey @Mason-Lin, I like this idea and have checked out the linked issue, thanks for adding all the motivation and related reads, this helps 👍

I do think that this can be a useful addition, so I'd be happy to add it. But I'd like to make sure that we don't create diverging ways for booting up the application code. Right now we indeed don't have any [afaik] customization possibilities of the routing of the app, and in that sense-making, a configurable endpoint for the health would be a new step.

I tried adding --health-endpoint to RunConfig, but I found that there was no config when creating the endpoint
I finally chose to use environment variables HEALTH_ENDPOINT.
(from #441)

Thinking about this, I'd like to still try to find a way to make this configuration a part of the CLI interface in order to keep it consistent with other options. But I'm not 100% sure how to tweak the existing config to make this work on top of my head either :) Right now I can think of looking into the DEFAULTS and the rest of the config.py code to see what could be done cleanly. I'd be happy to dive deeper into this tomorrow and see if I can give some suggestions if that's okay with you.

As an extra detail, thanks a lot for adding the tests 😊

Hi @dee-me-tree-or-love ,

In the beginning, I tried to copy FALLBACK_URL and add --health-endpoint to DEFAULTS and RunConfig and run_parser

Then I replaced @app.route("/health") with @app.route(config.health_endpoint)

I got an error while executing the test

Then I checked app = pypiserver.app_from_config(config)
I found that app_from_config is use __import__ to import _app first, and then mutate global variable _app.config = config

Because the bottle.route decorator already needs config.health_endpoint when import _app, it seems that the current import order cannot get CLI arguments while creating the bottle instance.
I'm not quite sure how to solve this.
I saw a function in the bottle add_route, it may be able to delay the time of adding endpoints but I am not very familiar with the bottle.

The above is the blockage I encountered while exploring the CLI interface.😅

I would greatly appreciate it if you kindly give me some suggestions.

Hey @Mason-Lin! Thanks a lot for the detailed description, this helps a lot, indeed I see the trouble 👍 I'm sorry I didn't get back to this yesterday, had some unexpected changes to my plans, but I will try to think along in the coming days and will keep you posted.

What you describe is indeed a bit of a blocking issue, so I will try to see how to work around this with minimal changes.

Alternatively, I have come across the explicit routing of the bottle library: (see Request Routing -> EXPLICIT ROUTING CONFIGURATION). This seems to allow defining the routes within a function, which could technically solve the import order issue.

# Taken from the bottle docs:
#  see https://bottlepy.org/docs/dev/routing.html#explicit-routing-configuration

def setup_routing(app):
    app.route('/new', ['GET', 'POST'], form_new)
    app.route('/edit', ['GET', 'POST'], form_edit)

app = Bottle()
setup_routing(app)

The only thing is that I am a bit wary of coupling our code to the bottle's interface even more. And if that is to be the solution, I'd like to pick this up in a separate PR to keep the changes isolated.

But feel to give it a try, maybe that makes it work with config, and I will also try to get myself some time to try it out in code too :)

Furthermore, not to make many changes to the internals of pypiserver, it could be a compromise to use the explicit routing approach, but on the client side, when configuring your pypiserver instance. A little similar to what I did with adding hooks after the app creation in a separate use case (this get_app on the client side). This could also be a way to go around this, making all the changes purely on the client side :)

In any case, I'll get back to this soon, thanks a lot for all your work! 👍

Hi @dee-me-tree-or-love ,

Thank you for the suggestions.
I'll find time next week to look into these solutions. 😀

Hi @dee-me-tree-or-love ,

Thanks again for the suggestion
Using explicit routing can solve the problem I had before 👍

The practice of get_app() mentioned to set entrypoint: gunicorn -b :8081 -w 2 'main:get_app()'
I haven't experimented with this method yet, but it seems that adding a health endpoint can be done without modifying pypiserver.
Once get the app from pypiserver
The user should be able to patch their endpoint from the client side.
Making all the changes purely on the client side.

My current usage is actually to deploy pypiserver/pypiserver:latest with docker
Then start a private PyPI server in the private network through CMD ["run", "--disable-fallback"]
I wish I could keep using entrypoint.sh and passing some more parameters to accomplish the requirement.
I hope the customized health endpoint becomes a feature out of the box.

I will now change the environment variable approach to using the CLI to set the health endpoint. 😊

notes:
build fail related to #439

Hey, @Mason-Lin I've fixed the #439 issue with #444. The CI should be back to normal if you update from master. I will get to reviewing this more properly soon, thanks for all the updates! :)

Hey @Mason-Lin, thanks a lot for all the updates on this branch! ✌️ I've had a few things that kept me busy last week, but I'll try to walk through your PR in detail this week :D

Hey @Mason-Lin, thanks a lot for all the updates on this branch! ✌️ I've had a few things that kept me busy last week, but I'll try to walk through your PR in detail this week :D

No problem, thank you 😁

Hey @Mason-Lin, thanks a lot for your work and adjusting it to the explicit routing setup! 😊 This is already looking nicer, and I think this addition could be an interesting enabler for some extra configurations for the pypiserver in the future. I added a few comments with requests to change below. Those are some alignment details to keep this new behaviour as close as possible to other features of pypiserver, plus to make sure it's modular enough for future extensions or changes. I think it's getting there! Could you take a look at the comments? :)

Special thanks for making all the test cases, these are useful and helpful to see! Then, when you finish this up, don't forget to add a note about this in the Recipes section of the README and update the help command output in the Quickstart section, this will be nice ;) 👍 🚀

Hi @dee-me-tree-or-love,

Ok no problem, thank you very much for your review.
I have a work in progress that will probably be done this week, and then I'll start working on this PR 😁

Hi @dee-me-tree-or-love ,
Thank you for your good advice and clear explanation of ideas considering project maintenance and user experience :)

Hi @dee-me-tree-or-love ,
Could you enabled the GitHub actions to run on this PR again? so I can fix them before next review. 🫡

Thanks for the correction in reStructuredText.
I usually use Markdown, I am unfamiliar with reStructuredText, and I misuse syntax ^^"

Hey @Mason-Lin, this is great work! Thanks a lot for the effort, it was a pleasure to discuss this change 👍 Sorry for my late response, last few weeks I had to manage some challenges on the personal side and was largely AFK. But now getting back to this :) I left a few final nitty comments to polish up the readme and the rest, the code looks good to go IMHO 👍

no problem, thanks for your time 👍

Hi @dee-me-tree-or-love ,
Do I need to update the branch or keep what it is?
If it needs to be updated, should I rebase it or merge it?

Thank you.

Don't worry, I've also learned some good ideas along the way, thanks for every suggestion you give.

Regarding the workflow part, because “First-time contributors need a maintainer to approve running workflows”, I have an idea but I don't know if it is suitable or not, that is to provide a dockerfile, which contains mypy, pylint, black, etc.

If I want to confirm whether the PR will be pass the workflow, I can check with docker build.

Regarding the workflow part, because “First-time contributors need a maintainer to approve running workflows”, I have an idea but I don't know if it is suitable or not, that is to provide a dockerfile, which contains mypy, pylint, black, etc.

If I want to confirm whether the PR will be pass the workflow, I can check with docker build.

Hey @Mason-Lin, I like this idea! I totally agree that some improved flow for checking will be nice :) Maybe a tool like pre-commit, drkns, or even a collection of scripts in a Makefile may be less resource-heavy but allow the same as Dockerfile for checking the code styles :) However the Dockerfile will ensure that the environment is consistent and of course, in that sense, it would also be very beneficial 👍 Maybe a combination of these options will be a nice middle ground, thanks a lot for the suggestion, I really like this proposal.

Hey @Mason-Lin, I like this idea! I totally agree that some improved flow for checking will be nice :) Maybe a tool like pre-commit, drkns, or even a collection of scripts in a Makefile may be less resource-heavy but allow the same as Dockerfile for checking the code styles :) However the Dockerfile will ensure that the environment is consistent and of course, in that sense, it would also be very beneficial 👍 Maybe a combination of these options will be a nice middle ground, thanks a lot for the suggestion, I really like this proposal.

I think your suggestion is good, and after rethinking it, I guess pre-commit install is the best fit because it's the easiest to set up. (For an experienced developer like you, these may not be a problem😅, my thoughts are just for your reference)

The reason is that people who use the pypiserver project do not necessarily have experience using docker and makefile because the most basic usage of pypiserver is to use pip install directly. (from QuickStart)

When developers find a requirement, they may be able to contribute to it, and the project's contribute guide can guide them to complete the PR smoothly with the lowest learning cost.
They don't need to understand makefile syntax or docker or even wsl (if their OS is windows). Sparks won't be doused by frustration with these pre-set-up processes.

Yes, maybe a combination of these options will be a nice middle ground.

# code quality check
FROM base AS check

# install code quality check requirements 
# RUN coverage, pytest, pylint, mypy, black
# Guide user to docker build target: check, if they have docker environment 

# Build production image
FROM base AS prd

WORKDIR /data
…