-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Upgrade go version to 1.21 #10911
Upgrade go version to 1.21 #10911
Conversation
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
cc @chensun
@chensun could you take a look? |
yeah seems good to me. I thought about bumping the kfp-kubernetes one myself :) /lgtm |
@gregsheremeta: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chensun The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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 just noticed this PR and got interested. No need to change anything if you don't think it's relevant 😄
On another note: should we consider using Go workspaces to reduce duplication between projects? That would mean we only have to change the Go version in the go.work
file at the root of the repo
@@ -23,7 +23,7 @@ jobs: | |||
- name: Install Go | |||
uses: actions/setup-go@v4 | |||
with: | |||
go-version: 1.20.x | |||
go-version: 1.21.x |
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.
It might make sense to use something like this instead:
go-version: 1.21.x | |
go-version-file: './go.mod' |
This would reduce the number of places where we have to upgrade Go in the future.
Obviously there are multiple go.mod
s in this repo, so if there is one that makes more sense than the one in the root of the repository, it could be changed to that 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.
Hey @AndersBennedsgaard
I think these are very valid suggestions, but they are out of scope of this PR which is merely to upgrade the go version. My suggestion is to create an issue for this or propose a separate PR.
@DharmitD unfortunately tide is preventing merge due to: This might be because you need to rebase the new changes to the github workflows |
Signed-off-by: ddalvi <ddalvi@redhat.com>
Rebased the PR. @HumairAK @gregsheremeta @hbelmiro could any of you lgtm again? |
@DharmitD: The following tests failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
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
Description of your changes:
Resolves #10912
Checklist: