modernize sources #63

sivchari · 2025-04-12T08:33:20Z

copy #58

I executed modernize analyzer to this project.
This analyzer makes sources simplified.

sivchari · 2025-04-12T08:33:37Z

/kind cleanup

JoelSpeed

Do we need to upgrade to golangci-lint v2 to be able to use modernize? Are we going to regress if we merge this now without enforcing it within the configuration?

go.mod

everettraven · 2025-04-14T12:11:55Z

@sivchari thanks for creating this PR, but what gains do we get from doing this?

How are we going to enforce that new changes adhere to what the modernize analyzer would expect?

While it looks like there is a single change that may produce some minor performance benefits (i.e replacing Split in "for range strings.Split(...)" by go1.24's more efficient SplitSeq;), does the syntactic sugar of interface{} --> any (which seems like the largest change in the PR) actually do anything for us than make us more "modern"?

sivchari · 2025-04-14T13:00:22Z

Hi, @everettraven

The motivation that I want to add modernize analyzer is to reduce reviewing pr about fixing tiny change.
If we introduce this linter, I believe we won't review such as replace interface{} with any.
In addition, the modernize is maintained by Go team, so we can forward to latest syntax.

@JoelSpeed

Do we need to upgrade to golangci-lint v2 to be able to use modernize? Are we going to regress if we merge this now without enforcing it within the configuration?

No, it's not necessary to migrate v2 since the linter is independent from golangci-lint.

JoelSpeed · 2025-04-24T11:01:30Z

go.mod

@@ -209,3 +210,5 @@ require (
 	mvdan.cc/gofumpt v0.7.0 // indirect
 	mvdan.cc/unparam v0.0.0-20250301125049-0df0534333a4 // indirect
 )
+
+tool golang.org/x/tools/gopls/internal/analysis/modernize/cmd/modernize


Should we create a tools directory and module to include our tooling and keep the top level separate? We can move golangci-lint there too

golang/go#73279 describes that modernize shouldn't be contained as dependency, then they recommended to just run by using go run. So I use tool directive for golangci-lint not modernize.
And it's not possible to run golangci-lint located in tools directory to root directory, so I added tools directory to root go.mod

JoelSpeed · 2025-04-24T11:02:13Z

.github/workflows/lint.yaml

+      - name: modernize
+        run: |
+          if [ -n "$(go tool modernize -diff ./...)" ]; then
+            echo "The changes can't be modernized. Please run `go tool modernize -fix ./...`"


Please add a Makefile target make modernize that runs this

JoelSpeed · 2025-04-29T12:53:35Z

Should we be concerned about this rule from modernize?

replacing omitempty by omitzero on structs, added in go1.24;

Since omitempty is important in Kube APIs, I wonder if this will affect us.

I see that the modernize isn't complaining about test data at the moment, so that's good 🤔

JoelSpeed · 2025-04-29T12:59:46Z

Makefile

 .PHONY: lint
 lint: ## Run golangci-lint over the codebase.
-	${GOLANGCI_LINT} run ./... --timeout 5m -v
+	go tool golangci-lint run ./... --timeout 5m -v

 .PHONY: lint-fix
 lint-fix: ## Run golangci-lint over the codebase and run auto-fixers if supported by the linter
-	${GOLANGCI_LINT} run ./... --timeout 5m -v --fix
+	go tool golangci-lint run ./../... --timeout 5m -v --fix


As an end user, I want to be able to run all linting tools with one target. So I'd expect make lint to run both modernize and golangci-lint.

Perhaps we need to adjust this so we have something like below, WDYT

lint: modernize golangci-lint lint-fix: modernize-fix golangci-lint-fix golangci-lint: go tool golangci-lint run ./../... --timeout 5m -v

sivchari · 2025-04-29T14:23:24Z

This PR will be merged after #72 is merged.

Signed-off-by: sivchari <shibuuuu5@gmail.com>

.github/workflows/lint.yaml

JoelSpeed · 2025-04-30T08:50:06Z

Makefile

+
+.PHONY: modernize
+modernize: ## Run modernize on the codebase.
+	go run golang.org/x/tools/gopls/internal/analysis/modernize/cmd/modernize@v${MODERNIZE_VERSION} -diff ./...


Does it not need to be added to the tools module? Like the tool directive for golangci-lint?

Signed-off-by: sivchari <shibuuuu5@gmail.com>

sivchari · 2025-04-30T13:46:42Z

tools/go.mod

@@ -74,12 +77,12 @@ require (
 	github.com/golangci/dupl v0.0.0-20250308024227-f665c8d69b32 // indirect
 	github.com/golangci/go-printf-func-name v0.1.0 // indirect
 	github.com/golangci/gofmt v0.0.0-20250106114630-d62b90e6713d // indirect
-	github.com/golangci/golangci-lint/v2 v2.1.5 // indirect
+	github.com/golangci/golangci-lint/v2 v2.0.2 // indirect


After v2.1.0, golangci-lint use golang.org/x/tools v0.32.0, but the modernize depending on the version must be failed since the distribution is broken ref. So I downgrade the golangci-lint from v2.1.5 to v2.0.2. The addition between 2 versions is almost about new feature or upgrading modules, so this downgrade doesn't happen critical changes.

JoelSpeed · 2025-04-30T16:36:26Z

/approve
/lgtm

Thanks @sivchari

k8s-ci-robot · 2025-04-30T16:36:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed, sivchari

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:

~~OWNERS~~ [JoelSpeed]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 12, 2025

k8s-ci-robot requested review from everettraven and jpbetz April 12, 2025 08:33

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 12, 2025

k8s-ci-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Apr 12, 2025

JoelSpeed reviewed Apr 14, 2025

View reviewed changes

go.mod Show resolved Hide resolved

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 14, 2025

sivchari force-pushed the modernize-project branch from 278f09c to 54b14a6 Compare April 18, 2025 13:53

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 21, 2025

sivchari force-pushed the modernize-project branch from 54b14a6 to cb4e912 Compare April 22, 2025 03:18

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 22, 2025

JoelSpeed reviewed Apr 24, 2025

View reviewed changes

sivchari force-pushed the modernize-project branch 3 times, most recently from 00a5538 to e44a106 Compare April 25, 2025 16:56

JoelSpeed reviewed Apr 29, 2025

View reviewed changes

JoelSpeed mentioned this pull request Apr 29, 2025

Move golangci-lint to a tools directory and remove vendor #72

Merged

sivchari added 4 commits April 30, 2025 02:25

add modernize task

4786699

Signed-off-by: sivchari <shibuuuu5@gmail.com>

upgrade Go1.24.2

c9deac2

Signed-off-by: sivchari <shibuuuu5@gmail.com>

modify workflow

f4b8d90

Signed-off-by: sivchari <shibuuuu5@gmail.com>

run modernize

38ed5cb

Signed-off-by: sivchari <shibuuuu5@gmail.com>

sivchari force-pushed the modernize-project branch from b41c987 to 38ed5cb Compare April 29, 2025 17:27

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 29, 2025

JoelSpeed reviewed Apr 30, 2025

View reviewed changes

sivchari force-pushed the modernize-project branch from df35c27 to efdc833 Compare April 30, 2025 13:39

sivchari added 2 commits April 30, 2025 22:40

unify workflow into 1

9ff6cb3

Signed-off-by: sivchari <shibuuuu5@gmail.com>

adjust dependency and add modernize to tool directive

a45bde9

Signed-off-by: sivchari <shibuuuu5@gmail.com>

sivchari force-pushed the modernize-project branch from efdc833 to a45bde9 Compare April 30, 2025 13:40

sivchari commented Apr 30, 2025

View reviewed changes

k8s-ci-robot assigned JoelSpeed Apr 30, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 30, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 30, 2025

k8s-ci-robot merged commit 0c6bb03 into kubernetes-sigs:main Apr 30, 2025
4 checks passed

modernize sources #63

modernize sources #63

Uh oh!

Conversation

sivchari commented Apr 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sivchari commented Apr 12, 2025

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

everettraven commented Apr 14, 2025

Uh oh!

sivchari commented Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoelSpeed Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

sivchari Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoelSpeed Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

JoelSpeed commented Apr 29, 2025

Uh oh!

JoelSpeed Apr 29, 2025

Choose a reason for hiding this comment

Uh oh!

sivchari Apr 29, 2025

Choose a reason for hiding this comment

Uh oh!

sivchari commented Apr 29, 2025

Uh oh!

Uh oh!

JoelSpeed Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

sivchari Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

JoelSpeed commented Apr 30, 2025

Uh oh!

k8s-ci-robot commented Apr 30, 2025

Uh oh!

Uh oh!

Uh oh!

sivchari commented Apr 12, 2025 •

edited

Loading

sivchari commented Apr 14, 2025 •

edited

Loading

sivchari Apr 25, 2025 •

edited

Loading