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

Run golangci-lint on all packages #1801

Open
yondonfu opened this issue Mar 18, 2021 · 2 comments
Open

Run golangci-lint on all packages #1801

yondonfu opened this issue Mar 18, 2021 · 2 comments
Labels
area: CI good first issue type: tech debt chores, performance improvements, etc

Comments

@yondonfu
Copy link
Member

We currently only run golangci-lint on certain packages in CI. We should run golangci-lint locally, fix all the warnings/errors and then start running golangci-lint on all packages in CI. The one thing to consider here is which linters should be enabled by default (or just use the default linters used by golangci-lint).

@leszko leszko added type: tech debt chores, performance improvements, etc and removed tech debt labels May 11, 2022
@cyberj0g
Copy link
Contributor

cyberj0g commented Sep 18, 2022

Might want to prioritize because of bugs like #2589 (PR #2591).
golangci-lint can catch that:

58:core/orchestrator.go:634:4: lostcancel: the cancel function is not used on all paths (possible context leak) (govet)
59-			ctx, cancel := context.WithTimeout(context.Background(), transcodeLoopTimeout)
60-			^
61:core/orchestrator.go:639:5: lostcancel: this return statement may be reached without using the cancel var defined on line 634 (govet)
62-				return
63-				^

There are about 200 linting warnings currently, I see a few options how to minimize efforts of enabling it:

  1. disable linters which check for less severe errors, like errcheck, govet, unused, gosimple
  2. only lint files changed in the commit, which still may be painful and could be combined with option 1
  3. do not lint tests

@yondonfu
Copy link
Member Author

disable linters which check for less severe errors

In the case of #2589 we would have wanted govet to be enabled to flag the context issues right?

only lint files changed in the commit, which still may be painful and could be combined with option 1

A few alternatives I am wondering about:

  1. Lint changed lines only
  2. Configure the linter to emit certain less severe errors as just warnings so that CI doesn't fail for them and only fails for the more "interesting" errors

Then, a gradual approach might look like:

  • Lint files changed in the commit
  • If someone opens a PR that fails linting, first create a separate PR that fixes the linting issues in the relevant files
  • Merge the separate PR and then rebase the original PR which should pass linting

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: CI good first issue type: tech debt chores, performance improvements, etc
Projects
None yet
Development

No branches or pull requests

3 participants