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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

HealthCheck as an interface #1283

Closed

Conversation

chids
Copy link
Contributor

@chids chids commented Mar 10, 2018

Been longing for HealthCheck to be an interface for some time and had som recent code that'd been better of if it were so decided to take a stab at it. If for nothing else to allow easier assessment of the impact of such a change.

(wasn't able to run all tests locally due to networking weirdnesses, partly addressed in #1282, but hey it complies 馃槵)

Closes #697
Partly addresses #846

@arteam
Copy link
Member

arteam commented Mar 11, 2018

Thanks for the PR! The biggest setback for turning HealthCheck into an interface from an abstract class was backwards compatibility between Metrics 3.2 and Metrics 4.0. Ideally, users shouldn't rewrite their health checks when upgrading to Metrics 4.0.

Nevertheless, it seems reasonable to implement this change in Metrics 5.0 along with other breaking changes.

@chids
Copy link
Contributor Author

chids commented Mar 12, 2018

Makes sense. Do you want me to create a PR against the 5.0-development branch instead?

@arteam
Copy link
Member

arteam commented Mar 14, 2018

Yes, please. That would be great!

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.

None yet

2 participants