chore: use canonical golangci-lint github action #12134

faddat · 2022-06-03T08:38:43Z

Description

This is the canonical github action for golangci-lint.

https://github.com/golangci/golangci-lint-action

It has been set to allow the version of the linters to "roll" which will reduce the matienance needed and ensure that linting is kept up to date automatically.

By eschewing the typical pattern where we use a github action that runs linters against only a diff, we ensure that as the linters are updated upstream, we apply those changes here.

In the series of PR's described in #12133 , this one should be merged last.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

.github/workflows/lint.yml

tac0turtle · 2022-06-03T10:15:00Z

.github/workflows/lint.yml

-      - uses: technote-space/get-diff-action@v6.0.1
-        id: git_diff
-        with:
-          PATTERNS: |
-            **/**.go
-            go.mod
-            go.sum


we usually opt to have this to not run this position of ci when there are no changes to the go code.

I recommend that we run it all the time since the compute to do that costs nothing and the previous setup resulted in not linting at all.

But there's no sense in running it if there are zero code changes.

Yeah I can roll with it, I’ll make that change I’m just worried that I could introduce another bug that reproduces the prior state

please revert this change, after this we can look at merging

tac0turtle · 2022-06-03T10:15:35Z

.github/workflows/lint.yml

      - name: golangci-lint
-        if: env.GIT_DIFF
        uses: golangci/golangci-lint-action@v3


I think we have to migrate away from this action since it doesn't support workspaces or multiple go.mods in a single repo for now.

.github/workflows/lint.yml

Co-authored-by: Marko <marko@baricevic.me>

## Description This PR works towards #12133 and does a fumpt for consistency once sdk.Int is merged. The suggested merge order is to begin with sdk.Int, merge in the various linter PR's one by one, and then finally merge the changes to CI from #12134 --- ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [x] added `!` to the type prefix if API or client breaking change - [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting)) - [x] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [x] reviewed "Files changed" and left comments if necessary - [x] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable)

alexanderbez

We should be running make lint instead of using this job IMO. I say this because it uses a consistent version that both devs and CI will use. If we go with the job approach, the version thats used via make lint can differ from CI.

faddat · 2022-06-03T14:29:05Z

@alexanderbez not if we change the makefile to latest -- would that work for you?

The golangci-lint team makes very clear recommendations around using their github action, and the goals of this PR are:

currently the linters here are effectively broken, just like on gaia.
simplification and canonicalization
start a dialogue on exactly which linters we wish to enforce, by running golangci-lint at its latest version, with every code commit.

Because we used the diff github action in the past, I was able to solve hundreds of linter errors today. No exaggeration.

Then, our code practices here in the sdk filter into other ecosystem projects like gaia.

alexanderbez · 2022-06-03T14:36:06Z

We can use latest, yes that's fine, assuming dependabot keeps the versions in sync.

faddat · 2022-06-03T14:51:42Z

@alexanderbez I'm going to posit:

the current situation fixed by this PR is more serious than synced versions. We effectively got no feedback from the linters. Thousands of commits happened, without the linters doing what they should do.

So my preference here is to strictly follow the guidelines given by golangci-lint so that we end up getting that feedback in the future and can act on it

CHANGELOG.md

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

…ch-9

faddat · 2022-06-09T19:57:35Z

I may revert the changes to the cmd folder...

…ch-9

… I'll fix, as soon as I can remember how to.

faddat · 2022-06-09T20:18:37Z

This should now be complete. Once again I would like to note that there were a few items I was unsure how to handle. For example, do we want to change perfectly functional code that contains else if chains and could be refactored to be switch statements?

The only thing I don't like is I can't currently remember the solution to the filepath.Join(dir) <- only one argument, it isnt a join... problem. I know I have solved it somewhere, and will likely remember and bring the solution back here.

codecov · 2022-06-09T20:35:58Z

Codecov Report

Merging #12134 (0461757) into main (bc1c833) will increase coverage by 0.42%.
The diff coverage is 66.66%.

❗ Current head 0461757 differs from pull request most recent head 24c2175. Consider uploading reports for the commit 24c2175 to get more accurate results

@@            Coverage Diff             @@
##             main   #12134      +/-   ##
==========================================
+ Coverage   66.13%   66.56%   +0.42%     
==========================================
  Files         675      691      +16     
  Lines       71274    72535    +1261     
==========================================
+ Hits        47137    48280    +1143     
- Misses      21492    21575      +83     
- Partials     2645     2680      +35

Impacted Files	Coverage Δ
depinject/container.go	`91.52% <ø> (ø)`
depinject/errors.go	`100.00% <ø> (ø)`
simapp/app.go	`74.88% <ø> (-1.33%)`	⬇️
x/auth/client/testutil/suite.go	`94.15% <0.00%> (-0.25%)`	⬇️
x/genutil/module.go	`66.66% <ø> (ø)`
x/group/client/cli/util.go	`6.97% <0.00%> (ø)`
x/group/client/testutil/tx.go	`99.13% <ø> (ø)`
x/slashing/module.go	`67.85% <ø> (-0.38%)`	⬇️
client/keys/add.go	`63.51% <100.00%> (+0.16%)`	⬆️
client/tx/tx.go	`33.21% <100.00%> (ø)`
... and 57 more

faddat · 2022-06-09T20:43:01Z

x/auth/client/testutil/suite.go

@@ -1789,6 +1790,7 @@ func (s *IntegrationTestSuite) TestAuxToFeeWithTips() {
 					tc.feePayerArgs...,
 				)

+				//nolint:gocritic // rewriting this as a switch statement didn't make sense.


this is totally what I mean about codecov

eg:

comments aren't new code lacking test coverage

nolint statements aren't new code lacking test coverage

…ch-9

depinject/container.go

@@ -3,9 +3,10 @@
 import (
 	"bytes"
 	"fmt"
+	"reflect"


tac0turtle · 2022-06-13T09:18:21Z

.gitpod.yml

+# Please adjust to your needs (see https://www.gitpod.io/docs/config-gitpod-file)
+# and commit this file to your remote git repository to share the goodness with others.
+
+tasks:


please remove, this is out of scope for this pr

100%, sorry about that. It puts those there automatically, and is most annnoying part of gitpod.

tac0turtle · 2022-06-15T22:03:06Z

a few more stragglers, do you want to complete then we can go ahead with the merge

tac0turtle · 2022-06-15T22:03:06Z

a few more stragglers, do you want to complete then we can go ahead with the merge

faddat · 2022-06-20T17:49:34Z

hey sorry about the delay!

I thought no stragglers remained-- let me check er out now :)

tac0turtle · 2022-06-21T17:13:03Z

@faddat ill close this pr with #12318 to follow up. they seem to be overlapping

## Description This PR works towards #12133 and does a fumpt for consistency once sdk.Int is merged. The suggested merge order is to begin with sdk.Int, merge in the various linter PR's one by one, and then finally merge the changes to CI from #12134 --- ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [x] added `!` to the type prefix if API or client breaking change - [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting)) - [x] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [x] reviewed "Files changed" and left comments if necessary - [x] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable) (cherry picked from commit b7097c3) # Conflicts: # CHANGELOG.md # x/group/keeper/keeper.go

Update lint.yml

d952235

faddat requested a review from a team as a code owner June 3, 2022 08:38

github-actions bot added the Type: CI label Jun 3, 2022

faddat changed the title ~~Update lint.yml~~ chore: use canonical golangci-lint github action Jun 3, 2022

This was referenced Jun 3, 2022

Improve Linting #12133

Closed

chore!: var-naming linter errors #12135

Merged

faddat added 3 commits June 3, 2022 16:40

fix suspicious join by using .Dir instead of .Join

400fef3

Merge branch 'main' into suspicious-join

960919a

Update CHANGELOG.md

9dffa36

tac0turtle reviewed Jun 3, 2022

View reviewed changes

.github/workflows/lint.yml Outdated Show resolved Hide resolved

tac0turtle reviewed Jun 3, 2022

View reviewed changes

.github/workflows/lint.yml Outdated Show resolved Hide resolved

faddat and others added 6 commits June 3, 2022 17:50

Update .github/workflows/lint.yml

edc57bb

Co-authored-by: Marko <marko@baricevic.me>

Update .github/workflows/lint.yml

4118316

Co-authored-by: Marko <marko@baricevic.me>

Update CHANGELOG.md

9db408c

Merge branch 'main' into patch-9

0b66c4d

Merge branch 'main' into patch-9

241a808

Merge branch 'main' into patch-9

3e991f1

Merge branch 'main' into patch-9

f022bd3

alexanderbez reviewed Jun 3, 2022

View reviewed changes

Merge branch 'main' into patch-9

75ef457

Merge branch 'main' into patch-9

d801c2f

alexanderbez reviewed Jun 3, 2022

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

faddat and others added 2 commits June 4, 2022 03:36

Update CHANGELOG.md

d2cabb0

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

Merge branch 'main' into patch-9

30cb49a

faddat added 2 commits June 10, 2022 00:41

Merge branch 'patch-9' of https://github.com/faddat/basecoin into pat…

18835a4

…ch-9

Merge branch 'main' into patch-9

bd5667e

faddat added 6 commits June 10, 2022 03:03

unfix suspicious join in testnet.go

18becdf

Merge branch 'patch-9' of https://github.com/faddat/basecoin into pat…

d382053

…ch-9

Merge branch 'main' into patch-9

388eff7

Merge branch 'patch-9' of https://github.com/faddat/basecoin into pat…

6b97db8

…ch-9

revert changes from fix suspicious join

ca29bf1

I have marked two suspicious joins as nolint. There is a solution and…

a53a342

… I'll fix, as soon as I can remember how to.

Update lint.yml

8961afe

faddat commented Jun 9, 2022

View reviewed changes

faddat added 2 commits June 10, 2022 03:56

fumpt x/slashing/module.go

a503ef7

Merge branch 'patch-9' of https://github.com/faddat/basecoin into pat…

c16f56e

…ch-9

github-actions bot added the C:x/slashing label Jun 9, 2022

faddat added 3 commits June 10, 2022 07:33

Merge branch 'main' into patch-9

3ae1cc6

Merge branch 'main' into patch-9

9e1cbb4

fumpt

d78d968

github-actions bot added the C:x/genutil genutil module issues label Jun 10, 2022

github-advanced-security bot found potential problems Jun 10, 2022

View reviewed changes

depinject/container.go

@@ -3,9 +3,10 @@

import (

"bytes"

"fmt"

"reflect"

Check notice

Code scanning / CodeQL

Sensitive package import

Certain system packages contain functions which may be a possible source of non-determinism

tac0turtle reviewed Jun 13, 2022

View reviewed changes

Delete .gitpod.yml

0461757

Merge branch 'main' into patch-9

24c2175

tac0turtle closed this Jun 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: use canonical golangci-lint github action #12134

chore: use canonical golangci-lint github action #12134

faddat commented Jun 3, 2022 •

edited

tac0turtle Jun 3, 2022

faddat Jun 3, 2022

alexanderbez Jun 3, 2022

faddat Jun 7, 2022

tac0turtle Jun 13, 2022

tac0turtle Jun 3, 2022

alexanderbez left a comment

faddat commented Jun 3, 2022 •

edited

alexanderbez commented Jun 3, 2022

faddat commented Jun 3, 2022 •

edited

faddat commented Jun 9, 2022

faddat commented Jun 9, 2022

codecov bot commented Jun 9, 2022 •

edited

faddat Jun 9, 2022

faddat Jun 9, 2022

tac0turtle Jun 13, 2022

faddat Jun 13, 2022

tac0turtle commented Jun 15, 2022

tac0turtle commented Jun 15, 2022

faddat commented Jun 20, 2022

tac0turtle commented Jun 21, 2022

chore: use canonical golangci-lint github action #12134

chore: use canonical golangci-lint github action #12134

Conversation

faddat commented Jun 3, 2022 • edited

Description

Author Checklist

Reviewers Checklist

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

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

faddat commented Jun 3, 2022 • edited

alexanderbez commented Jun 3, 2022

faddat commented Jun 3, 2022 • edited

faddat commented Jun 9, 2022

faddat commented Jun 9, 2022

codecov bot commented Jun 9, 2022 • edited

Codecov Report

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

tac0turtle commented Jun 15, 2022

tac0turtle commented Jun 15, 2022

faddat commented Jun 20, 2022

tac0turtle commented Jun 21, 2022

faddat commented Jun 3, 2022 •

edited

faddat commented Jun 3, 2022 •

edited

faddat commented Jun 3, 2022 •

edited

codecov bot commented Jun 9, 2022 •

edited