Feature/bare status #114
Feature/bare status #114
Conversation
|
Thanks for the submission, I’ll try to take a look soon. 😊 |
|
@mwarkentin Did you get a chance to look at this yet? |
|
Hey @jamesmallen - initial review looks good, just trying to review the diff in |
There was a problem hiding this comment.
@jamesmallen Sorry about the delay - had a chance to run through it and I think everything looks good. Just a few small tweaks and I'll be happy to merge!
watchman/views.py
Outdated
| def dashboard(request): | ||
| _deprecation_warnings() | ||
| def bare_status(request): | ||
| checks, ok = run_checks(request) |
There was a problem hiding this comment.
Do you want to handle the "no checks" case here or are you ok with a 200 even if nothing has been checked?
There was a problem hiding this comment.
For our needs, a 200 even if nothing has been checked is fine - that is a legitimate situation where the webserver is up and responding from our POV (and there might not be any other subsystems to check)
There was a problem hiding this comment.
Not sure if you saw the ping endpoint which was added in the previous release, it’s use case is very similar.
| @@ -172,6 +172,17 @@ requests, you can call ping: | |||
| It will return the text ``pong`` with a 200 status code. Calling this doesn't | |||
| run any of the checks. | |||
|
|
|||
| Bare status view | |||
There was a problem hiding this comment.
Can you add a 0.15.0 (Unreleased) heading to the history file and add a bullet for this feature?
| self.assertEqual(response.content.decode(), '') | ||
|
|
||
| @patch('watchman.checks._check_databases') | ||
| @override_settings(WATCHMAN_ERROR_CODE=503) |
There was a problem hiding this comment.
Can you add another test that the default error code for the bare status view will be 500? You can just copy this one without the override.
watchman/views.py
Outdated
|
|
||
| return HttpResponse('pong', content_type='text/plain') | ||
| return checks, 200 if ok else settings.WATCHMAN_ERROR_CODE, {WATCHMAN_VERSION_HEADER: __version__} |
There was a problem hiding this comment.
Would you mind pulling the http_code out into it's own variable directly above? Just makes this line a bit easier to parse I think (same for in bare_status).
There was a problem hiding this comment.
@jamesmallen did this get pushed to this branch? I don't see the update on the PR.
Not sure if it's a Github issue or what.
|
I accidentally pushed to a different branch. Everything should be fixed (in the correct branch!) in 446c889 |
|
@jamesmallen Sorry, one more update if possible - could you update the sample project with the required configuration to make this endpoint available at |
|
If you’re not sure how to do this, let me know and I can take care of it. |
|
Sure thing. Added it in ae48537 |
Addresses #113