Skip to content
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

*: fix goimports linter #16628

Merged
merged 2 commits into from
Sep 22, 2023
Merged

*: fix goimports linter #16628

merged 2 commits into from
Sep 22, 2023

Conversation

fuweid
Copy link
Member

@fuweid fuweid commented Sep 21, 2023

Besides goimports --fix, also update the import groups manually. The layout is based on the goimports setting:

  • first group: golib
  • second group: 3rd party libs
  • last group: packages has go.etcd.io prefix

And remove goimports(_fix)_pass and add lint_fix_pass to fix the lint issues.

Parts of #16610

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

Signed-off-by: Wei Fu <fuweid89@gmail.com>
Signed-off-by: Wei Fu <fuweid89@gmail.com>
@fuweid fuweid marked this pull request as ready for review September 21, 2023 14:09
verify-govet-shadow
fix: fix-goimports fix-bom fix-lint fix-yamllint
fix: fix-bom fix-lint fix-yamllint
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that you added lint_fix_pass to fix goimport issue, but I do not see where to verify the goimport issue after removing the verify-goimport? Did I miss anything?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was surprised when I prepared this patch. The problem is that the old goimports_pass was using run go list --f "{{with \$d:=.}}{{range .TestGoFiles}}{{\$d.Dir}}/{{.}}{{\"\n\"}}{{end}}{{end}}" ./... to list test code. It can only list out the test files using the package name as same as the folder name. For example, the backend package has several test files but the go list only shows two files.

➜  backend git:(main) pwd
/home/fuwei/go/src/go.etcd.io/etcd/server/storage/backend

➜  backend git:(main) ls -al *_test.go
-rw-r--r-- 1 fuwei fuwei 1440 Sep 22 00:04 backend_bench_test.go
-rw-r--r-- 1 fuwei fuwei 8741 Sep 22 00:04 backend_test.go
-rw-r--r-- 1 fuwei fuwei 4838 Sep 21 20:16 batch_tx_test.go
-rw-r--r-- 1 fuwei fuwei  845 Sep  9 21:55 export_test.go
-rw-r--r-- 1 fuwei fuwei 3696 Sep 22 00:04 hooks_test.go
-rw-r--r-- 1 fuwei fuwei 2275 Sep 17 10:54 tx_buffer_test.go
-rw-r--r-- 1 fuwei fuwei 3010 Sep 18 17:10 verify_test.go

➜  backend git:(main) go list  --f "{{with \$d:=.}}{{range .TestGoFiles}}{{\$d.Dir}}/{{.}}{{\"\n\"}}{{end}}{{end}}" ./...
/home/fuwei/go/src/go.etcd.io/etcd/server/storage/backend/export_test.go
/home/fuwei/go/src/go.etcd.io/etcd/server/storage/backend/tx_buffer_test.go

There are several test files using backend_test instead of backend. So, after switch to golangci-lint, we can see several errors as the following result described.

