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

[feat] Healthcheck to consider MQTT only if enabled #469

Closed
docbobo opened this issue Feb 5, 2021 · 5 comments · Fixed by #500
Closed

[feat] Healthcheck to consider MQTT only if enabled #469

docbobo opened this issue Feb 5, 2021 · 5 comments · Fixed by #500
Assignees
Labels
enhancement New feature or request

Comments

@docbobo
Copy link
Contributor

docbobo commented Feb 5, 2021

Is your feature request related to a problem? Please describe.
It's a mix between a bug and a feature request. Right now, documentation for the Healthcheck (/health) says that 200 OK is being returned when both MQTT and zwave are up and running. I am running my system with MQTT disabled and the /health check returns 500. Technically, this is exactly as described. However, I feel that this is not the desired behavior.

Describe the solution you'd like
I'd like to see a general healtcheck like /health that takes into account if MQTT is enabled.

Describe alternatives you've considered
If anybody wants to provide a general purpose kubernetes recipe, switching between different kind of probes is not an option and will result in problems. For example, the deployment.yaml in the repo will only work if MQTT is enabled. Otherwise, it will kill the container after 12 attempts.

@docbobo docbobo added the enhancement New feature or request label Feb 5, 2021
@robertsLando
Copy link
Member

@docbobo Maybe it's not documented but you can request the healthcheck of just mqtt/zwave by adding mqtt or zwave as suffix.

Source: https://github.com/zwave-js/zwavejs2mqtt/blob/master/app.js#L225

@robertsLando
Copy link
Member

@docbobo
Copy link
Contributor Author

docbobo commented Feb 6, 2021

I am fully aware that it's documented. That's the reason why I filed this as a feature request, not as a bug. BTW, that's also what I wrote in the description above ;)

Here's why I still think it matters:

  • The GitHub repository contains Kubernetes manifests. Those manifests use /health as a liveliness probe.
  • When MQTT is enabled, that all works nicely.
  • When someone disables MQTT, everything almost looks well for a while. Unless the 12 attempts are right, at which point the container is killed and restarted.

Purely from an API design perspective, I feel that there should be an API that is agnostic to all of that. An API that will report the health status of all the components that are enabled.

Hope this makes more sense now ;)

@robertsLando
Copy link
Member

robertsLando commented Feb 6, 2021

OK now I got it, in poor words you would like the /health api to check also settings and return 200 if mqtt is disabled instead of return 500. Seems legit, sorry I didn't get this in your first question

BTW AFAIK you can customize the healthcheck endpoint, don't you?

@robertsLando robertsLando reopened this Feb 6, 2021
@docbobo
Copy link
Contributor Author

docbobo commented Feb 6, 2021

Yes, that's what I meant.

And don't get me wrong: I changed the endpoint and have everything working. The current design just makes it impossible to e.g. create a helm chart that works out of the box for everyone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants