Add golangci lint check on pull requests #645

fkneeland-figure · 2021-10-13T16:50:15Z

Resolves #620
I added a github action to run the golangci lint test on all non-test golang files when a pr is created.

fkneeland-figure · 2021-10-13T17:10:39Z

@alpe This is ready for review whenever you are free.

ethanfrey · 2021-10-13T17:35:15Z

Thanks for this contribution.

It seems GitHub actions are not run on this repo.
We use CircleCI for all other scripts, would it be possible to add it there instead?

codecov · 2021-10-13T19:43:24Z

Codecov Report

Merging #645 (ea497e9) into master (c34b486) will decrease coverage by 0.27%.
The diff coverage is 42.55%.

@@            Coverage Diff             @@
##           master     #645      +/-   ##
==========================================
- Coverage   60.22%   59.95%   -0.28%     
==========================================
  Files          48       48              
  Lines        5315     5324       +9     
==========================================
- Hits         3201     3192       -9     
- Misses       1890     1906      +16     
- Partials      224      226       +2

Impacted Files	Coverage Δ
app/encoding.go	`100.00% <ø> (ø)`
app/export.go	`11.76% <0.00%> (-0.36%)`	⬇️
app/test_helpers.go	`0.85% <0.00%> (-0.02%)`	⬇️
x/wasm/client/cli/new_tx.go	`0.00% <0.00%> (ø)`
x/wasm/client/cli/query.go	`0.00% <0.00%> (ø)`
x/wasm/client/cli/tx.go	`24.00% <0.00%> (-0.66%)`	⬇️
x/wasm/keeper/api.go	`42.85% <0.00%> (ø)`
x/wasm/keeper/ibc.go	`77.77% <ø> (ø)`
x/wasm/keeper/metrics.go	`0.00% <ø> (ø)`
x/wasm/keeper/query_plugins.go	`79.93% <ø> (ø)`
... and 18 more

ethanfrey

One tip to ensure this is run in the CI.
Then I will see the results of the CI run.

Thanks for hooking this up.

ethanfrey · 2021-10-14T11:01:38Z

.circleci/config.yml

@@ -60,6 +60,17 @@ jobs:
          paths:
            - ".git"

+  lint:


Looks good.

I was surprised it didn't show up on the PR. But then realised it is missing a section like this: https://github.com/CosmWasm/wasmd/pull/645/files#diff-78a8a19706dbd2a4425dd72bdab0502ed7a2cef16365ab7030a5a0588927bf47L194-L196

Can you add 3 lines to workflow (running after setup-dependencies makes sense to me), and I can see the results in the CI.

Sweet, that makes sense I was wondering why it wasn't running. I just pushed a change adding it to the workflow like you said. Thank you!

Makefile

x/wasm/ibc.go

fkneeland-figure · 2021-10-14T18:58:16Z

I added all the changes from running make format into this pr, I am cool with those changes being merged in this pr or separately in the other pr whatever is preferred.

#648

ethanfrey · 2021-10-14T19:33:51Z

Personally, I would revert the last 2 commits (or move them to another PR on top of this).

I could merge the CI changes alone (that is a clear unit) and then there can be multiple "cleanup" PRs.

ethanfrey · 2021-10-14T19:34:15Z

Can you ping me when you revert those 2 and I can merge

fkneeland-figure · 2021-10-14T20:02:53Z

@ethanfrey sounds good, lets merge: #648 first to take care of those two commits and it looks like there are still a couple issues I need to fix with the golinter so I will work on those in the meantime.

fkneeland-figure · 2021-10-14T22:35:08Z

This is now passing ci and is ready for review once #648 has been merged into master and then master is merged into this pr

…lint_check

alpe

Very nice start. This was a lot of work I guess. Thanks a lot! 💐
The linter also worked well and I discovered some 🐛 in the code that we should be addressed.
If you don't have the time/capacity then please let me know.

app/test_helpers.go

cmd/wasmd/root.go

x/wasm/client/cli/genesis_msg.go

x/wasm/keeper/msg_dispatcher.go

x/wasm/keeper/staking_test.go

