Skip to content

docker: add basic healthcheck #555

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

Merged
merged 2 commits into from
Dec 31, 2022
Merged

Conversation

bt90
Copy link
Contributor

@bt90 bt90 commented Dec 23, 2022

@binwiederhier couldn't spot any kind of version endpoint, or I would have preferred that.

@codecov-commenter
Copy link

codecov-commenter commented Dec 23, 2022

Codecov Report

Base: 66.14% // Head: 65.94% // Decreases project coverage by -0.19% ⚠️

Coverage data is based on head (dab18e5) compared to base (4e7e6e5).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #555      +/-   ##
==========================================
- Coverage   66.14%   65.94%   -0.20%     
==========================================
  Files          36       36              
  Lines        3801     3811      +10     
==========================================
- Hits         2514     2513       -1     
- Misses        925      934       +9     
- Partials      362      364       +2     
Impacted Files Coverage Δ
server/server.go 59.33% <0.00%> (-0.74%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@binwiederhier
Copy link
Owner

I added a preliminary health API endpoint for this. Seems like a good thing to have. For now it's just a dummy endpoint that returns 200, but we could make that more functional later: dd28296

curl -v localhost:2586/v1/health
*   Trying 127.0.0.1:2586...
* Connected to localhost (127.0.0.1) port 2586 (#0)
> GET /v1/health HTTP/1.1
> Host: localhost:2586
> User-Agent: curl/7.81.0
> Accept: */*
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< Access-Control-Allow-Origin: *
< Content-Type: text/json
< Date: Sat, 24 Dec 2022 17:25:05 GMT
< Content-Length: 17
< 
{"healthy":true}
* Connection #0 to host localhost left intact

@bt90
Copy link
Contributor Author

bt90 commented Dec 26, 2022

Should the /v1 be blacklisted in config.js ?

@binwiederhier
Copy link
Owner

The disallowed topics are just for paths that do not have a /, so v1/health is fine :-)

@bt90
Copy link
Contributor Author

bt90 commented Dec 27, 2022

@binwiederhier adapted the check for the v1/health endpoint.

@binwiederhier
Copy link
Owner

Do you think the grep is even necessary right now? I mean if it returns a non-200 code, wget will fail and retry anyway, right?

@bt90
Copy link
Contributor Author

bt90 commented Dec 28, 2022

Depends on the API. If a "health":false is guaranteed to be sent with a non-200 code, we can safely drop the grep.

It's just a bit odd as you're querying the health of the server and the status code is for the query. If the API would return "health":false because your server is burning, it would be a valid response for the query and i would expect a HTTP 200.

@bt90 bt90 marked this pull request as ready for review December 30, 2022 16:55
@binwiederhier
Copy link
Owner

and i would expect a HTTP 200

I honestly don't know what the best practice is here. I tried to Google but without success. Let's just leave it like this for now.

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.

3 participants