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

ci: implement golang linter #11

Merged
merged 9 commits into from
May 22, 2024
Merged

ci: implement golang linter #11

merged 9 commits into from
May 22, 2024

Conversation

joelsmith-2019
Copy link
Contributor

Would love some feedback on which linter rules we want and don't want. I threw them all in there essentially and wrote a note that they can be enabled/disabled on a per repo basis.

@joelsmith-2019 joelsmith-2019 self-assigned this May 9, 2024
@joelsmith-2019
Copy link
Contributor Author

Once the CodeQL repo get's merged, there will be a simple go module in the repo that this could try to lint. Right now its failing because it cannot find a go module.

@joelsmith-2019 joelsmith-2019 requested a review from a team as a code owner May 10, 2024 22:09
Makefile Outdated Show resolved Hide resolved
Copy link
Member

@jtieri jtieri left a comment

Choose a reason for hiding this comment

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

this looks pretty good! i went through the list of configured linters and made sure that all of the enabled ones make sense and provide something useful and these all seem good. i left a couple comments with some additional linters i would like to see added.

here is a full list of linters that golangci-lint provides, in case anyone else wants to pick through the list and see if there is something we ought to enable. maybe @mark-rushakoff has some insightful opinions here, if not i think this looks good to me and we can always iterate on this as necessary.

i'm unsure about making use of the make command in CI instead of using the actual GitHub action. i think we miss out on some performance gains if we don't use the action because it is optimized for CI and adds some things like smart caching. it also appears to provide annotations and logs that are easier to analyze when issues are found in CI so you don;t have to dig through the build logs. i know we discussed it means having to keep the versioning in check for both the Makefile and the GitHub action's config file but tbh i think the benefits the dedicated action provide outweigh the low burden of having to update versions in two places.

Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
.golangci.yml Show resolved Hide resolved
@joelsmith-2019
Copy link
Contributor Author

joelsmith-2019 commented May 20, 2024

All suggestions were implemented!

  • Checks if go packages are installed in Makefile before installing again
  • Adds all lint recommendations
  • Now uses the golangci lint action instead of Makefile
  • Adds make gofumpt to Makefile

Copy link
Member

@jtieri jtieri left a comment

Choose a reason for hiding this comment

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

this looks great Joel, good job!!

@jtieri jtieri merged commit 51ecf47 into main May 22, 2024
3 of 5 checks passed
@jtieri jtieri deleted the joel/linter branch May 22, 2024 20:59
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