snapd icon indicating copy to clipboard operation
snapd copied to clipboard

run-checks: add `golangci-lint` to static checks

Open olivercalder opened this issue 1 year ago • 5 comments

golangci-lint is run as part of Github workflows on pull request, but it is not run as part of local testing via ./run-checks. This PR proposes adding it to ./run-checks, with automatic installation (assuming curl or wget).

Installation instructions for golangci-lint can be found here: https://golangci-lint.run/usage/install/#local-installation

Some other possibilities:

  1. Check the sha256 hash of the installed binary to make sure it has not been tampered with
  2. Install golangci-lint to /tmp or some other workspace which is later discarded (would make testing fail without internet -- bad)

olivercalder avatar Aug 01 '23 00:08 olivercalder

This relates to the commit here (https://github.com/snapcore/snapd/pull/13040) which fixes lint errors introduced here (https://github.com/snapcore/snapd/commit/b27425ab99c47cf8fc1b3bb8b0df96d143ea6fa3) which were not caught by local tests before pushing to master.

olivercalder avatar Aug 01 '23 00:08 olivercalder

Would it be preferable to use the snap of golangci-lint instead? I maintain this currently myself but would be happy to transfer it to Canonical and maintain it there instead if desired.

alexmurray avatar Aug 01 '23 02:08 alexmurray

Codecov Report

Merging #13042 (b66395a) into master (661258f) will increase coverage by 0.00%. The diff coverage is n/a.

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@           Coverage Diff           @@
##           master   #13042   +/-   ##
=======================================
  Coverage   78.74%   78.74%           
=======================================
  Files        1002     1002           
  Lines      124950   124950           
=======================================
+ Hits        98392    98395    +3     
+ Misses      20377    20375    -2     
+ Partials     6181     6180    -1     
Flag Coverage Δ
unittests 78.74% <ø> (+<0.01%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1 file with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

codecov-commenter avatar Aug 22 '23 22:08 codecov-commenter

Would it be preferable to use the snap of golangci-lint instead? I maintain this currently myself but would be happy to transfer it to Canonical and maintain it there instead if desired.

Thank you for the offer! That would certainly be the easiest solution. I think the most up-to-date version of golangci-lint shipped in the snap is more strict than the version currently used in the CI checks. I'm somewhat worried about ./run-checks failing as a result of a golangci-lint snap update, when no code changes have occurred in snapd. This would be particularly problematic when working on changes for a PR when an update occurs and various orthogonal fixes are required to fix linting errors before the work which is the focus of the PR can be continued. Thus, my thought was to match the version used for the CI. But perhaps it wouldn't be a bad thing to be more strict than the CI.

olivercalder avatar Aug 23 '23 13:08 olivercalder

Fwiw, I like the idea of using the snap better than using curl | bash. We could use "snap install --revision=xxx" if we want to pin to specific versions (or use tracks) if this is a concern.

mvo5 avatar Oct 12 '23 13:10 mvo5