*: fix ineffassign lint #16605

fuweid · 2023-09-18T04:40:15Z

PASSES="ineffassign" ./scripts/test.sh
Running with --race
Starting at: Mon Sep 18 12:40:24 HKT 2023

'ineffassign' started at Mon Sep 18 12:40:24 HKT 2023
% (cd tools/mod && 'go' 'install' 'github.com/gordonklaus/ineffassign')
% (cd api && '/home/fuwei/go/bin/ineffassign' './...')
% (cd tools/mod && 'go' 'install' 'github.com/gordonklaus/ineffassign')
% (cd pkg && '/home/fuwei/go/bin/ineffassign' './...')
% (cd tools/mod && 'go' 'install' 'github.com/gordonklaus/ineffassign')
% (cd client/pkg && '/home/fuwei/go/bin/ineffassign' './...')
% (cd tools/mod && 'go' 'install' 'github.com/gordonklaus/ineffassign')
% (cd client/internal/v2 && '/home/fuwei/go/bin/ineffassign' './...')
% (cd tools/mod && 'go' 'install' 'github.com/gordonklaus/ineffassign')
% (cd client/v3 && '/home/fuwei/go/bin/ineffassign' './...')
% (cd tools/mod && 'go' 'install' 'github.com/gordonklaus/ineffassign')
% (cd server && '/home/fuwei/go/bin/ineffassign' './...')
% (cd tools/mod && 'go' 'install' 'github.com/gordonklaus/ineffassign')
% (cd etcdutl && '/home/fuwei/go/bin/ineffassign' './...')
% (cd tools/mod && 'go' 'install' 'github.com/gordonklaus/ineffassign')
% (cd etcdctl && '/home/fuwei/go/bin/ineffassign' './...')
% (cd tools/mod && 'go' 'install' 'github.com/gordonklaus/ineffassign')
% (cd tests && '/home/fuwei/go/bin/ineffassign' './...')
% (cd tools/mod && 'go' 'install' 'github.com/gordonklaus/ineffassign')
% '/home/fuwei/go/bin/ineffassign' './...'
'ineffassign' PASSED and completed at Mon Sep 18 12:40:25 HKT 2023
SUCCESS

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

fuweid · 2023-09-18T09:04:05Z

    logger.go:130: 2023-09-18T04:51:30.214Z	WARN	m0.raft	23c867d67cc4267b stepped down to follower since quorum is not active	{"member": "m0"}
    logger.go:130: 2023-09-18T04:51:30.215Z	INFO	m0.raft	23c867d67cc4267b became follower at term 3	{"member": "m0"}
    logger.go:130: 2023-09-18T04:51:30.215Z	INFO	m0.raft	raft.node: 23c867d67cc4267b lost leader 23c867d67cc4267b at term 3	{"member": "m0"}
    logger.go:130: 2023-09-18T04:51:30.551Z	WARN	m1	request stats	{"member": "m1", "start time": "2023-09-18T04:51:30.013Z", "time spent": "537.193567ms", "remote": "@", "response type": "/etcdserverpb.Lease/LeaseTimeToLive", "request count": -1, "request size": -1, "response count": -1, "response size": -1, "request content": ""}
    lease_test.go:180: 
        	Error Trace:	/__w/etcd/etcd/tests/common/lease_test.go:180
        	            				/__w/etcd/etcd/tests/framework/testutils/execute.go:38
        	            				/__t/go/1.21.1/x64/src/runtime/asm_amd64.s:1650
        	Error:      	"2" is not greater than "9223372036"
        	Test:       	TestLeaseGrantKeepAliveOnce/PeerAutoTLS

The leader has been changed during invoking LeaseTimeToLive. And the new leader didn't Promote in time.
The expiredTime is still zero. So the server return time.Duration(math.MaxInt64).Seconds(). I will file to pr to deflake with checking no leader change during the test.

serathius · 2023-09-18T16:22:44Z

scripts/test.sh

 function ineffassign_pass {
-  run_for_modules generic_checker ineffassign_per_package


Shouldn't ineffassign be enabled as part of golangci? Check tools/.golangci.yaml

Thanks for the reminder! The tools/.golangci.yaml already has the ineffassign. But the golangci-lint doesn't support multiple modules so... currently the make verify-lint doesn't work.

# at root (no error) ➜ etcd git:(main) golangci-lint run --config ./tools/.golangci.yaml -v INFO [config_reader] Used config file tools/.golangci.yaml INFO [lintersdb] Active 7 linters: [goimports ineffassign revive staticcheck stylecheck unconvert unused] INFO [loader] Go packages loading at mode 575 (types_sizes|compiled_files|deps|exports_file|files|imports|name) took 412.865392ms INFO [runner/filename_unadjuster] Pre-built 0 adjustments in 6.088202ms INFO [linters_context/goanalysis] analyzers took 0s with no stages INFO [runner] Issues before processing: 10, after processing: 0 INFO [runner] Processors filtering stat (out/in): exclude-rules: 0/10, identifier_marker: 10/10, skip_dirs: 10/10, filename_unadjuster: 10/10, path_prettifier: 10/10, skip_files: 10/10, autogenerated_exclude: 10/10, exclude: 10/10, cgo: 10/10 INFO [runner] processing took 1.314322ms with stages: path_prettifier: 465.208µs, exclude-rules: 277.505µs, autogenerated_exclude: 251.005µs, identifier_marker: 221.403µs, skip_dirs: 70.401µs, skip_files: 19µs, cgo: 3µs, filename_unadjuster: 1.4µs, max_same_issues: 1.4µs, nolint: 900ns, fixer: 900ns, diff: 400ns, source_code: 400ns, max_from_linter: 300ns, max_per_file_from_linter: 200ns, uniq_by_line: 200ns, path_prefixer: 200ns, exclude: 200ns, severity-rules: 100ns, path_shortener: 100ns, sort_results: 100ns INFO [runner] linters took 154.52938ms with stages: goanalysis_metalinter: 152.939553ms INFO File cache stats: 0 entries of total size 0B INFO Memory: 7 samples, avg is 33.4MB, max is 50.9MB INFO Execution took 588.932231ms # cd ./server ➜ server git:(main) golangci-lint run --config ../tools/.golangci.yaml lease/lease_queue.go:25:6: exported: type name will be used as lease.LeaseWithTime by other packages, and that stutters; consider calling this WithTime (revive) type LeaseWithTime struct { ^ lease/lessor.go:823:39: unused-parameter: parameter 'dr' seems to be unused, consider removing or renaming it as _ (revive) func (fl *FakeLessor) SetRangeDeleter(dr RangeDeleter) {} ^ lease/lease.go:119:6: exported: type name will be used as lease.LeaseItem by other packages, and that stutters; consider calling this Item (revive) type LeaseItem struct { ^ lease/lease_queue.go:31:6: exported: type name will be used as lease.LeaseQueue by other packages, and that stutters; consider calling this Queue (revive) type LeaseQueue []*LeaseWithTime

May I fix the lint one by one and then fix the make verify-lint after that? The small changeset is easy to be reviewed.

Oh, that's unfortunate. Fixing golangci sounds good to me. Thanks for investigating this!

After fixing all the lint's issues, I will fix golangci-lint issue.

Can you file an issue about golangci being broken so we can track fixes to lint issues in one place?

Filed #16610 .

ahrtr · 2023-09-19T08:58:39Z

@fuweid I just merged a huge PR #16595, could you please rebase this PR although github doesn't show conflict? thx

serathius · 2023-09-19T09:49:05Z

scripts/test.sh

 function ineffassign_pass {
-  run_for_modules generic_checker ineffassign_per_package
+  run_for_modules generic_checker run_go_tool "github.com/gordonklaus/ineffassign"


What's your plan for ineffassign_pass if this will be covered by make verify-lint? Should we maybe remove it?

# TODO: https://github.com/etcd-io/etcd/issues/16610 # # The golangci-lint doesn't verify sub modules. Before #16610 fixed, verify-lint # still depends on legacy {ineffassign,nakedret,unparam,...}_pass. These X_pass # will be removed when the golangci-lint covers all the sub modules. .PHONY: verify-lint verify-lint: verify-ineffassign golangci-lint run --config tools/.golangci.yaml

I add new recipe verify-ineffassign and verify-lint depends on it. So, it can cover the new change.
when all the lint issues are covered by _pass, we can switch to golangci-lint. After that, all the legacy lint_pass can be removed. Does it make senses?

Makes sense, should be good to prevent regressions until we fix golangci.

when all the lint issues are covered by _pass, we can switch to golangci-lint. After that, all the legacy lint_pass can be removed.

I assume there is a ticket to track this.

Yes. #16610 is tracking this

Signed-off-by: Wei Fu <fuweid89@gmail.com>

fuweid · 2023-09-20T08:56:07Z

ping @ahrtr @serathius @jmhbnz ~

ahrtr

LGTM

thx. @fuweid

ahrtr · 2023-09-20T09:44:24Z

scripts/test.sh

 function ineffassign_pass {
-  run_for_modules generic_checker ineffassign_per_package
+  run_for_modules generic_checker run_go_tool "github.com/gordonklaus/ineffassign"


when all the lint issues are covered by _pass, we can switch to golangci-lint. After that, all the legacy lint_pass can be removed.

I assume there is a ticket to track this.

fuweid force-pushed the fix-ineffassign branch from 62e5ffb to 89959d5 Compare September 18, 2023 09:05

fuweid marked this pull request as ready for review September 18, 2023 09:45

fuweid closed this Sep 18, 2023

fuweid reopened this Sep 18, 2023

serathius reviewed Sep 18, 2023

View reviewed changes

fuweid force-pushed the fix-ineffassign branch from 89959d5 to ce48c87 Compare September 19, 2023 09:00

serathius mentioned this pull request Sep 19, 2023

[lint] make verify-lint doesn't verify {api,server,client,...} modules #16610

Closed

7 tasks

serathius reviewed Sep 19, 2023

View reviewed changes

fuweid force-pushed the fix-ineffassign branch from ce48c87 to 32fc09b Compare September 19, 2023 10:06

*: fix ineffassign lint

df86cad

Signed-off-by: Wei Fu <fuweid89@gmail.com>

fuweid force-pushed the fix-ineffassign branch from 32fc09b to df86cad Compare September 19, 2023 14:21

serathius approved these changes Sep 20, 2023

View reviewed changes

ahrtr approved these changes Sep 20, 2023

View reviewed changes

ahrtr merged commit 021edb0 into etcd-io:main Sep 20, 2023

fuweid deleted the fix-ineffassign branch September 20, 2023 09:58

jmhbnz mentioned this pull request Sep 25, 2023

Nominating @fuweid as etcd reviewer #16650

Closed

9 tasks

fuweid mentioned this pull request Oct 8, 2023

{markdown_you,govet_shadow,unparam,staticcheck,..}_pass fail in main branch #15549

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: fix ineffassign lint #16605

*: fix ineffassign lint #16605

fuweid commented Sep 18, 2023 •

edited

Loading

fuweid commented Sep 18, 2023

serathius Sep 18, 2023

fuweid Sep 19, 2023 •

edited

Loading

serathius Sep 19, 2023

fuweid Sep 19, 2023

serathius Sep 19, 2023

fuweid Sep 19, 2023

ahrtr commented Sep 19, 2023

serathius Sep 19, 2023

fuweid Sep 19, 2023

serathius Sep 19, 2023 •

edited

Loading

ahrtr Sep 20, 2023

fuweid Sep 20, 2023

ahrtr Sep 20, 2023

fuweid commented Sep 20, 2023

ahrtr left a comment

ahrtr Sep 20, 2023

		function ineffassign_pass {
		run_for_modules generic_checker ineffassign_per_package

*: fix ineffassign lint #16605

*: fix ineffassign lint #16605

Conversation

fuweid commented Sep 18, 2023 • edited Loading

fuweid commented Sep 18, 2023

Choose a reason for hiding this comment

fuweid Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahrtr commented Sep 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serathius Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuweid commented Sep 20, 2023

ahrtr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuweid commented Sep 18, 2023 •

edited

Loading

fuweid Sep 19, 2023 •

edited

Loading

serathius Sep 19, 2023 •

edited

Loading