-
Notifications
You must be signed in to change notification settings - Fork 211
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 kubectl-retina version
#1023
base: main
Are you sure you want to change the base?
Conversation
@microsoft-github-policy-service agree |
I think the issue lies with the passing of the Currently, my approach involves using logging to verify if If possible, could you enable CI testing for this? @nddq |
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.
Can we get rid of the changes to go.mod
and go.sum
, doesn't seem relevant to this PR.
@@ -15,6 +15,9 @@ ENV GOOS=${GOOS} | |||
ARG GOARCH=amd64 | |||
ENV GOARCH=${GOARCH} | |||
|
|||
# Debugging: Print the values of VERSION and APP_INSIGHTS_ID | |||
RUN echo "VERSION=${VERSION}" && echo "APP_INSIGHTS_ID=${APP_INSIGHTS_ID}" |
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 remove the debug lines before merging?
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.
+1 . We should remove the debug lines.
@@ -11,11 +11,20 @@ import ( | |||
"github.com/microsoft/retina/internal/buildinfo" | |||
) | |||
|
|||
// This variable is used by the "version" command and is set during build. | |||
// Default to a safe value if not set. | |||
var defaultVersion = "v0.0.5" |
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.
Wouldn't it be a misleading info? I don't think it makes sense to have a "default" version
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.
Yeah, I'd almost rather an empty string here and a loud notification that this wasn't set by some pipeline.
@@ -15,6 +15,9 @@ ENV GOOS=${GOOS} | |||
ARG GOARCH=amd64 | |||
ENV GOARCH=${GOARCH} | |||
|
|||
# Debugging: Print the values of VERSION and APP_INSIGHTS_ID | |||
RUN echo "VERSION=${VERSION}" && echo "APP_INSIGHTS_ID=${APP_INSIGHTS_ID}" |
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.
+1 . We should remove the debug lines.
fmt.Println("buildinfo.Version is not set successfully, showing default version:") | ||
fmt.Println(defaultVersion) |
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 think you can probably get a more useful "default" value out of https://pkg.go.dev/runtime/debug#BuildInfo . There's a property in there for the vcs.revision
which should be the commit the binary was built with. It won't be the tag, but the commit is plenty useful (and would be handy for development builds).
@@ -11,11 +11,20 @@ import ( | |||
"github.com/microsoft/retina/internal/buildinfo" | |||
) | |||
|
|||
// This variable is used by the "version" command and is set during build. | |||
// Default to a safe value if not set. | |||
var defaultVersion = "v0.0.5" |
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.
Yeah, I'd almost rather an empty string here and a loud notification that this wasn't set by some pipeline.
@@ -22,7 +22,7 @@ retract ( | |||
|
|||
require ( | |||
cel.dev/expr v0.15.0 // indirect | |||
code.cloudfoundry.org/clock v1.0.0 // indirect | |||
code.cloudfoundry.org/clock v1.21.0 // indirect |
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'd drop the go.mod
and go.sum
changes from this. We're not changing any dependencies here, and the upgrades should be handled by dependabot. That'll make the PR simpler.
Description
Since
v0.0.17
,kubectl-retina version
will only show a empty line, demonstrated in #1013Related Issue
#1013
Checklist
git commit -S -s ...
). See this documentation on signing commits.Screenshots (if applicable) or Testing Completed
None
Additional Notes
None
Please refer to the CONTRIBUTING.md file for more information on how to contribute to this project.