Refactor/set openapi server url #272
Conversation
dylanhitt
force-pushed
the
refactor/set-openapi-serverURL
branch
2 times, most recently
from
9a126be to
f03071d
Compare
| Get(s, "/", func(*ContextNoBody) (MyStruct, error) { | ||
| return MyStruct{}, nil | ||
| }) |
There was a problem hiding this comment.
Nitpick: You can use dummyController for lisibility
| type OpenAPIConfig struct { | ||
| DisableSwagger bool // If true, the server will not serve the Swagger UI nor the OpenAPI JSON spec | ||
| DisableSwaggerUI bool // If true, the server will not serve the Swagger UI | ||
| DisableLocalSave bool // If true, the server will not save the OpenAPI JSON spec locally | ||
| SwaggerUrl string // URL to serve the swagger UI | ||
| UIHandler func(specURL string) http.Handler // Handler to serve the OpenAPI UI from spec URL | ||
| JsonUrl string // URL to serve the OpenAPI JSON spec | ||
| JsonFilePath string // Local path to save the OpenAPI JSON spec | ||
| PrettyFormatJson bool // Pretty prints the OpenAPI spec with proper JSON indentation | ||
| // Overrides the server description generated by fuego | ||
| Server *openapi3.Server | ||
| // If true, the server will not serve the Swagger UI nor the OpenAPI JSON spec | ||
| DisableSwagger bool |
There was a problem hiding this comment.
Not sure that a chunk (*openapi3.Server) of the OpenAPI Description should be in the OpenAPIConfig.
OpenAPIConfig is more about "How will Fuego handle the output of the OpenAPI Description". But as there's no comment, you wouldn't have guessed it, sorry about that!
There was a problem hiding this comment.
Yeah I figured as much. I personally think this does have to do with the OpenApi description. That's all this change is really focusing on.
| } | ||
|
|
||
| s.OpenAPI.Description().Servers = append(s.OpenAPI.Description().Servers, &openapi3.Server{ | ||
| URL: "http://" + s.Addr, | ||
| Description: "local server", | ||
| }) | ||
|
|
||
| s.startTime = time.Now() | ||
|
|
There was a problem hiding this comment.
Good thing to get rid of this here, thanks!
| s := NewServer( | ||
| WithOpenAPIConfig(OpenAPIConfig{ | ||
| Server: &openapi3.Server{ | ||
| URL: "foo", | ||
| Description: "bar", | ||
| }, | ||
| SwaggerUrl: "/api", | ||
| JsonUrl: "/api/openapi.json", |
There was a problem hiding this comment.
Mmh I prefer our users to code themselves:
s.OpenAPI.Description().Servers = append(...)Rather than giving them a shortcut that does exactly the same. I mean, we have no added value of doing this
| @@ -95,6 +95,14 @@ func declareAllTagsFromOperations(s *Server) { | |||
| // Also serves a Swagger UI. | |||
| // To modify its behavior, use the [WithOpenAPIConfig] option. | |||
| func (s *Server) OutputOpenAPISpec() openapi3.T { | |||
| if s.OpenAPIConfig.Server == nil { | |||
| s.OpenAPIConfig.Server = &openapi3.Server{ | |||
| URL: s.proto() + "://" + s.Addr, | |||
There was a problem hiding this comment.
So to sum up, the strong point of this PR is mostly this s.proto am I right?
Nice!
There was a problem hiding this comment.
No, it's to provide a way to configure a value that we've been setting for our users. This leads into #248.
If you don't think the configurability is worth it. Please let me just revert the single commit that adds the feature. Thanks
dylanhitt
force-pushed
the
refactor/set-openapi-serverURL
branch
from
f03071d to
90312b0
Compare
|
Closing per suggestions and #273 |
closes #248
Will conflict with #245 but it does solve the issue of creating the server too early so that code could be simplified I believe. This also adds native option to configure the openapi.Description() server.
Thanks