Initial Swagger compatibility proposal #15
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
|
@googlebot I signed it! |
| var SwaggerCodegenVersion = "2.4.5" | ||
|
|
||
| // MergedClient : whether to keep the generated Swagger clients separate or to merge the paths | ||
| // TODO: implement function that merges the paths of each Swagger spec into one file before running codegen |
There was a problem hiding this comment.
I'm not sure I follow this: what would the merge look like of each Swagger spec? What's the end result?
There was a problem hiding this comment.
TL;DR I took the liberty of pulling in the project API spec, and thought it might be nice to merge it with the grafeas spec at some point.
I hadn't taken the time to implement it yet, but the idea was (I'm sure you can speak to the validity of this more than I can) was to have a more uniform "client" experience. Right now, the code generation was only using grafeas.swagger.json from the upstream grafeas repository. Because of this, it is missing some of the project API interactions (e.g. create, delete, list, get). I think that this would be nice to have as part of the overarching client. So, at a minimum I have pulled in the projects.swagger.json from the upstream repo as part of the go generate
Define all swagger spec files
https://github.com/grafeas/client-go/pull/15/files#r379532306
Iterate over all swagger spec files
https://github.com/grafeas/client-go/pull/15/files#r379532718
With that in place the interaction looks something like
package main
import (
"context"
"fmt"
"log"
grafeasAPI "github.com/psprings/client-go/0.1.4"
)
func main() {
projectID := "projects/myproject"
grafeasCfg := &grafeasAPI.Configuration{
BasePath: "http://localhost:8080",
DefaultHeader: make(map[string]string),
UserAgent: "Swagger-Codegen/0.1.0/go",
}
grafeasClient := grafeasAPI.NewAPIClient(grafeasCfg)
notesResp, _, err := grafeasClient.GrafeasV1Beta1Api.ListNotes(context.Background(), projectID, nil)
if err != nil {
log.Fatal(err)
} else {
log.Print(notesResp.Notes)
}
}By adding in the project.swagger.json we now also get access to:
package main
import (
"context"
"fmt"
"log"
grafeasProjectAPI "github.com/psprings/client-go/0.1.4/project"
)
func main() {
grafeasProjectCfg := &grafeasProjectAPI.Configuration{
BasePath: "http://localhost:8080",
DefaultHeader: make(map[string]string),
UserAgent: "Swagger-Codegen/0.1.0/go",
}
grafeasProjectClient := grafeasProjectAPI.NewAPIClient(grafeasProjectCfg)
ctx := context.Background()
projectID := "projects/myproject"
createdProject, _, err := grafeasProjectClient.ProjectsApi.CreateProject(ctx, grafeasProjectAPI.ProjectProject{
Name: projectID,
})
if err != nil {
log.Println(err)
}
log.Println(createdProject)
projectsResp, _, _ := grafeasProjectClient.ProjectsApi.ListProjects(ctx, nil)
log.Printf("%#v", projectsResp)
}My thought was that it might be a more desirable/uniform experience to merge the spec files in such a way that you only have to do one client import. If I'm off base here, I can remove the project.swagger.json import and scrap this logic?
There was a problem hiding this comment.
Hey @aysylu what do you think about adding the project API to the generated client (to accompany the generated "grafeas" API) per my comment above?
Any thoughts on the notion of:
- Including the
project.swagger.jsonspec in the client generation (at a minimum)
[and/or]
- Merging the two swagger spec files to form a cohesive client capable with interacting with both APIs?
Presumably the Configuration type will always be the same underlying struct [literal] between the grafeasAPI and grafeasProjectAPI above.
// Grafeas API Configuration & Client
grafeasCfg := &grafeasAPI.Configuration{
BasePath: "http://localhost:8080",
}
grafeasClient := grafeasAPI.NewAPIClient(grafeasCfg)
// Project Configuration & Client
grafeasProjectCfg := &grafeasProjectAPI.Configuration{
BasePath: "http://localhost:8080",
}
grafeasProjectClient := grafeasProjectAPI.NewAPIClient(grafeasProjectCfg)Instead of having to do conversion or reflection or just duplicating the Configuration to interact with both parts of the API, it seems like it might be more desirable to the average consumer of the client library to interact with a common grafeasAPI client?
There was a problem hiding this comment.
Hi @psprings, thanks for clarifying! I absolutely agree that merging the 2 APIs for the more powerful client is a great idea. And it's a strict improvement over the previous state.
|
Thanks for this PR. I think it's a strict improvement over the current state of things with Swagger client generation, and I'm open to merging it once the PR is finalized and comments are addressed. |
| } | ||
|
|
||
| func getSwaggerNames() []string { | ||
| return []string{"grafeas.swagger.json", "project.swagger.json"} |
There was a problem hiding this comment.
Pulling in both swagger specs to have access to the ProjectsAPI in addition to the GrafeasV1Beta1Api
|
|
||
| func swaggerGenerate(mergedClient bool) { | ||
| swaggerSpecNames := getSwaggerNames() | ||
| for _, swaggerSpecName := range swaggerSpecNames { |
There was a problem hiding this comment.
Iterate over all defined swagger specs to generate all API interactions
|
Just thought of one more missing piece, which would be updating the |
| c.jar = stringOrDefault(c.jar, "./swagger-codegen-cli.jar") | ||
| c.configFile = stringOrDefault(c.configFile, "./config.go.json") | ||
| c.outputDirectory = stringOrDefault(c.outputDirectory, ServerVersion) | ||
| args := []string{"-jar", c.jar, "generate", "-i", c.inputSpec, "-l", c.language, "-o", c.outputDirectory, "-c", c.configFile} |
There was a problem hiding this comment.
Could use this logic instead of updating and committing the version in the config.json each time:
args := []string{"-jar", c.jar, "generate", "-i", c.inputSpec, "-l", c.language, "-o", c.outputDirectory, "-c", c.configFile, fmt.Sprintf("-DpackageVersion=", ServerVersion)}Or something like this to forego the file entirely
args := []string{"-jar", c.jar, "generate", "-i", c.inputSpec, "-l", c.language, "-o", c.outputDirectory, "-DpackageName=grafeas", fmt.Sprintf("-DpackageVersion=", ServerVersion)}There was a problem hiding this comment.
To clarify: what would be the difference in output in the 3 versions discussed here?
There was a problem hiding this comment.
Ultimately, the output in terms of directory name and content would be the same. From my perspective this just streamlines the process a bit, ensures a little more consistency (ensures packageVersion matches output directory override), and is a little more CI server friendly? Hopefully this helps explain it a little bit better.
Version 1 (as-is implementation)
This is essentially "feature parity" with the existing process. It relies on the config.go.json being updated by the user prior to running the client generate.
config.go.jsonprovides both thepackageVersionand thepackageNamein this example- command line args remain "unmodified" from the existing implementation
args := []string{"-jar", c.jar, "generate", "-i", c.inputSpec, "-l", c.language, "-o", c.outputDirectory, "-c", c.configFile}Which would yield something like:
java -jar ./swagger-codegen-cli.jar generate -i ./grafeas.swagger.json -l go -o 0.1.0 -c ./config.go.jsonVersion 2
This pulls in the "server version" that is being used to generate the client and overrides the packageVersion present in the config.go.json (or means, in essence, that you can omit this value from that file altogether). As a result, there is more of an assurance that the version is consistent, and could be more automation friendly.
The nice thing about this is that version can be passed in as one of the flags, or can be ingested via an environment variable. If neither are specified, this will grab the latest tag from the grafeas/grafeas repository and "release" that.
https://github.com/psprings/client-go/blob/swagger-compatibility/generate/main.go#L46-L54
config.go.jsonprovides only thepackageName- the command line args provide
packageVersion
args := []string{"-jar", c.jar, "generate", "-i", c.inputSpec, "-l", c.language, "-o", c.outputDirectory, "-c", c.configFile, fmt.Sprintf("-DpackageVersion=", ServerVersion)}Which would yield something like:
java -jar ./swagger-codegen-cli.jar generate -i ./grafeas.swagger.json -l go -o 0.1.0 -c ./config.go.json -DpackageVersion=0.1.0Version 3
If Version 2 was agreeable enough, it seems less meaningful to have a config.go.json file which only provides a single value (packageName) so you might as well just leverage that as a command line arg.
config.go.jsonprovides nothing (and can be removed)- the command line args provide both
packageNameandpackageVersion
args := []string{"-jar", c.jar, "generate", "-i", c.inputSpec, "-l", c.language, "-o", c.outputDirectory, "-DpackageName=grafeas", fmt.Sprintf("-DpackageVersion=", ServerVersion)}Which would yield something like:
java -jar ./swagger-codegen-cli.jar generate -i ./grafeas.swagger.json -l go -o 0.1.0 -c ./config.go.json -DpackageName=grafeas -DpackageVersion=0.1.0There was a problem hiding this comment.
Got it, thanks so much for the thorough clarifications on this! I agree that more automation-friendly approach would be better, and I like Version 2/3 because this allows for more automated generation of values. Moving packageName out of the configuration seems reasonable. For packageVersion, I'm open to automating this as well, with one detail: the intention here is to separate server release version from the client, eg server is at version 0.1.4 and the client is 0.2.1 because of some new features and bug fixes added to the client, which are independent of the server releases. What'd you suggest as the best way to address this, while minimizing the use of config.go.json?
There was a problem hiding this comment.
Ah, okay that makes sense. I was operating under the assumption that there was some coordination of the client version with the server version. We could pretty easily include a ClientVersion in the code which is accessible via ldflags when doing generation which could be supplied as a flag or otherwise passed as an environment variable (CLIENT_VERSION or GRAFEAS_CLIENT_VERSION etc) which would be ingested and would determine the output directory name and the packageVersion flag?
I was trying to avoid needing to vendor any additional (external) dependencies so, I don't have anything doing semantic version parsing or incrementing, but the logic could be something like
if len(ClientVersion) == 0 {
// I've used something like https://github.com/Masterminds/semver in the past
// for some of the parsing and incrementing functionality
ClientVersion = autoIncrementPatch()
}But this would rely on version being known at which point you would need a VERSION, version.go, or I suppose the config.go.json file unless you do some logic to extrapolate tag based on current commit (assuming that this repo would be subject to tags and releases).
All of that to say, I guess this could be manually supplied for now via the config.go.json, via a ldflag and/or environment variable?
There was a problem hiding this comment.
Yes, I agree that specifying the client version via config.go.json, via ldflag and env var would be great here, as you suggested above. My concern with auto-incrementing versions is that it'd be harder to encode whether micro, minor or major version should change based on the commits, but this is a decision humans can make more effectively.
Happy to merge this once the client version change is in. It sounds like we're on the same page about everything else!
There was a problem hiding this comment.
Great! I will try to set aside a little bit of time in the next day or two to get that sorted and committed.
|
@aysylu any feedback on the review comments/replies above before I start making some changes? |
|
Hi @psprings: sorry for the delay on the next round of review. I have an outstanding review draft to send it to you, will be ready by EOD today. Thanks for your patience! |
|
@aysylu no problem -- I know you're juggling a number of other issues across the Grafeas repos. I'll keep an eye out for your forthcoming review. I think there are a few people that have a vested interest in this, so I just wanted to make sure I gave this some attention while I still can remember some of the assumptions I made. |
| var SwaggerCodegenVersion = "2.4.5" | ||
|
|
||
| // MergedClient : whether to keep the generated Swagger clients separate or to merge the paths | ||
| // TODO: implement function that merges the paths of each Swagger spec into one file before running codegen |
There was a problem hiding this comment.
Hi @psprings, thanks for clarifying! I absolutely agree that merging the 2 APIs for the more powerful client is a great idea. And it's a strict improvement over the previous state.
| c.jar = stringOrDefault(c.jar, "./swagger-codegen-cli.jar") | ||
| c.configFile = stringOrDefault(c.configFile, "./config.go.json") | ||
| c.outputDirectory = stringOrDefault(c.outputDirectory, ServerVersion) | ||
| args := []string{"-jar", c.jar, "generate", "-i", c.inputSpec, "-l", c.language, "-o", c.outputDirectory, "-c", c.configFile} |
There was a problem hiding this comment.
To clarify: what would be the difference in output in the 3 versions discussed here?
|
Hi @psprings: we'd love to merge your contribution into the repository. If you don't have time to address the final clarification, could you please let us know and we'd be happy to follow up with the fix. If you could please sign the CLA, then we can merge the PR as is, to ensure credit goes where credit's due.:) |
@aysylu sorry, I've been heads down on some work on a few COVID-focused projects in my spare time since all of this craziness started. I'll try to get that CLA addressed and if I have a few moments today see how much effort it would take to get that spec merge sorted out. |
All good and understandable! I'll keep this PR open for when you get a chance. And as I mentioned earlier, the final clarification can be addressed by one of us in a follow-up PR, no problem. Hope you're staying healthy and safe! |
@aysylu thanks, hope all is well on your end as well! I had a few minutes today and took a really rough pass at the merged client concept. Just need to do some cleanup and a few more tests then it should be good to go. |
Sounds great, thank you! Looking forward to this. |
Hi @psprings - is this something you are still considering moving forward with? |
| } | ||
|
|
||
| func swaggerCompatibility(jsonInput string) string { | ||
| re := regexp.MustCompile(`\/{.*(=.*)}`) |
There was a problem hiding this comment.
Small comment on this: because GetNote: "/v1beta1/{name=projects/*/notes/*}" and GetOccurrence: "/v1beta1/{name=projects/*/occurrences/*}" both share the same form of /v1beta1/{name}, the swagger generation only renders GetOccurrence.
I don't think there's a way of fixing this other than changing the grafeas.swagger.json.
|
I have raised a PR that will fix the openapi paths and not require the regex bit in this PR: https://github.com/grafeas/grafeas/pull/532/files @psprings are you ok with me taking the work you've done here and re-raising a change? |
Description
This is an idea to modify the existing
go generateprocess to use more "OS-agnostic" logic by leveraging Go to handle the file downloads, string replacement, and ultimately the execution of Swagger Codegen in response to #13#13 (comment)
This could really be stored in the https://github.com/grafeas/grafeas repository, and run as part of the CI process when a release is tagged. It could technically accept "language" as one of the
-ldflags(or adapted to use environment variables, args, etc) and be used to generate clients for any language.Details
http.Getinstead ofwgethttp.Getinstead ofwgetproject.swagger.jsonsince the "project client" was missingWill wait to generate and submit the updated client after an approach can be agreed upon