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

[all] Enable more linters on this repository #9109

Closed
10 tasks done
mx-psi opened this issue Apr 7, 2022 · 11 comments
Closed
10 tasks done

[all] Enable more linters on this repository #9109

mx-psi opened this issue Apr 7, 2022 · 11 comments
Assignees
Labels
ci-cd CI, CD, testing, build issues good first issue Good for newcomers help wanted Extra attention is needed

Comments

@mx-psi
Copy link
Member

mx-psi commented Apr 7, 2022

The opentelemetry-collector repository has two checks that are not enabled here. This is a meta issue to track these.

Linters

Errcheck

Gocritic

Depguard (for sync/atomic):

See more context at open-telemetry/opentelemetry-collector#5215.

Exhaustive

We also have an existing issue for checking coverage for all metric types that can be tackled with a linter:

Vulncheck

Possible additional work

Some more linters that could be enabled here (these are not in the core repo): deadcode, structcheck, unused, ineffassign, tenv, go-errorlint.

Approach

Fixing all errors at once is a lot of work, so I would suggest doing things in several steps:

  1. A first PR enables the linter, excluding it on any components that are not passing right now.
  2. Subsequent PRs make it pass on individual components and progressively remove the exclusion rules on faulty components. We can create individual issues for each linter and each component, depending on how hard they are to fix.
@mx-psi mx-psi added help wanted Extra attention is needed good first issue Good for newcomers ci-cd CI, CD, testing, build issues labels Apr 7, 2022
@fatsheep9146
Copy link
Contributor

I'd love to help with this. If ok, you can assign this to me. @mx-psi

@github-actions
Copy link
Contributor

github-actions bot commented Jan 2, 2023

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

@lootek
Copy link
Contributor

lootek commented Mar 1, 2023

I think it'd be great to add govulncheck as well. The good news is it passes on current main when run using go1.19.6 on mac OS:

$ govulncheck ./...
govulncheck is an experimental tool. Share feedback at https://go.dev/s/govulncheck-feedback.

Using go1.19.6 and govulncheck@v0.0.0 with
vulnerability data from https://vuln.go.dev (last modified 28 Feb 23 22:58 UTC).

Scanning your code and 3181 packages across 587 dependent modules for known vulnerabilities...
No vulnerabilities found.

@mx-psi
Copy link
Member Author

mx-psi commented Mar 1, 2023

@lootek can you open a separate issue for this? I can list it on this meta issue once opened. Based on golangci/golangci-lint/issues/3094 it looks like this is not available on golangci-lint but we could run it separately if this is useful

@lootek
Copy link
Contributor

lootek commented Mar 1, 2023

@mx-psi, Good point, thanks. It's done.

Do you think it makes sense to add all the components listed one by one? Since the linter seems to be passing right now it should be straightforward. Or should I list them anyway?

@mx-psi
Copy link
Member Author

mx-psi commented Mar 1, 2023

If it currently passes without errors I think it's fine to just enable it.

@lootek
Copy link
Contributor

lootek commented Mar 1, 2023

Yeah, it's green. Though govulncheck runs against the current Go toolchain / std lib version so the question is which version do we use here in GH actions. I looked for that and it's not defined once but several times as 1.19.6 (and that'd be great) and many times as just 1.19 (with no clue if it's gonna take 1.19.0 or the highest available 1.19.x).

@mx-psi
Copy link
Member Author

mx-psi commented Mar 1, 2023

Let's pin it to 1.19.6, that makes more sense to me

@github-actions
Copy link
Contributor

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

@github-actions
Copy link
Contributor

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

  • issue: Github issue template generation code needs this to generate the corresponding labels.

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@fatsheep9146
Copy link
Contributor

@mx-psi I think this issue is finally ready to be closed :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-cd CI, CD, testing, build issues good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants