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(CI): add golangci-lint to dependencies #10112

Merged
merged 14 commits into from
Sep 14, 2021
+417 −20
Merged

fix(CI): add golangci-lint to dependencies #10112

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
a0d8a88
fix(CI): add golangci-lint to dependencies
robert-zaremba Sep 10, 2021
f1a60a2
update github workflow
robert-zaremba Sep 10, 2021
eb54288
tidy go.mod
robert-zaremba Sep 10, 2021
011d00c
add missing entries to go.mod
robert-zaremba Sep 10, 2021
51171e4
update deps
robert-zaremba Sep 10, 2021
d6c8cf1
add missing run command
robert-zaremba Sep 10, 2021
f7c7c5e
remove golint and use revive instead
robert-zaremba Sep 10, 2021
0b45fdb
fix workflow setup
robert-zaremba Sep 10, 2021
fa975fa
lint only changed files
robert-zaremba Sep 10, 2021
8a23a2a
gofmt go tags
robert-zaremba Sep 10, 2021
bddb64a
update comments
robert-zaremba Sep 10, 2021
d90176d
Merge branch 'master' into robert/golang-linter
tac0turtle Sep 10, 2021
1e221db
Merge branch 'master' into robert/golang-linter
fedekunze Sep 13, 2021
e6d385b
make lint-go
robert-zaremba Sep 14, 2021
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
14 changes: 6 additions & 8 deletions .github/workflows/lint.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,19 +11,17 @@ jobs:
golangci:
name: golangci-lint
runs-on: ubuntu-latest
timeout-minutes: 6
steps:
- uses: actions/checkout@v2
- uses: actions/setup-go@v2.1.4
with:
go-version: 1.16
- uses: technote-space/get-diff-action@v5
id: git_diff
with:
PATTERNS: |
**/**.go
go.mod
go.sum
- uses: golangci/golangci-lint-action@master
with:
# Required: the version of golangci-lint is required and must be specified without patch version: we always use the latest patch version.
version: v1.39
args: --timeout 10m
github-token: ${{ secrets.github_token }}
- name: run go linters
run: go run github.com/golangci/golangci-lint/cmd/golangci-lint run --out-format=tab ${{ env.GIT_DIFF }}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm linting only changed files (NOT all files). I think this speedups the flow.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just run the make target?

Copy link
Member

Choose a reason for hiding this comment

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

this will be nice if another pr causes issues for a PR that doesnt touch it. That being said i could see linting getting messy if prs are merged without fully checking things

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why not just run the make target?

Because make job lints all files. Linting only changed files speeds up the process. I'm fixing "all" issues in other PR.

That being said i could see linting getting messy if prs are merged without fully checking things

This requires a social consensus and an admin power - in that case we should have a task to fix that later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moving forward - I'm OK to change it to always lint everything. My motivation was that an author should be responsible for "his" changes in a PR, and not being blocked by someone else changes which get in using super powers.
Also speeding up the CI is a nice thing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@alexanderbez it is the ${{ env.GIT_DIFF }} part

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@marbar3778 We have only actions for PRs. I'm not sure if we can make an action for a commit to master.... This sounds more like a status. But that sounds like a new feature: master dashoboard

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let's make a decision. My preference is to only lint changed files for the reason I explained above. But it's not a strong preference.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, but that has nothing to do with the actual lint command. This is why I suggested to use a make target instead of running the lint command directly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@alexanderbez : how about breaking down the make lint and add make lint-go and use $GIT_DIFF there?

if: env.GIT_DIFF
2 changes: 1 addition & 1 deletion .golangci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,14 @@ linters:
- gocritic
- gofmt
- goimports
- golint
- gosec
- gosimple
- govet
- ineffassign
- misspell
- nakedret
- prealloc
- revive
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

golint is deprecated (warning was issued) and replaced by revive

- exportloopref
- staticcheck
- structcheck
Expand Down
7 changes: 5 additions & 2 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,12 +344,15 @@ markdownLintImage=tmknom/markdownlint
containerMarkdownLint=$(PROJECT_NAME)-markdownlint
containerMarkdownLintFix=$(PROJECT_NAME)-markdownlint-fix

golangci_lint_cmd=go run github.com/golangci/golangci-lint/cmd/golangci-lint

# NOTE: if you change the `lint` job then you must update ./github/workflows/lint.yml
Copy link
Member

Choose a reason for hiding this comment

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

can we just use this command? this will fail so quickly and we will diverge

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we can - but this will lint "everything".

lint:
golangci-lint run --out-format=tab
$(golangci_lint_cmd) run --out-format=tab
@if docker ps -a --format '{{.Names}}' | grep -Eq "^${containerMarkdownLint}$$"; then docker start -a $(containerMarkdownLint); else docker run --name $(containerMarkdownLint) -i -v "$(CURDIR):/work" $(markdownLintImage); fi
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have docker commands here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is for Markdown linting, not for Go code. We use a Tendermint container image here.


lint-fix:
golangci-lint run --fix --out-format=tab --issues-exit-code=0
$(golangci_lint_cmd) run --fix --out-format=tab --issues-exit-code=0
@if docker ps -a --format '{{.Names}}' | grep -Eq "^${containerMarkdownLintFix}$$"; then docker start -a $(containerMarkdownLintFix); else docker run --name $(containerMarkdownLintFix) -i -v "$(CURDIR):/work" $(markdownLintImage) . --fix; fi

.PHONY: lint lint-fix
Expand Down
8 changes: 7 additions & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ require (
github.com/gogo/protobuf v1.3.3
github.com/golang/mock v1.6.0
github.com/golang/protobuf v1.5.2
github.com/golangci/golangci-lint v1.42.1 // indirect
github.com/gorilla/handlers v1.5.1
github.com/gorilla/mux v1.8.0
github.com/grpc-ecosystem/go-grpc-middleware v1.3.0
Expand All @@ -26,8 +27,12 @@ require (
github.com/hdevalence/ed25519consensus v0.0.0-20210204194344-59a8610d2b87
github.com/improbable-eng/grpc-web v0.14.1
github.com/jhump/protoreflect v1.9.0
github.com/kr/text v0.2.0 // indirect
github.com/magiconair/properties v1.8.5
github.com/mattn/go-isatty v0.0.14
github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e // indirect
github.com/onsi/ginkgo v1.16.4 // indirect
github.com/onsi/gomega v1.13.0 // indirect
github.com/otiai10/copy v1.6.0
github.com/pkg/errors v0.9.1
github.com/prometheus/client_golang v1.11.0
Expand All @@ -45,10 +50,11 @@ require (
github.com/tendermint/go-amino v0.16.0
github.com/tendermint/tendermint v0.34.13
github.com/tendermint/tm-db v0.6.4
golang.org/x/crypto v0.0.0-20201221181555-eec23a3978ad
golang.org/x/crypto v0.0.0-20210513164829-c07d793c2f9a
google.golang.org/genproto v0.0.0-20210602131652-f16073e35f0c
google.golang.org/grpc v1.40.0
google.golang.org/protobuf v1.27.1
gopkg.in/check.v1 v1.0.0-20200227125254-8fa46927fb4f // indirect
gopkg.in/yaml.v2 v2.4.0
)

Expand Down
Loading