-
Notifications
You must be signed in to change notification settings - Fork 6
feat: support nexus api-version header #345
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
feat: support nexus api-version header #345
Conversation
Updated the Go SDK to set the `API-Version` header to the Nexus OpenAPI specification version when making API requests. Closes SSE-61.
fc68024 to
73bf93f
Compare
internal/generate/version.go
Outdated
| SDKVersion string | ||
| OpenAPIVersion string | ||
| }{ | ||
| SDKVersion: "v0.8.0", |
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.
Probably a good time to read this from VERSION and establish that as the source of truth for SDK version? It seems like an easy thing to miss, even with the release checklist.
I thought a quick embed would do the trick, but it can't access files from parent directory. But reading the file directly should still be simple enough. I can push a commit for this if you don't have to time to work on it.
If we prefer to keep the version here as well, I would suggest at least moving it to a const in the beginning of the file so it's easier to find it.
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'm personally not a huge fan of the external VERSION and OMICRON_VERSION files. I'd prefer to have a single source of truth for these versions, ideally within the Go source code itself. However, since we use the values within the Makefile I think it's okay to keep them as external files. I'll push a commit for it.
As an aside, I'd love to have a larger conversation around our usage of Makefiles versus moving more of that into Go itself.
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 updated the implementation according to your comments. Let me know what you think.
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 don't personally mind the version files. My main gripe was having the version defined (and having to be updated) in two places. One advantage of having it as a plain text file is that it's easier to parse from other tools, but we could also bring it to code by reading from the file into a variable.
As for Makefiles, I don't mind them either, as long as they're kept simple, but I would be curious to see what you have in mind.
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 also don't mind the version files. For me the issue comes in the form of sprawl. Like you mentioned, having plain text files makes it easy to parse the value but then when you have multiple files you get file sprawl. Then you put the version within Go code or some structured file like JSON and now you'll get tool sprawl with the Makefile needing to call out to something like jq to parse the versions.
There's no right answer here really. I'd be curious to try an all Go approach using go run and go tool but I'm sure it'll be more complicated than Makefile targets.
Updated the implementation to treat `VERSION` as the source of truth instead of having a sort of split brain scenario.
b43321d to
78ab480
Compare
Updated the Go SDK to set the
API-Versionheader to the Nexus OpenAPI specification version when making API requests.Closes SSE-61.