'lint' started at Fri Sep 22 00:01:30 HKT 2023
% (cd api && 'golangci-lint' 'run' '--config' '/home/fuwei/go/src/go.etcd.io/etcd/tools/.golangci.yaml' '--fix' './...')
% (cd pkg && 'golangci-lint' 'run' '--config' '/home/fuwei/go/src/go.etcd.io/etcd/tools/.golangci.yaml' '--fix' './...')
% (cd client/pkg && 'golangci-lint' 'run' '--config' '/home/fuwei/go/src/go.etcd.io/etcd/tools/.golangci.yaml' '--fix' './...')
% (cd client/internal/v2 && 'golangci-lint' 'run' '--config' '/home/fuwei/go/src/go.etcd.io/etcd/tools/.golangci.yaml' '--fix' './...')
% (cd client/v3 && 'golangci-lint' 'run' '--config' '/home/fuwei/go/src/go.etcd.io/etcd/tools/.golangci.yaml' '--fix' './...')
% (cd server && 'golangci-lint' 'run' '--config' '/home/fuwei/go/src/go.etcd.io/etcd/tools/.golangci.yaml' '--fix' './...')
% (cd etcdutl && 'golangci-lint' 'run' '--config' '/home/fuwei/go/src/go.etcd.io/etcd/tools/.golangci.yaml' '--fix' './...')
% (cd etcdctl && 'golangci-lint' 'run' '--config' '/home/fuwei/go/src/go.etcd.io/etcd/tools/.golangci.yaml' '--fix' './...')
% (cd tests && 'golangci-lint' 'run' '--config' '/home/fuwei/go/src/go.etcd.io/etcd/tools/.golangci.yaml' '--fix' './...')
% 'golangci-lint' 'run' '--config' '/home/fuwei/go/src/go.etcd.io/etcd/tools/.golangci.yaml' '--fix' './...'
'lint' PASSED and completed at Fri Sep 22 00:01:33 HKT 2023
SUCCESS
➜  etcd git:(main) ✗ gst
On branch main
Your branch is up to date with 'upstream/main'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
        modified:   scripts/test.sh
        modified:   server/storage/backend/backend_bench_test.go
        modified:   server/storage/backend/backend_test.go
        modified:   server/storage/backend/hooks_test.go
        modified:   server/storage/datadir/datadir_test.go
        modified:   tests/integration/clientv3/concurrency/session_test.go
        modified:   tests/integration/clientv3/connectivity/black_hole_test.go
        modified:   tests/integration/clientv3/connectivity/dial_test.go
        modified:   tests/integration/clientv3/connectivity/network_partition_test.go
        modified:   tests/integration/clientv3/lease/leasing_test.go
        modified:   tests/integration/clientv3/snapshot/v3_snapshot_test.go
        modified:   tests/integration/embed/embed_test.go
        modified:   tests/integration/revision_test.go
        modified:   tests/integration/snapshot/v3_snapshot_test.go
        modified:   tests/integration/v2store/store_tag_test.go
        modified:   tests/integration/v2store/store_test.go
        modified:   tools/.golangci.yaml

Most of them were modified manually. If it's unnecessary, I can rollback it.
I don't have knowledge about the X_test package naming. Maybe we can change them to X in the future.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My question is... if there is any not well organised import entries, is the github workflow able to detect it in future?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/etcd-io/etcd/pull/16628/files#diff-fe909f04d646d2ac68018e03d59a12d2b2ed8201f3618a9547c35d27ab255e25

Added the goimports in the golangci-linter. The CI can verify it.

The linter can detect the issue that both the third-party pkgs and go.etcd.io pkgs are in the same group.
The case is descripted by the following code.

imports (
     "testing"

     "third-party/pkg/xxxx"
     "go.etcd.io/etcd/xxx"
)

But it can't detect the issue that there is multiple go.etcd.io groups. That's why I have to modify the groups manually.

imports (
     "testing"

     "third-party/pkg/xxxx"

      "go.etcd.io/etcd/xxx"

      "go.etcd.io/etcd/xxy"

      "third-party/pkg/xxxy"
)

Not sure it answers your question.

@ahrtr
Copy link
Member

ahrtr commented Sep 21, 2023

Most of the changes are simple & straightforward changes. Ping @jmhbnz and @chaochn47 to double check. thx

Copy link
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - Great work @fuweid

Very supportive of moving away from the custom bash we have to maintain to instead rely on standard features of golangci-lint!

@@ -17,7 +17,7 @@ linters:
# - deadcode
# - structcheck
# - varcheck
# - goimports # TODO: enable by #16610
- goimports
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, I missed this

Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

thx @fuweid

@ahrtr ahrtr merged commit e1ebc26 into etcd-io:main Sep 22, 2023
27 checks passed
@jmhbnz jmhbnz mentioned this pull request Sep 25, 2023
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants