-
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
Merged
Merged
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.3in its go.mod file. https://github.com/kubernetes/kube-openapi/blob/f107216b40e23161bb0fcfe35456c10cbd63b9c6/go.mod#L3This 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:
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
godeclaration.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:
So we apparently have no choice but to start putting the
toolchaindirective into our go.mod as well, unless we want to figure out how to explicitly override it.It picked
1.22.0because 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
toolchaindirective to effectively let the rest of the world know the version of Go that we actually use for our development work. Thetoolchaindirective "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 theirGOTOOLCHAINenvironment 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 actionsactions/setup-go@*.