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

Consolidate linters with golangci-lint #3758

Merged
merged 1 commit into from
Dec 23, 2022
Merged

Conversation

MichaelSnowden
Copy link
Contributor

@MichaelSnowden MichaelSnowden commented Dec 22, 2022

What changed?
In this PR, I added golangci-lint to our repo, consolidating our compatible linters with this linter aggregator.

Why?
I made this change for a few reasons:

  1. This linter aggregator has a GitHub action which shows violations in-line on PRs
  2. This linter has a --new option which allows it to be applied only to new changes because we currently have a massive number of violations.
  3. golint is deprecated
  4. No actions were taken based on the issues raised by some of the previous linters--all of these warnings are blocking and actionable

How did you test it?
I ran it locally and saw that only new errors were checked. I also tested it locally a few times using act. To test this in a real example, I created a new branch here and verified that the annotations were published inline (on push), so we can see the annotations before the PR is even opened: d72112d

Potential risks
Slows down dev to fix linter warnings.

Is hotfix candidate?
No

@MichaelSnowden MichaelSnowden force-pushed the linters/golangci-lint branch 5 times, most recently from 62d3e95 to f282960 Compare December 22, 2022 19:10
@MichaelSnowden MichaelSnowden marked this pull request as ready for review December 22, 2022 19:41
@MichaelSnowden MichaelSnowden requested a review from a team as a code owner December 22, 2022 19:41
@@ -264,7 +243,7 @@ shell-check:
@printf $(COLOR) "Run shellcheck for script files..."
@shellcheck $(ALL_SCRIPTS)

check: copyright-check goimports-check lint vet staticcheck errcheck
Copy link
Member

Choose a reason for hiding this comment

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

I would just remove check.

@alexshtin
Copy link
Member

This is great! Thanks @MichaelSnowden for doing this.

@MichaelSnowden MichaelSnowden merged commit 86e9d32 into master Dec 23, 2022
@MichaelSnowden MichaelSnowden deleted the linters/golangci-lint branch December 23, 2022 03:17
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.

2 participants