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

Jump to bottom

Set GO111MODULE to auto in golint script– #1743 #1744

Merged
merged 3 commits into from
Dec 10, 2021
Merged

Set GO111MODULE to auto in golint script– #1743 #1744

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4572813
Set GO111MODULE to auto in golint script– #1743
rbrtl Dec 9, 2021
61017a8
Remove redundant code – golint script
rbrtl Dec 10, 2021
d76ace1
Replace `golint` with `go vet`
rbrtl Dec 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion scripts/check_golint.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,4 @@
# Run the linter on everything except generated code
set -euo pipefail

golint -set_exit_status $(GO111MODULE=off go list ./... | grep -v '/mocks')
golint -set_exit_status $(GO111MODULE=auto go list ./... | grep -v '/mocks')
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for this change. I believe we have now started to build for go1.17.

Go1.17 will ignore this flag: https://go.dev/blog/go116-module-changes.
I believe: #815 was an explicit choice though to make the CI faster

I am not sure if this will work anymore. Maybe we should just remove it: cc @marwan-at-work ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I think it's worth removing. Also, do we even have /mock? I wonder if we can do golinst ./... or golint go list ./...

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'll do it right and raise another PR!

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 checked golint ./... vs. golint $(go list ./...)... I am struggling to prove it definitively prove that the former is linting all the files covered by the latter. The go list subshell command version takes noticeably longer, but that's not to say, necessarily, that the golint command isn't examining all the correct files. Any input on that much appreciated.

Copy link
Member

@manugupt1 manugupt1 Dec 10, 2021 •
edited

Choose a reason for hiding this comment

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

Thank you for doing it right. I just noticed that even golint is deprecated.
https://github.com/golang/lint
Would you be able to update the code to use staticcheck and go vet?

We already use govet

athens/.drone.yml

Line 32 in fd54bab

- go vet ./...

I would probably just start with govet, open an issue and then do a separate PR for staticcheck. I hope this helps.