For OpenAPI, enable CORS. #17
Conversation
|
What prompted the rename of |
|
I see what you did, they are now part of the |
|
@jmrodri take another look, thanks! |
|
@n3wscott I like that. 👍 |
|
LGTM |
|
|
||
| if s.EnableCORS { | ||
| //Allow CORS here By * or specific origin | ||
| w.Header().Set("Access-Control-Allow-Origin", "*") |
There was a problem hiding this comment.
Will you make constants for these in a follow-up PR?
|
|
||
| // writeResponse will serialize 'object' to the HTTP ResponseWriter | ||
| // using the 'code' as the HTTP status code | ||
| func (s *APISurface) writeResponse(w http.ResponseWriter, code int, object interface{}) { |
There was a problem hiding this comment.
I worry a little bit about these methods accumulating logic, but I can't see a better way at this point than making them methods with APISurface as a receiver.
There was a problem hiding this comment.
only way i can see is to always pass in the enableCORS as a boolean to the writeResponse. Or you'd have to get fancy with functions or middleware. So I think this is okay.
pkg/rest/writer.go
Outdated
|
|
||
| if s.EnableCORS { | ||
| //Allow CORS here By * or specific origin | ||
| w.Header().Set("Access-Control-Allow-Origin", "*") |
There was a problem hiding this comment.
I'd prefer to see this value be configurable by the broker, instead of blanket allowing any origin.
There was a problem hiding this comment.
Are you okay with that in a follow-up?
| Metrics *metrics.OSBMetricsCollector | ||
| Broker broker.Interface | ||
| Metrics *metrics.OSBMetricsCollector | ||
| EnableCORS bool |
There was a problem hiding this comment.
Would be great to add some comment docs for this, above this line.
There was a problem hiding this comment.
I would take the doc comment in a follow-up. Are you LGTM otherwise?
There was a problem hiding this comment.
Sure 👍 But let's wait for @carolynvs as she had a comment.
|
LGTM |
vendor: Cleanup unused files
I wanted to test my new fancy broker with the swagger UI but CORS was not enabled and no way to do it. So I added it.