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

Health endpoint #3

Merged
merged 6 commits into from
Jan 17, 2024
Merged

Health endpoint #3

merged 6 commits into from
Jan 17, 2024

Conversation

nein09
Copy link
Collaborator

@nein09 nein09 commented Jan 17, 2024

I added a few tests and a very simple, configurable health check endpoint. Can you take a look at this and tell me if I'm on the right track? Or even test it out and see if it does what you want?

@nein09 nein09 requested a review from eldang January 17, 2024 21:59
@eldang
Copy link
Owner

eldang commented Jan 17, 2024

Nice, thank you! And thank you for adding to and improving the tests, too.

This needs just one more thing for our use case: a run-time flag to override the default /health path, like how we have no-preview, simply because that's going to be easier to integrate into the ADO workflow than anything in a config file.

To satisfy everyone posting in CrunchyData#131 there should also be a way to specify additional paths to the health endpoint. But I'm inclined to leave that as an exercise for someone who needs it, and/or me later on volunteer time outside the scope of the project which is funding this bit. Just adding the flag will satisfy our use case and the majority of other peoples'.

@nein09
Copy link
Collaborator Author

nein09 commented Jan 17, 2024

@eldang let me know how that flag suits you!

@eldang
Copy link
Owner

eldang commented Jan 17, 2024

Perfect, thank you.

I'm going to merge the PR within this repo now, and then would you like to make the PR to the upstream repository? It seems appropriate for you to get the credit having done the work.

@eldang eldang merged commit e8e0341 into master Jan 17, 2024
6 checks passed
@eldang
Copy link
Owner

eldang commented Jan 17, 2024

Not deleting the branch because it may be tidier to make the PR to upstream from this than from local-main.

@nein09
Copy link
Collaborator Author

nein09 commented Jan 17, 2024

ah, cool, I'll make the PR!

@eldang eldang deleted the health-endpoint branch January 17, 2024 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants