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.

Sign up for GitHub

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 ineffassign lint #16605

Merged
merged 1 commit into from
Sep 20, 2023
Merged

*: fix ineffassign lint #16605

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,13 @@ fix-bom:
verify-dep:
PASSES="dep" ./scripts/test.sh

# 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-lint: verify-ineffassign
golangci-lint run --config tools/.golangci.yaml

.PHONY: fix-lint
Expand Down Expand Up @@ -145,6 +150,10 @@ verify-yamllint:
verify-govet-shadow:
PASSES="govet_shadow" ./scripts/test.sh

.PHONY: verify-ineffassign
verify-ineffassign:
PASSES="ineffassign" ./scripts/test.sh

YAMLFMT_VERSION = $(shell cd tools/mod && go list -m -f '{{.Version}}' github.com/google/yamlfmt)

.PHONY: fix-yamllint
Expand Down
11 changes: 1 addition & 10 deletions scripts/test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,17 +442,8 @@ function unconvert_pass {
run_for_modules generic_checker run_go_tool "github.com/mdempsky/unconvert" unconvert -v
}

function ineffassign_per_package {
# bash 3.x compatible replacement of: mapfile -t gofiles < <(go_srcs_in_module)
local gofiles=()
while IFS= read -r line; do gofiles+=("$line"); done < <(go_srcs_in_module)

# TODO: ineffassign should work with package instead of files
run_go_tool github.com/gordonklaus/ineffassign "${gofiles[@]}"
}

function ineffassign_pass {
run_for_modules generic_checker ineffassign_per_package
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@fuweid fuweid Sep 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed #16610 .

run_for_modules generic_checker run_go_tool "github.com/gordonklaus/ineffassign"
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

# 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?

Copy link
Member

@serathius serathius Sep 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. #16610 is tracking this

Copy link
Member

Choose a reason for hiding this comment

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

thx

}

function nakedret_pass {
Expand Down
4 changes: 0 additions & 4 deletions server/lease/lessor_bench_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,6 @@ func benchmarkLessorGrant(benchSize int, b *testing.B) {
b.StopTimer()
if tearDown != nil {
tearDown()
tearDown = nil
}
le, tearDown = setUp(b)
b.StartTimer()
Expand All @@ -117,7 +116,6 @@ func benchmarkLessorRevoke(benchSize int, b *testing.B) {
b.StopTimer()
if tearDown != nil {
tearDown()
tearDown = nil
}
le, tearDown = setUp(b)
for j := 1; j <= benchSize; j++ {
Expand Down Expand Up @@ -148,7 +146,6 @@ func benchmarkLessorRenew(benchSize int, b *testing.B) {
b.StopTimer()
if tearDown != nil {
tearDown()
tearDown = nil
}
le, tearDown = setUp(b)
for j := 1; j <= benchSize; j++ {
Expand Down Expand Up @@ -181,7 +178,6 @@ func benchmarkLessorFindExpired(benchSize int, b *testing.B) {
b.StopTimer()
if tearDown != nil {
tearDown()
tearDown = nil
}
le, tearDown = setUp(b)
for j := 1; j <= benchSize; j++ {
Expand Down
2 changes: 1 addition & 1 deletion tests/common/compact_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ func TestCompact(t *testing.T) {
t.Fatalf("compactTest: Compact error (%v)", err)
}

get, err = cc.Get(ctx, "key", config.GetOptions{Revision: 3})
_, err = cc.Get(ctx, "key", config.GetOptions{Revision: 3})
if err != nil {
if !strings.Contains(err.Error(), "required revision has been compacted") {
t.Fatalf("compactTest: Get compact key error (%v)", err)
Expand Down
2 changes: 2 additions & 0 deletions tests/e2e/corrupt_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,7 @@ func TestCompactHashCheckDetectCorruptionInterrupt(t *testing.T) {

t.Log("compaction started...")
_, err = cc.Compact(ctx, 5, config.CompactOption{})
require.NoError(t, err)

err = epc.Procs[slowCompactionNodeIndex].Close()
require.NoError(t, err)
Expand All @@ -333,6 +334,7 @@ func TestCompactHashCheckDetectCorruptionInterrupt(t *testing.T) {

t.Logf("restart proc %d to interrupt its compaction...", slowCompactionNodeIndex)
err = epc.Procs[slowCompactionNodeIndex].Restart(ctx)
require.NoError(t, err)

// Wait until the node finished compaction and the leader finished compaction hash check
_, err = epc.Procs[slowCompactionNodeIndex].Logs().ExpectWithContext(ctx, expect.ExpectedResponse{Value: "finished scheduled compaction"})
Expand Down
1 change: 1 addition & 0 deletions tests/e2e/ctl_v3_member_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ func memberListSerializableTest(cx ctlCtx) {
require.NoError(cx.t, err)

resp, err = getMemberList(cx, true)
require.NoError(cx.t, err)
require.Equal(cx.t, 2, len(resp.Members))
}

Expand Down
2 changes: 2 additions & 0 deletions tests/e2e/v3_curl_cluster_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ func testCurlV3ClusterOperations(cx ctlCtx) {
// update member
cx.t.Logf("Update peerURL from %q to %q for member %q", peerURL, updatedPeerURL, newMemberIDStr)
newMemberID, err := strconv.ParseUint(newMemberIDStr, 10, 64)
require.NoError(cx.t, err)

updateMemberReq, err := json.Marshal(&pb.MemberUpdateRequest{ID: newMemberID, PeerURLs: []string{updatedPeerURL}})
require.NoError(cx.t, err)

Expand Down
2 changes: 1 addition & 1 deletion tests/framework/e2e/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,7 +510,7 @@ func (cfg *EtcdProcessClusterConfig) EtcdServerProcessConfig(tb testing.TB, i in

name := fmt.Sprintf("%s-test-%d", testNameCleanRegex.ReplaceAllString(tb.Name(), ""), i)

dataDirPath := cfg.DataDirPath
var dataDirPath string
if cfg.DataDirPath == "" {
dataDirPath = tb.TempDir()
} else {
Expand Down
4 changes: 2 additions & 2 deletions tests/integration/v3_auth_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,12 +444,12 @@ func TestV3AuthWatchAndTokenExpire(t *testing.T) {
// The first watch gets a valid auth token through watcher.newWatcherGrpcStream()
// We should discard the first one by waiting TTL after the first watch.
wChan := c.Watch(ctx, "key", clientv3.WithRev(1))
watchResponse := <-wChan
<-wChan

time.Sleep(5 * time.Second)

wChan = c.Watch(ctx, "key", clientv3.WithRev(1))
watchResponse = <-wChan
watchResponse := <-wChan
testutil.AssertNil(t, watchResponse.Err())
}

Expand Down