Conversation
| if call_arguments.method_properties.function == methods_v2.health: | ||
| # For ease of use, the health endpoint should be accessible without authentication. | ||
| return |
There was a problem hiding this comment.
What's the rationale for configuring this here vs in the (default) policy file?
There was a problem hiding this comment.
Good question. I think you are right that it would be better to include this in the policy.
There was a problem hiding this comment.
Given this a second though, I am on the fence now. The idea is to use the health endpoint in the docker health check and to rely on the assumption that it doesn't require authentication. So if users remove the rule from the policy file, it will break the docker health check. We could of course document this well and provide more flexibility to the user by adding it to the policy. What do you think?
There was a problem hiding this comment.
You make a good argument. I was only vaguely aware of the purpose of this endpoint. In that case I think it's more a property of the system than a policy, so I think it belongs here after all.
| def health() -> ReturnValue[None]: | ||
| """ | ||
| Returns a 200 response code if the server is healthy | ||
| or a 500 if the server is not healthy. |
There was a problem hiding this comment.
500 Internal Server Error? That's unconventional, isn't it?
There was a problem hiding this comment.
on K8S livenes probes (which are the best reference I have for this),
Any code greater than or equal to 200 and less than 400 indicates success. Any other code indicates failure.
the example there also uses 500 to indicate failure
There was a problem hiding this comment.
I didn't really find a clear answer on what is conventional. Some applications return 500 or 503 for not healthy. Others return a 200 and mention the status in the json body. What do you consider the conventional way?
There was a problem hiding this comment.
I just thought that 500 was reserved for unexpected errors. But Wouter's response and yours imply that that's a misunderstanding on my part.
There was a problem hiding this comment.
I'm not too sure either thb
There was a problem hiding this comment.
I was expecting that a specific response code would exist to indicate "unhealthy", but it seems there isn't one. But there is one for "I'm a teapot" :D
|
Processing this pull request |
|
Failed to merge changes into iso8 due to merge conflict. Please open a pull request for these branches separately by cherry-picking the commit that was made on the branch master (git cherry-pick 6657665). |
|
Failed to merge changes into iso7 due to merge conflict. Please open a pull request for these branches separately by cherry-picking the commit that was made on the branch master (git cherry-pick 6657665). |
|
Merged into branches master in 6657665 |
# Description The `/health` endpoint provides a yes/no answer on whether the server is healthy or not. It doesn't require authentication in contrast to the /serverstatus endpoint. Part of #9061 # Self Check: - [x] Attached issue to pull request - [x] Changelog entry - [x] Type annotations are present - [x] Code is clear and sufficiently documented - [x] No (preventable) type errors (check using make mypy or make mypy-diff) - [x] Sufficient test cases (reproduces the bug/tests the requested feature) - [x] Correct, in line with design - [ ] End user documentation is included or an issue is created for end-user documentation - [ ] ~~If this PR fixes a race condition in the test suite, also push the fix to the relevant stable branche(s) (see [test-fixes](https://internal.inmanta.com/development/core/tasks/build-master.html#test-fixes) for more info)~~
|
Not closing this pull request due to previously commented issues for some of the destination branches. Please open a separate pull request for those branches by cherry-picking the relevant commit. You can safely close this pull request and delete the source branch. |
|
This branch was not deleted as it seems to still be in use. |
# Description **This PR just adds a dummy changelog entry. We are not going to add the health endpoint in iso7, because iso7 is missing the capability for a slice to report its status. It's also not required because the iso7 docker image doesn't have a health check either. Parent PR #9239 The `/health` endpoint provides a yes/no answer on whether the server is healthy or not. It doesn't require authentication in contrast to the /serverstatus endpoint. Part of #9061 # Self Check: - [x] Attached issue to pull request - [x] Changelog entry - [x] Type annotations are present - [x] Code is clear and sufficiently documented - [x] No (preventable) type errors (check using make mypy or make mypy-diff) - [x] Sufficient test cases (reproduces the bug/tests the requested feature) - [x] Correct, in line with design - [ ] End user documentation is included or an issue is created for end-user documentation - [ ] ~~If this PR fixes a race condition in the test suite, also push the fix to the relevant stable branche(s) (see [test-fixes](https://internal.inmanta.com/development/core/tasks/build-master.html#test-fixes) for more info)~~
# Description **Same PR as #9239 but applied on the iso8 branch due to a merge conflict.** The `/health` endpoint provides a yes/no answer on whether the server is healthy or not. It doesn't require authentication in contrast to the /serverstatus endpoint. Part of #9061 # Self Check: - [x] Attached issue to pull request - [x] Changelog entry - [x] Type annotations are present - [x] Code is clear and sufficiently documented - [x] No (preventable) type errors (check using make mypy or make mypy-diff) - [x] Sufficient test cases (reproduces the bug/tests the requested feature) - [x] Correct, in line with design - [ ] End user documentation is included or an issue is created for end-user documentation - [ ] ~~If this PR fixes a race condition in the test suite, also push the fix to the relevant stable branche(s) (see [test-fixes](https://internal.inmanta.com/development/core/tasks/build-master.html#test-fixes) for more info)~~
Description
The
/healthendpoint provides a yes/no answer on whether the server is healthy or not. It doesn't require authentication in contrast to the /serverstatus endpoint.Part of #9061
Self Check:
If this PR fixes a race condition in the test suite, also push the fix to the relevant stable branche(s) (see test-fixes for more info)