-
Notifications
You must be signed in to change notification settings - Fork 4.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
[NET-4968] Upgrade Go to 1.21 #20062
Conversation
35f9114
to
01567a7
Compare
d02fcc3
to
9379260
Compare
6e5b640
to
31ea777
Compare
GOLANGCI_LINT_VERSION='v1.51.1' | ||
GOLANGCI_LINT_VERSION='v1.55.2' |
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.
Needed for compatibility w/ Go 1.21
31ea777
to
9d82e31
Compare
9379260
to
ea888cd
Compare
238434e
to
2006920
Compare
list-type: denylist | ||
include-go-root: true | ||
# A list of packages for the list type specified. | ||
# Default: [] | ||
packages: | ||
- net/rpc | ||
# A list of packages for the list type specified. | ||
# Specify an error message to output when a denied package is used. | ||
# Default: [] | ||
packages-with-error-message: | ||
- net/rpc: "only use forked copy in github.com/hashicorp/consul-net-rpc/net/rpc" | ||
- github.com/golang/protobuf: "only use google.golang.org/protobuf" | ||
rules: | ||
main: | ||
listMode: lax | ||
deny: | ||
- pkg: net/rpc | ||
desc: "only use forked copy in github.com/hashicorp/consul-net-rpc/net/rpc" | ||
- pkg: github.com/golang/protobuf | ||
desc: "only use google.golang.org/protobuf" |
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.
See golangci/golangci-lint#3906. Some other projects disabled this linter or replaced w/ another tool like Semgrep in response, but since we use custom rules and don't yet use Semgrep, I tried to adapt ours in-place.
d25fe6c
to
eb9e190
Compare
eb9e190
to
f924a05
Compare
f924a05
to
cdcac95
Compare
cdcac95
to
01ca961
Compare
@@ -0,0 +1,3 @@ | |||
```release-note:improvement | |||
Upgrade to use Go 1.21.6. |
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.
@curtbushko FYI for K8s + DP bumps, we got 1.21.6 earlier this week
For our submodules and other places we choose to test against previous Go versions, detect this version automatically from the current one rather than hard-coding it.
go-test-api-1-19: | ||
go-test-api-backwards-compatibility: | ||
name: go-test-api-${{ needs.get-go-version.outputs.go-version-previous }} |
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.
@@ -446,17 +449,19 @@ jobs: | |||
consul-license: ${{secrets.CONSUL_LICENSE}} | |||
datadog-api-key: "${{ !endsWith(github.repository, '-enterprise') && secrets.DATADOG_API_KEY || '' }}" | |||
|
|||
go-test-sdk-1-19: | |||
go-test-sdk-backwards-compatibility: |
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 love this. No more twiddling versions whenever we update the go major version.
@@ -138,6 +138,7 @@ func destinationRulesByPort(allPorts []string, destinationRules []*pbauth.Destin | |||
return out | |||
} | |||
|
|||
//nolint:unparam |
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.
Just curious but why was this necessary. AFAICT allPorts and dr are both used in the function.
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.
Or is the linter even smarter and figuring out that one of the two is always nil based on other code.
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 believe the latter - that was my best explanation for why it suddenly started failing on the linter upgrade (same question you asked led me to notice the same)
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.
Looks great!
Description
Upgrade Go to 1.21.
This is needed in advance of the EOL of Go 1.20 ~Feb. 1. The ideal timing is before our next patch release scheduled for the end of January.
A follow-up change will introduce
toolchain
directives to pin to the latest version; the hope is to use a root-levelgo.work
to accomplish this, which requires some additional changes to support. See #20058 for more context.That PR should set this change up to cleanly backport and sync without intervention 🤞🏻
Testing & Reproduction steps
Tests continue to pass, and Go 1.21 can be observed as the output of
get-go-version
.PR Checklist