-
Notifications
You must be signed in to change notification settings - Fork 56
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
fix(openapi): remove spec file generation from live code path #1290
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3321,11 +3321,9 @@ | |
"operationId": "ListUsers", | ||
"parameters": [ | ||
{ | ||
"example": "email@example.com", | ||
"in": "query", | ||
"name": "email", | ||
"schema": { | ||
"example": "email@example.com", | ||
Comment on lines
-3324
to
-3328
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was surprised to see these changes, but running the test on I guess that the API definitions changed in another PR that was merged at the same time as the openapi spec generation changes. |
||
"type": "string" | ||
} | ||
}, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
package main | ||
|
||
import ( | ||
"fmt" | ||
"os" | ||
|
||
"github.com/infrahq/infra/internal/server" | ||
) | ||
|
||
func main() { | ||
if err := run(os.Args[1:]); err != nil { | ||
fmt.Fprintln(os.Stderr, err.Error()) | ||
os.Exit(1) | ||
} | ||
} | ||
|
||
func run(args []string) error { | ||
if len(args) < 1 { | ||
return fmt.Errorf("missing command line argument: path to openapi spec file") | ||
} | ||
filename := args[0] | ||
|
||
s := server.Server{} | ||
s.GenerateRoutes() | ||
|
||
return server.WriteOpenAPISpecToFile(filename) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,25 +1,20 @@ | ||
package server | ||
|
||
import ( | ||
"os" | ||
"strings" | ||
"testing" | ||
|
||
"github.com/stretchr/testify/require" | ||
) | ||
|
||
func TestOpenAPIGen(t *testing.T) { | ||
// must run from infra root dir | ||
wd, err := os.Getwd() | ||
require.NoError(t, err) | ||
|
||
parts := strings.Split(wd, string(os.PathSeparator)) | ||
|
||
if parts[len(parts)-1] != "infra" { | ||
err := os.Chdir("../..") | ||
Comment on lines
-13
to
-19
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
require.NoError(t, err) | ||
} | ||
|
||
s := &Server{} | ||
// TestWriteOpenAPISpec is not really a test. It's a way of ensuring the openapi | ||
// spec is updated. | ||
// TODO: replace this with a test that uses golden, and a CI check to make sure the | ||
// file in git matches the source code. | ||
func TestWriteOpenAPISpec(t *testing.T) { | ||
s := Server{} | ||
s.GenerateRoutes() | ||
|
||
filename := "../../docs/api/openapi3.json" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can't assume the test will always run from this directory. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See my comment, you can! (although I can't find a good docs link right now, I'll keep looking) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess I'm mistaken. I tested different run methods and couldn't find a problem. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure where it is documented, but I've always seen it work this way. Somewhere in |
||
err := WriteOpenAPISpecToFile(filename) | ||
require.NoError(t, err) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -92,8 +92,6 @@ func (a *API) registerRoutes(router *gin.RouterGroup) { | |
|
||
get(unauthorized, "/version", a.Version) | ||
} | ||
|
||
generateOpenAPI() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was the main goal of this PR, to remove this from the production code path. |
||
} | ||
|
||
func get[Req, Res any](r *gin.RouterGroup, path string, handler ReqResHandlerFunc[Req, Res]) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is weird. should not have changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment below :)
It happens on
main
as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked this and you are correct, it was removed in main and not regenerated