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

Added /ping to /api/v2 #147

Merged
merged 3 commits into from Mar 28, 2017
Merged

Added /ping to /api/v2 #147

merged 3 commits into from Mar 28, 2017

Conversation

giorgiosironi
Copy link
Contributor

@giorgiosironi giorgiosironi commented Mar 27, 2017

To be targeted by journal in its status page and possibly by monitoring.

Copy link
Contributor

@lsh-0 lsh-0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple of small things to fix up.
if the caching headers for /ping are different from caching headers for other responses, please leave a comment in def ping()

@@ -2,6 +2,7 @@
from . import api_v2_views as views

urlpatterns = [
url(r'/ping$', views.ping, name='ping'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no leading slash

def ping(request):
"returns a test response"
# produce standard authenticated header
is_authenticated(request)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this isn't doing anything. is_authenticated has no side effects

"returns a test response"
# produce standard authenticated header
is_authenticated(request)
return Response('pong', content_type='text/plain; charset=UTF-8', headers={'Cache-Control': 'must-revalidate, no-cache, no-store, private'})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if these cache headers are different for the /ping endpoint than for all the others, then this is fine, otherwise, caching headers are handled in custom middleware in core.middleware (beneath the kong authentication middleware) and this can be removed.

resp = self.c.get(reverse('v2:ping'))
self.assertEqual(resp.status_code, 200)
self.assertEqual(resp.content_type, 'text/plain; charset=UTF-8')
self.assertEqual(resp['Cache-Control'], 'must-revalidate, no-cache, no-store, private')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, double handling if /ping cache headers are no different from regular caching headers.

@giorgiosironi
Copy link
Contributor Author

Changes made

@lsh-0 lsh-0 merged commit 7f8edd0 into develop Mar 28, 2017
@lsh-0 lsh-0 deleted the ping branch March 28, 2017 22:05
@giorgiosironi
Copy link
Contributor Author

For some reason this failed when in develop, trying to reproduce locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants