-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
all: use built-in min/max #16661
all: use built-in min/max #16661
Conversation
Since go.mod is 1.21, we can simplify code by using these built-in functions. Signed-off-by: Jes Cok <xigua67damn@gmail.com>
https://github.com/golangci/golangci-lint/blob/dafe1469eb4b35ca04af0b9725bed7d69f3c5918/go.mod#L3 |
Looks like golangci-lint supports go 1.21 fully from version v1.54.1, refer golangci/golangci-lint#3933. We need to update our golangci-lint version here https://github.com/etcd-io/etcd/blob/main/.github/workflows/static-analysis.yaml#L18. Can you please add a second commit to this pr @gocurr to bump that? I believe it will resolve the issue you're seeing and allow us to merge this. |
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 - Thanks for cleaning this up @gocurr 👍🏻
Users won't be able to build etcd (main) using golang 1.20.x any more if we merge this PR. Although we suggest to build the main branch using go 1.21, technically we should NOT prevent users from building main using go 1.20.x for now. So suggest on hold this PR until go 1.20.x is out of support. |
@@ -15,7 +15,7 @@ jobs: | |||
- name: golangci-lint | |||
uses: golangci/golangci-lint-action@3a919529898de77ec3da873e3063ca4b10e7f5cc # v3.7.0 | |||
with: | |||
version: v1.53.3 | |||
version: v1.54.1 |
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.
This can be merged separately.
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 think it should be kept to prevent new contributors
from using the new features of 1.21.
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.
Explicitly mark this as request changes.
Seems awkward that you have to keep explaining to new contributors
|
Not sure I understand the motivation. We have updated the go version in go.mod to 1.21. This is a clear suggestion that libraries we publish are go1.21 compatible. As for 1.20 backward compatibility, I would agree it's important, however only for There is no reason to require go1.20 compatibility from |
Suggestion != mandatory It forces the applications/users, which depend on/use etcd, to bump their golang version to 1.21. Overall it isn't a big deal, since 1.20 will be out of support about 4~5 months later. But let's try not to bring bad user experience. |
For It's a development branch with no guarantee around stability. If there are exciting new features in golang these might come with a degree of complexity so the sooner we can get those features into our CI system and start observing their behavior over time the better. This pr is a trivial example in terms of new feature, but I'm glad it's forced this discussion. I acknowledge that updating golang minor version locally can be annoying, but as developers working on a development branch I think it's to be expected from time to time. |
True, it's a suggestion because not every golang version introduces backward compatible changes which would make it mandatory. |
The principle is simple, let's try not technically prevent users or user applications from using a go version (1.20.x) which is still officially supported by golang team. The change in this PR isn't a big deal, I suggest to on-hold this PR until 1.20.x is out of support. Of course, just as I mentioned above #16661 (comment), it isn't a big deal. I will not insist on this if majority maintainers think we should do it. |
Asked @liggitt, he said that Kubernetes doesn't even do this. If you want to propose this, then we should discuss this. |
Still, I agree that just the |
If you need to keep compatibility with older go versions, a possibility is to put the use of new language features in a go version-gated file and have an alternate implementation in a second file. Of course, that often negates the convenience of using the stdlib version in the first place. |
thx for the suggestion. The change in this PR isn't worth of introducing additional complexity, nor breaking golang compatibility. |
Abandoned this for 1.20 compatibility. |
Since go.mod is 1.21, we can simplify code by using these built-in functions.