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

Added http health check endpoint. #137

Merged
merged 5 commits into from
Jan 25, 2019
Merged

Added http health check endpoint. #137

merged 5 commits into from
Jan 25, 2019

Conversation

Kugelschieber
Copy link
Contributor

Added:

  • GET endpoint on /health to check if the server is up and running
  • unit test for it
  • documentation in readme
  • added .idea/ to .gitignore (to ignore GoLand IDE settings)

@Kugelschieber
Copy link
Contributor Author

Kugelschieber commented Jan 25, 2019

My test passed, but TestUpdateAllowedFromIP failed. Any idea why?

@joohoi
Copy link
Owner

joohoi commented Jan 25, 2019

My test passed, but TestUpdateAllowedFromIP failed. Any idea why?

Weird indeed. Checking it out now.

@joohoi
Copy link
Owner

joohoi commented Jan 25, 2019

Ok, so there was a bug in the failing test. The previous order that the tests were run ensured that the failing test had a non-default state for Config struct. This is fixed in #139 which I'll merge ASAP. That should fix this issue.

For the scope of this PR, it looks great! Thanks! The only modification I'd like to see would be ensuring returning HTTP status 200, as the default behavior of the underlying framework might change in the future. I'm thinking something simple along the lines of:

// Endpoint used to check the readiness and/or liveness (health) of the server.
func healthCheck(w http.ResponseWriter, r *http.Request, _ httprouter.Params) {
	// does nothing except returning status code 200 (which it does by default)
	w.WriteHeader(http.StatusOK)
}

@joohoi
Copy link
Owner

joohoi commented Jan 25, 2019

Oh, and if you could add a note under "Unreleased" in Changelog about the change.

@Kugelschieber
Copy link
Contributor Author

Kugelschieber commented Jan 25, 2019

Like this?

## Changelog


- master
   - Unreleased
      - Added new endpoint to perform health checks
   - Changed
      - A new protocol selection for DNS server "both", that binds both - UDP and TCP ports.
- v0.6

@coveralls
Copy link

coveralls commented Jan 25, 2019

Coverage Status

Coverage increased (+0.03%) to 92.937% when pulling 0d8b61c on Kugelschieber:feature/http-health-check-endpoint into 7fbb526 on joohoi:master.

@joohoi
Copy link
Owner

joohoi commented Jan 25, 2019

Thanks for your work! LGTM, and merging when the test runs finish!

@joohoi joohoi merged commit aff13a0 into joohoi:master Jan 25, 2019
jacobmyers-codeninja pushed a commit to jacobmyers-codeninja/acme-dns that referenced this pull request Sep 30, 2020
* Added http health check endpoint.

* Fixed performing POST on GET endpoint.

* Explicitly return http status 200 in health check endpoint.

* Updated changelog.
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