x/wasm/types/genesis.go

x/wasm/types/types.pb.go

.golangci.yml

alpe · 2021-10-21T13:00:36Z

@fkneeland-figure would you rebase this on top of master? As I wrote there are some bugs in the original code that I would like to address soon. If you don't have time/capacity at the moment then please let me know so that I start another linter branch.

…ays revert

fkneeland-figure · 2021-10-22T15:51:34Z

@alpe I am available to work on this today and will try to get those bug fixes in quickly. Thanks!

…lint_check

fkneeland-figure · 2021-10-22T17:01:26Z

app/export.go

-		_, _ = app.distrKeeper.WithdrawValidatorCommission(ctx, val.GetOperator())
+		_, err := app.distrKeeper.WithdrawValidatorCommission(ctx, val.GetOperator())
+		if err != nil {
+			panic(err)


@alpe I added these to address these errors being ignored, but idk if this is desired behavior?

This was copied from the SDk: https://github.com/cosmos/cosmos-sdk/blob/v0.42.10/simapp/export.go#L75
We should stay with their defaults here. I looked a the code and there are 2 cases that can cause an error:

accumulated commission is 0

SendCoinsFromModuleToAccount returns error

The first case is more likely. I will address this in a new PR

fkneeland-figure · 2021-10-22T17:05:14Z

Thanks for the contributions @channa-figure

This is passing the lint test now and I believe we have addressed all of your comments @alpe whenever you get the chance to do another review of this. Thank you!

alpe

Very nice! Thanks a lot for your contribution and addressing all my review comments. I appreciate your work a lot. 💯

alpe · 2021-10-25T07:08:30Z

Makefile

-	find . -name '*.go' -type f -not -path "./vendor*" -not -path "*.git*" -not -path "./client/lcd/statik/statik.go" | xargs gofmt -w -s
-	find . -name '*.go' -type f -not -path "./vendor*" -not -path "*.git*" -not -path "./client/lcd/statik/statik.go" | xargs misspell -w
-	find . -name '*.go' -type f -not -path "./vendor*" -not -path "*.git*" -not -path "./client/lcd/statik/statik.go" | xargs goimports -w -local github.com/CosmWasm/wasmd
+	find . -name '*.go' -type f -not -path "./vendor*" -not -path "*.git*" -not -path "./client/lcd/statik/statik.go" -not -path "*.pb.go" | xargs gofmt -w -s


Formatting the auto generated files is completely fine. I was only concerned about manual changes that we need to avoid.
I will revert these modification in a new PR as the generated files do not match the formatted files in this PR anymore.

* added golangci lint check on pull requests * changelog update * updated to use circleci instead of github actions * testing a golang change * added workflow * updated how we find the golangci command in the make file * using orb for golangci * Added golangci install make command * use docker image for golangci * stop using make in circleci * reverted golang change to trigger ci * gofmt * make fmt * fixed a few things * updated version on golintci * fixed all the lint errors * check version * skipped wrongly failing lint * Revert generated file changes * fix import grouping, return errors on failures, initialization of arrays revert * fixed a few lint errors * addressed more code review comments * updated with error check * increased timeout for golangci-lint * dont format autogenerated files Co-authored-by: Carlton Hanna <channa@figure.com>

fkneeland-figure added 2 commits October 13, 2021 10:45

added golangci lint check on pull requests

9d94e1a

changelog update

ab59a57

fkneeland-figure requested a review from alpe as a code owner October 13, 2021 16:50

updated to use circleci instead of github actions

b227083

testing a golang change

d823038

ethanfrey reviewed Oct 14, 2021

View reviewed changes

fkneeland-figure added 9 commits October 14, 2021 09:49

added workflow

0f10cdc

updated how we find the golangci command in the make file

6f64807

using orb for golangci

dea6f5e

Added golangci install make command

b0ea893

use docker image for golangci

35ec810

stop using make in circleci

c61d6d5

reverted golang change to trigger ci

d0b2de1

gofmt

923ac58

make fmt

da0d52d

fkneeland-figure added 5 commits October 14, 2021 14:53

fixed a few things

d91096c

updated version on golintci

77c869b

fixed all the lint errors

1deb262

check version

8a73a78

skipped wrongly failing lint

024a487

Merge branch 'master' of github.com:CosmWasm/wasmd into add_golangci_…

8146898

…lint_check

alpe suggested changes Oct 18, 2021

View reviewed changes

This was referenced Oct 18, 2021

Go Formatting update #648

Merged

Improvement ideas #595

Closed

alpe mentioned this pull request Oct 21, 2021

Handle error on Address not found #657

Merged

channa-figure added 2 commits October 21, 2021 13:35

Revert generated file changes

42209ca

fix import grouping, return errors on failures, initialization of arr…

cff7a60

…ays revert

fkneeland-figure added 6 commits October 22, 2021 09:55

Merge branch 'master' of github.com:CosmWasm/wasmd into add_golangci_…

de272a3

…lint_check

fixed a few lint errors

54b1861

addressed more code review comments

c73cf0b

updated with error check

5df2dab

increased timeout for golangci-lint

90dc4c0

dont format autogenerated files

ea497e9

fkneeland-figure commented Oct 22, 2021

View reviewed changes

alpe approved these changes Oct 25, 2021

View reviewed changes

alpe merged commit 57517b0 into CosmWasm:master Oct 25, 2021

This was referenced Oct 25, 2021

Format generated files #661

Merged

Revert error handling on export #662

Merged

Add linter name and minor golint related updates #663

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add golangci lint check on pull requests #645

Add golangci lint check on pull requests #645

fkneeland-figure commented Oct 13, 2021 •

edited by alpe

Loading

fkneeland-figure commented Oct 13, 2021

ethanfrey commented Oct 13, 2021

codecov bot commented Oct 13, 2021 •

edited

Loading

ethanfrey left a comment

ethanfrey Oct 14, 2021

fkneeland-figure Oct 14, 2021

fkneeland-figure commented Oct 14, 2021 •

edited

Loading

ethanfrey commented Oct 14, 2021

ethanfrey commented Oct 14, 2021

fkneeland-figure commented Oct 14, 2021

fkneeland-figure commented Oct 14, 2021

alpe left a comment

alpe commented Oct 21, 2021

fkneeland-figure commented Oct 22, 2021

fkneeland-figure Oct 22, 2021

alpe Oct 25, 2021

fkneeland-figure commented Oct 22, 2021

alpe left a comment

alpe Oct 25, 2021

@@ @@ -60,6 +60,17 @@ jobs: @@
                         paths:
                           - ".git"
+                lint:

Add golangci lint check on pull requests #645

Add golangci lint check on pull requests #645

Conversation

fkneeland-figure commented Oct 13, 2021 • edited by alpe Loading

fkneeland-figure commented Oct 13, 2021

ethanfrey commented Oct 13, 2021

codecov bot commented Oct 13, 2021 • edited Loading

Codecov Report

ethanfrey left a comment

Choose a reason for hiding this comment

ethanfrey Oct 14, 2021

Choose a reason for hiding this comment

fkneeland-figure Oct 14, 2021

Choose a reason for hiding this comment

fkneeland-figure commented Oct 14, 2021 • edited Loading

ethanfrey commented Oct 14, 2021

ethanfrey commented Oct 14, 2021

fkneeland-figure commented Oct 14, 2021

fkneeland-figure commented Oct 14, 2021

alpe left a comment

Choose a reason for hiding this comment

alpe commented Oct 21, 2021

fkneeland-figure commented Oct 22, 2021

fkneeland-figure Oct 22, 2021

Choose a reason for hiding this comment

alpe Oct 25, 2021

Choose a reason for hiding this comment

fkneeland-figure commented Oct 22, 2021

alpe left a comment

Choose a reason for hiding this comment

alpe Oct 25, 2021

Choose a reason for hiding this comment

fkneeland-figure commented Oct 13, 2021 •

edited by alpe

Loading

codecov bot commented Oct 13, 2021 •

edited

Loading

fkneeland-figure commented Oct 14, 2021 •

edited

Loading