-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
release-1.15: Reduce minimum go toolchain in go.mod. #8399
release-1.15: Reduce minimum go toolchain in go.mod. #8399
Conversation
43c81aa
to
7faca77
Compare
327461d
to
6151a0b
Compare
6151a0b
to
daad3bd
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## release-1.15 #8399 +/- ##
=============================================
Coverage 58.98% 58.98%
=============================================
Files 367 367
Lines 38855 38855
=============================================
Hits 22919 22919
Misses 14474 14474
Partials 1462 1462 ☔ View full report in Codecov by Sentry. |
daad3bd
to
d083d39
Compare
pkg/cmd/cli/version/version.go
Outdated
@@ -69,6 +69,7 @@ func printVersion(w io.Writer, clientOnly bool, kbClient kbclient.Client, server | |||
fmt.Fprintln(w, "Client:") | |||
fmt.Fprintf(w, "\tVersion: %s\n", buildinfo.Version) | |||
fmt.Fprintf(w, "\tGit commit: %s\n", buildinfo.FormattedGitSHA()) | |||
fmt.Fprintf(w, "\tGo version: %s\n", buildinfo.GoVersion()) |
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.
Is this clear enough that it is not user's local go? do we want to say Built by Go Version
instead?
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.
IMO, the information is good enough. Binary print information should already imply it's related to the binary built environment. It's not about the environment running it.
f77f6e3
to
9679f60
Compare
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.
Removed Go Version from CLI/logs per community meeting.
9679f60
to
fd879ed
Compare
Signed-off-by: Tiger Kaovilai <[email protected]>
fd879ed
to
c3967c3
Compare
go 1.22.8 | ||
// Do not pin patch version here. Leave patch at X.Y.0 | ||
// Unset GOTOOLCHAIN to assume GOTOOLCHAIN=local where go cli version in path is used. | ||
// Use env GOTOOLCHAIN=auto to allow go to decide whichever is newer from go.mod or cli in path. |
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.
How does this change affect the release process? E.g., release of 1.15.1.
Do we need to set GOTOOLCHAIN
anywhere during the release?
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.
Nope.. no need to set GOTOOLCHAIN. Per prior versions of this PR which verified the golang version used to build, it inherits local go version
by default which right now is defined by the Dockerfile.
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.
Go version validation removed in
https://github.com/vmware-tanzu/velero/compare/f77f6e33f3d19e2fb41ee22e1440c5e285b3ecaf..9679f607a16d74a6bf96c75d829a65679fbcf1cc
per #8398 (comment)
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.
TL;DR: this change keeps 1.22.8 as go toolchain for this repo without enforcing it as minimum version on others.
Verification (click me!) prior to #8398 review
Thank you for contributing to Velero!
Please add a summary of your change
Reduce minimum go toolchain + add verification from #8398
Update: verification was removed per review in #8398, but we did validated this pr earlier that the correct toolchain was applied.
Does your change fix a particular issue?
Fixes #8397
Please indicate you've done the following:
make new-changelog
) or comment/kind changelog-not-required
on this PR.site/content/docs/main
.