Skip to content

Conversation

JoelSpeed
Copy link
Member

@JoelSpeed JoelSpeed commented Jun 14, 2020

Description

Adds healthchecking handler as a middleware

Motivation and Context

Handle all healthchecking in one place. Allow testing of healthchecking separate from other components. Allow registering health check middleware before logging handler to implement silent logging.

How Has This Been Tested?

Unit testing and manual testing using local testing environment

Checklist:

  • My change requires a change to the documentation or CHANGELOG.
  • I have updated the documentation/CHANGELOG accordingly.
  • I have created a feature (non-master) branch for my PR.

CC @chkohner @kskewes (I feel you may both have an interest in this)

@karlskewes
Copy link
Contributor

Nice, this is much cleaner. LGTM.

To remove the need for GCP specific code I guess we would need to make opts.PingPath a slice of strings rather than a single string. That could always be done later if desired.

@JoelSpeed
Copy link
Member Author

To remove the need for GCP specific code I guess we would need to make opts.PingPath a slice of strings rather than a single string. That could always be done later if desired.

I had imaged we just deprecate the options and say to people that they will have to migrate their apps to use a single health/readiness endpoint, it shouldn't be very difficult I wouldn't have thought

@JoelSpeed
Copy link
Member Author

Just did a bunch of manual testing of this and I'm happy that this is working as expected and retains all of the existing healthcheck behavour

@JoelSpeed
Copy link
Member Author

Merging based on my own and @kskewes review, if anyone has any later comments I'll address those in a follow up

@JoelSpeed JoelSpeed merged commit 713c392 into master Jun 19, 2020
@JoelSpeed JoelSpeed deleted the healthcheck-middleware branch June 19, 2020 17:15
Jing-ze pushed a commit to Jing-ze/oauth2-proxy that referenced this pull request Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants