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

lint: fix linter errors and update CI to require passing #4241

Merged
merged 20 commits into from
Aug 18, 2022

Conversation

cce
Copy link
Contributor

@cce cce commented Jul 7, 2022

Summary

I noticed some golangci-lint errors cropping up that should have been caught by our reviewdog "error" severity configuration (in .golangci.yml) but I think they were not blocking builds because we have reviewdog set to filter out issues based to just the lines added/modified.

This fixes the linter errors currently on master, and removes filtering from reviewdog to force failing CI if any new high-severity linter errors are encountered.

Warning-level severity linter output is still non-blocking and in .golangci-warnings.yml.

This also promotes a few of golangci-lint's default linters (nolintlint, gosimple, staticcheck, typecheck) that were in .golangci-warnings.yml to error level, these only had a couple of fixes required to get lint clean.

Test Plan

All checks should pass.

@cce cce added the Enhancement label Jul 7, 2022
@codecov
Copy link

codecov bot commented Jul 7, 2022

Codecov Report

Merging #4241 (b563b39) into master (c1b6a03) will increase coverage by 0.04%.
The diff coverage is 55.17%.

@@            Coverage Diff             @@
##           master    #4241      +/-   ##
==========================================
+ Coverage   55.13%   55.18%   +0.04%     
==========================================
  Files         397      397              
  Lines       50073    50076       +3     
==========================================
+ Hits        27607    27633      +26     
+ Misses      20170    20139      -31     
- Partials     2296     2304       +8     
Impacted Files Coverage Δ
cmd/goal/account.go 12.62% <0.00%> (ø)
crypto/merklesignature/const.go 33.33% <ø> (ø)
data/ledger.go 31.38% <0.00%> (ø)
ledger/accountdb.go 72.58% <ø> (ø)
ledger/acctonline.go 78.36% <ø> (-0.64%) ⬇️
network/wsNetwork.go 64.63% <ø> (-0.26%) ⬇️
util/sleep_linux_64.go 0.00% <0.00%> (ø)
protocol/codec_tester.go 10.61% <50.00%> (+10.61%) ⬆️
cmd/goal/application.go 12.75% <60.00%> (-0.04%) ⬇️
crypto/stateproof/coinGenerator.go 100.00% <100.00%> (ø)
... and 13 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

algorandskiy
algorandskiy previously approved these changes Jul 8, 2022
network/wsNetwork.go Outdated Show resolved Hide resolved
@cce cce requested a review from algojack August 16, 2022 15:51
protocol/codec_tester.go Outdated Show resolved Hide resolved
cmd/catchpointdump/net.go Outdated Show resolved Hide resolved
protocol/codec_tester.go Outdated Show resolved Hide resolved
crypto/stateproof/coinGenerator.go Outdated Show resolved Hide resolved
crypto/stateproof/coinGenerator.go Outdated Show resolved Hide resolved
protocol/codec_tester.go Outdated Show resolved Hide resolved
@id-ms
Copy link
Contributor

id-ms commented Aug 17, 2022

I think we should invoke this linter (with the same filters) when running make sanity, is it possible?
It is a good thing that we would be able to run it locally before CI fails.

protocol/codec_test.go Outdated Show resolved Hide resolved
@cce
Copy link
Contributor Author

cce commented Aug 17, 2022

Yes the issue was the linter was not correctly blocking builds with these issues, I think I fixed it in the reviewdog.yml change, and @winder has made this linter part of make sanity in #4418

winder
winder previously approved these changes Aug 17, 2022
Copy link
Contributor

@winder winder left a comment

Choose a reason for hiding this comment

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

LGTM

@cce
Copy link
Contributor Author

cce commented Aug 17, 2022

huh, fixing these errcheck linter warnings led to another issue:

Failed
panic: flag "app-arg" does not exist

goroutine 1 [running]:
github.com/algorand/go-algorand/cmd/goal.panicIfErr({0x1b3bd20, 0xc00059e160})
	/opt/cibuild/project/cmd/goal/application.go:199 +0x88
github.com/algorand/go-algorand/cmd/goal.init.1()
	/opt/cibuild/project/cmd/goal/application.go:194 +0x1dcc
FAIL	github.com/algorand/go-algorand/cmd/goal	0.138s

protocol/codec_test.go Outdated Show resolved Hide resolved
algorandskiy
algorandskiy previously approved these changes Aug 18, 2022
Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants