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

Set up golangci-lint linter based on gometalinter #670

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ginywiny
Copy link
Contributor

@ginywiny ginywiny commented Aug 7, 2021

Creating a new PR because the previous one had a weird amount of strange commit history which went over my head to squash despite the many attempts taken.

Test HIGHLY based on test-gometalinter, as it is used to lint the same files, except where golangci-lint is used instead of gometalinter.

Notes:

  1. When golangci-lint uses the --skip-files flag, it still lints the file, but simply doesn't display the linting results.
  2. The build fails because the revive linter has found issues with certain files. This is new as revive is a new linter being used to replace golint, as it runs more quickly. This will be fixed down the line.
  3. Comments and TODOs from the previous gometalinter test file were kept in case new linters are being used.

What was done:

  1. Updated make deps script to install golangci-lint
  2. Created golangci-lint script based on test-gometalinter
  3. Updated test script to run test-golangci-lint

@purpleidea
Copy link
Owner

I see:

./test.sh: line 34: ./test/test-golangci-lint.sh: Permission denied

Did you u+x the file? Where do you see the list of lint errors we expect?

@ginywiny ginywiny force-pushed the feat/create_golangci_lint_test_487 branch from 42c144f to 19116af Compare August 9, 2021 20:00
@ginywiny
Copy link
Contributor Author

ginywiny commented Aug 9, 2021

@purpleidea Whoops, must've forgot to change that. Fixed it and squashed.
The list of errors are expected for the "revive" linter.

@purpleidea
Copy link
Owner

Great! Please rebase to master and force-push and let's see what happens. (I pushed some fixes.)

@ginywiny ginywiny force-pushed the feat/create_golangci_lint_test_487 branch from 19116af to 9d05a93 Compare August 10, 2021 00:17
Copy link
Owner

@purpleidea purpleidea left a comment

Choose a reason for hiding this comment

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

Patch looks good, just needs a few small changes.
Also check your $EDITOR, it's letting you save files without EOF newlines.

.golangci.yml Outdated Show resolved Hide resolved
@@ -157,6 +157,9 @@ go get golang.org/x/lint/golint # for `golint`-ing
go get golang.org/x/tools/cmd/goimports # for fmt
go get github.com/kevinburke/go-bindata/go-bindata # for compiling in non golang files
go get github.com/dvyukov/go-fuzz/go-fuzz # for fuzzing the mcl lang bits
# heavily recommended to use curl to install instead of go get under https://golangci-lint.run/usage/install/
curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.41.1
Copy link
Owner

Choose a reason for hiding this comment

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

why is this version hardcoded? can we use the latest version? is there a safer way to do this so we don't pipe to shell? if not, please change this so it only runs in ci.

Copy link
Contributor Author

@ginywiny ginywiny Aug 13, 2021

Choose a reason for hiding this comment

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

It is recommended to use a specific version as per the installation instructions (https://golangci-lint.run/usage/install/), to avoid introducing new linter errors from new minor or major releases. However, would it still be preferred to use the latest version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed this to something else. Any thoughts of it now?

test.sh Outdated Show resolved Hide resolved
test/test-golangci-lint.sh Outdated Show resolved Hide resolved
# using .golangci.yml config file settings in ROOT
gcl='golangci-lint run'

# commented out from gometalinter linter test
Copy link
Owner

Choose a reason for hiding this comment

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

are these five lines of comments valid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added these lines in case somebody were to move over any changes from gometalinter to golangci-lint. There were some linters which were entirely commented out, whereas some were still left as TODO, since they still require some fixes. More for clarity purposes, but they can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are they still valid?

test/test-golangci-lint.sh Outdated Show resolved Hide resolved
test/test-golangci-lint.sh Outdated Show resolved Hide resolved
@ginywiny ginywiny force-pushed the feat/create_golangci_lint_test_487 branch 2 times, most recently from 0b0a333 to 62407e2 Compare August 13, 2021 02:45
@ginywiny ginywiny requested a review from purpleidea August 31, 2021 02:28
@ginywiny ginywiny force-pushed the feat/create_golangci_lint_test_487 branch 12 times, most recently from 4d5cf02 to 021b5bb Compare September 5, 2021 22:36
@ginywiny ginywiny force-pushed the feat/create_golangci_lint_test_487 branch from 021b5bb to 5ffc591 Compare September 7, 2021 04:51
skip-files:
- lang/lexer.nn.go
- lang/y.go
- bindata/bindata.go
Copy link
Contributor

@frebib frebib Sep 27, 2021

Choose a reason for hiding this comment

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

This is/will be removed in #671 so if that's merged first, this will need to be rebased and updated

@@ -157,10 +157,35 @@ go get golang.org/x/lint/golint # for `golint`-ing
go get golang.org/x/tools/cmd/goimports # for fmt
go get github.com/kevinburke/go-bindata/go-bindata # for compiling in non golang files
go get github.com/dvyukov/go-fuzz/go-fuzz # for fuzzing the mcl lang bits

if in_ci; then
go get -u gopkg.in/alecthomas/gometalinter.v1 && \
mv "$(dirname $(command -v gometalinter.v1))/gometalinter.v1" "$(dirname $(command -v gometalinter.v1))/gometalinter" && \
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove all of the gometalinter references from here and other places in the codebase?

# sh -s -- -b $(go env GOPATH)/bin/golangci-lint v1.42.0

# curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.42.0
curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh > $(go env GOPATH)/bin/golangci-lint-install
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know what @purpleidea thinks about this, but it seems wise to run this check in Github Actions, at least when running in Github. Perhaps we can skip installation and running in GH and only do it in other CI systems? Then to provide the same functionality, drop in the relevant configuration into https://github.com/purpleidea/mgmt/blob/master/.github/workflows/test.yaml
https://golangci-lint.run/usage/install/#github-actions

Copy link
Contributor

Choose a reason for hiding this comment

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

To clarify (as explained on the linked page), this provides useful comments in PRs in context of where the issues are, instead of being hidden away in the actions tab

@purpleidea
Copy link
Owner

@ginywiny Want to rebase to newest master and fixup?

@ginywiny
Copy link
Contributor Author

ginywiny commented Oct 5, 2021

@purpleidea I will rebase and make a fix.

@purpleidea purpleidea force-pushed the master branch 2 times, most recently from 9e8c92c to a009c52 Compare September 12, 2022 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants