fix test script and lint

[tessapham]

Fixes #16897 and test.sh script.

[jmhbnz]

Hey @tessapham - Thanks for raising this suggestion. Taking a look at both changes I'm not sure they are necessary.

The make verify command runs without incident for me in my local environment on main branch.

If running golangci-lint manually ensure you use the projects configuration file which is tools/.golangci.yaml.

Additionally the shell script convention for etcd us currently to use ${@} when referring to the collection of values and shellcheck does not complain about this for me. What was the motivation for changing this?

[tessapham]

Thanks @jmhbnz for your comment. Regarding the script, I got

./scripts/test.sh: line 671: @: unbound variable
make: *** [verify-gofmt] Error 1

when running make verify. Removing the curly braces solved this.

Looks like both ${@} and $@ are used in the script (e.g., https://github.com/etcd-io/etcd/blob/main/scripts/test.sh#L111) - is there any semantic difference between the two? $@ refers to the list of arguments passed into the script, so say for make verify-gofmt which is PASSES="gofmt" ./scripts/test.sh, we'll have:

run_pass "gofmt" "$@"
=> gofmt_pass <list of arguments>

Please correct me if I'm not getting this right.

The golangci-lint error I got was actually from make verify-lint, which does use the tools/.golangci.yaml config file. Maybe it doesn't complain in a different system, but the error makes sense:

FAIL: (code:1):
  % (cd server && golangci-lint run --config /Users/hpham111/git/etcd/tools/.golangci.yaml ./...)
etcdserver/api/v3discovery/discovery_test.go:266:9: indent-error-flow: if block ends with a return statement, so drop this else and outdent its block (revive)
	} else {
		fkv.t.Errorf("unexpected key: %s", key)
		return nil, fmt.Errorf("unexpected key: %s", key)
	}

[jmhbnz]

when running make verify. Removing the curly braces solved this.

I believe this is because you are running on a Mac locally? This error does not occur on Linux. We would need to validate that the change does not impact the behaviour of the script on all platforms before we merge.

The golangci-lint error I got was actually from make verify-lint, which does use the tools/.golangci.yaml config file. Maybe it doesn't complain in a different system, but the error makes sense:

I suspect this is because you are using a different version of golangci-lint to what the project uses: https://github.com/etcd-io/etcd/blob/main/.github/workflows/static-analysis.yaml#L18

I'm not opposed to fixing the lint error though we would normally do this at the same time we update golangci-lint and do it everywhere for the project in one commit.