-
Notifications
You must be signed in to change notification settings - Fork 66
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
Bump dependencies (adds gotoolchain to go.mod) and fix CodeQL and unit test races #1875
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1875 +/- ##
=======================================
Coverage 38.14% 38.14%
=======================================
Files 346 346
Lines 43638 43638
=======================================
Hits 16646 16646
- Misses 26475 26477 +2
+ Partials 517 515 -2 ☔ View full report in Codecov by Sentry. |
d411970
to
ca6687d
Compare
go.mod
Outdated
go 1.21.3 | ||
|
||
toolchain go1.22.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.
Hmm, I don't think we want this change.
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.
One of our direct deps now says go 1.21.3
in its go.mod file. https://github.com/kubernetes/kube-openapi/blob/f107216b40e23161bb0fcfe35456c10cbd63b9c6/go.mod#L3
This was a very recent change in kube-openapi, and the diff can be seen here: kubernetes/kube-openapi@b9a9bc3
According to https://go.dev/doc/toolchain:
"The go get and go mod tidy commands maintain the go line to be greater than or equal to the go line of any required dependency module."
So it seems that if we want to update to the latest version of our kube-openapi dep, then we have no choice but to inherit its go
declaration.
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.
According to https://go.dev/ref/mod#go-mod-file-toolchain:
"For reproducibility, the go command writes its own toolchain name in a toolchain line any time it is updating the go version in the go.mod file (usually during go get)."
So we apparently have no choice but to start putting the toolchain
directive into our go.mod as well, unless we want to figure out how to explicitly override it.
It picked 1.22.0
because that is the version of Go that we are currently telling CI to use when running our dependency bumping automation tool.
Using 1.22.0 seems like a reasonable choice, because that is what is declared in our Dockerfile so that is what CI will use when building the server container image. It is also the version that CI is currently using to build the CLI binaries, build the test binaries, run the tests, etc.
Although the project currently should work fine with the older 1.21.x also (I think, we don't fully test this in CI), it seems like it would not hurt to add this toolchain
directive to effectively let the rest of the world know the version of Go that we actually use for our development work. The toolchain
directive "declares a suggested Go toolchain to use with a module", so it apparently does not prevent someone from using Go 1.21 if they really want to try that for some reason. They could change their GOTOOLCHAIN
environment variable as described in https://go.dev/doc/toolchain.
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.
For some reason, this breaks the "CodeQL / Analyze (go)" check on the PR. Its output includes this error message: The go.mod file requires version v1.21.3 of Go, but version v1.20.14 is installed. Consider adding an actions/setup-go step to your workflow.
.
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 added a new commit to this branch to attempt to resolve the problem with CodeQL by changing its workflow yaml file. I upgraded all our steps to use the latest versions and added a new step for actions/setup-go
. I also changed this repo's GitHub settings to allow actions actions/setup-go@*
.
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.
Why did the automation change the go
declaration in go.mod and add the toolchain
declaration?
See deprecation of v2 message in README.md at https://github.com/github/codeql-action Added setup-go because codeql code scanning stopped working and gave this error message: The go.mod file requires version v1.21.3 of Go, but version v1.20.14 is installed. Consider adding an actions/setup-go step to your workflow.
12d3f33
to
d888833
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.
Why did the automation change the go declaration in go.mod and add the toolchain declaration?
I think I figured this out and I left detailed comments above.
e0d92a5
to
28251f8
Compare
Automatically bumped all go.mod direct dependencies and/or images in dockerfiles.
@cfryanr also added commits to this branch to help it pass CI:
login_test.go
units tests because the unit tests flaked and failed 3 times in a row for this PR. These races already existed before this PR, but it seems like this PR will not make it through CI unless I fix them here.Note that you may need to upgrade to the latest version of GoLand after this merges, if you are using GoLand for Pinniped development.