Adding websocket event publisher #65
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #65 +/- ##
===========================================
- Coverage 71.84% 71.48% -0.36%
===========================================
Files 20 20
Lines 959 996 +37
===========================================
+ Hits 689 712 +23
- Misses 270 284 +14
Continue to review full report at Codecov.
|
brew_view/__main__.py
Outdated
| @@ -23,6 +23,9 @@ def shutdown(): | |||
| brew_view.logger.info("Stopping server.") | |||
| brew_view.server.stop() | |||
|
|
|||
| # Close any open websocket connections | |||
There was a problem hiding this comment.
I'd rather see this as a docstring on the shutdown method.
brew_view/__init__.py
Outdated
| @@ -120,6 +126,7 @@ def _setup_tornado_app(): | |||
| (r'{0}config/swagger/?'.format(prefix), SwaggerConfigHandler), | |||
| (r'{0}version/?'.format(prefix), VersionHandler), | |||
| (r'{0}api/v1/spec/?'.format(prefix), SpecHandler), | |||
| (r'{0}events/?'.format(prefix), EventSocket), | |||
There was a problem hiding this comment.
I would think we would want to version this endpoint also? If we aren't sure, I'd still expose it on api/vbeta/websocket or something to that affect.
There was a problem hiding this comment.
Good call - my initial thought is to put it with the versioned ones. Let me know if you'd rather have it in beta.
There was a problem hiding this comment.
Ahh, I remember why I punted on this decision 😄
We already have a POST api/vbeta/events/ route that allows for publishing your own events. I think we eventually want to support retrieving events by an ID (assuming you've been saving them to mongo)?
Now I'm thinking it would be better to have this be something besides events so we don't muddy the RESTfulness.
There was a problem hiding this comment.
And now I remember more ... the apispec package we're using to construct the swagger spec doesn't like that we're putting a websocket handler in our 'published' path list. Which makes sense - it's not really designed for non-REST things. See:
swagger-api/swagger-socket#47
OAI/OpenAPI-Specification#55
So far all of the API we've wanted to publish has fit nicely into REST, so it's fit nicely into swagger (and apispec). We'll have to decide how to move forward with documenting the websocket support.
|
Yeah, this actually came about because I was annoyed at the admin page. I had a branch from a while ago where I did this and started reworking the admin page. Figured it was good to break them up and just get this merged. |
No description provided.