-
Notifications
You must be signed in to change notification settings - Fork 9
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: use new version pkg #119
Conversation
Codecov Report
@@ Coverage Diff @@
## master #119 +/- ##
===========================================
- Coverage 100.00% 93.78% -6.22%
===========================================
Files 15 15
Lines 162 161 -1
===========================================
- Hits 162 151 -11
- Misses 0 10 +10
|
if gin.Mode() == "debug" { | ||
c.Header("X-Vela-Version", apiVersion.String()) | ||
c.Header("X-Vela-Version", v.Semantic()) | ||
} else { // in prod we don't want the build number metadata | ||
apiVersion.Metadata = "" | ||
c.Header("X-Vela-Version", apiVersion.String()) | ||
c.Header("X-Vela-Version", v.Semantic()) |
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.
maybe i missed something, but do we need this if statement if we're returning the same thing?
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.
No, we don't need the if statement 👍
If you'd like I can comment out the if
but I left it in favor of being explicit on how we could customize it in the future.
In short, my thought process was by leaving it there, if (or when) we want to look at customizing how that header is populated from the API when running in debug
mode then the shell of the code is there.
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.
K, not a big deal to leave it in 👍
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.
LGTM 🐬
Dependent on go-vela/types#112
Updating to the new
go-vela/types/version
package to improve the information provided surrounding the application.If I compile the binary specifically for the OS on my workstation, I'm met with this from the
--version
flag:When I run
make up
locally, here is the output from http://localhost:8080/version:NOTE: This also updates the GitHub Actions pipelines to use our
make
commands for building the binary.