Update golangci-lint to v2 #4037
Conversation
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
| # https://golangci-lint.run/usage/configuration/ | ||
| version: "2" | ||
| run: | ||
| timeout: 10m |
There was a problem hiding this comment.
v1 had a default timeout of 1m so we had to bump it up since our linting would sometimes take a while. V2 has no timeout by default, so this is no longer an issue. You can configure the tiemout via a parameter when running golangci-lint now
| # Check for plain error comparisons. | ||
| comparison: false | ||
| forbidigo: | ||
| # Forbid the following identifiers (list of regexp). |
There was a problem hiding this comment.
Comments aren't migrated by migrate... so I could migrate the comments by-hand if someone requests me to.
| # - gomnd | ||
| - goprintffuncname | ||
| - gosec | ||
| - gosimple |
There was a problem hiding this comment.
A lot of these linters (gosimple, stylecheck, etc) got merged into staticcheck as part of v2
|
Good to see this update, but installing things from homebrew is not ideal. We want to enable local reproducibility of CI, and that requires ensuring the same versions in both places and we don't use homebrew in CI. Any interest in porting over the work I did in hypersdk to enable versioned install of golang tools? |
|
Yeah I realized that this was also breaking in nix since it seems like the version of golangci-lint on nix is also on v2. Our linter script works because it uses go install on the v1 version. I do find this inconsistent behavior annoying so I do think it would be nice if we could have versioned installs. |
|
Would it make sense to break this PR up so that a given linter and its changes could be reviewed one-by-one (i.e. by upgrading in the initial PR but disabling failing linters)? I worry that the lack of traceability for any given change makes it all but impossible for a reviewer to do a good job. |
|
Something like that could be possible. I could probably do something where we have a initial PR to migrate + any failing linters disabled, and have subsequent stacked PRs per-linter that we fix and merge down into this branch. Is that what you had in mind? |
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
joshua-kim
force-pushed
the
golangci-migrate
branch
from
e5c5c14 to
695e6f9
Compare
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
.github/workflows/ci.yml
Outdated
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: umbrelladocs/action-linkspector@a0567ce1c7c13de4a2358587492ed43cab5d0102 #v1.3.4 | ||
| - uses: umbrelladocs/action-linkspector@v1.2.5 |
There was a problem hiding this comment.
So this line looks pretty weird. This seems to be an issue on v1.3 of this action (ref). Seems like the work-around it to downgrade in the meantime. It seems like this failed on this PR because this action only runs when *.yml and *.md files are changed by default. I'm not entirely sure why it broke on this specific PR, since we have made changes to other *.md and *.yml files before.
I re-ran the job MANY times and it did not pass until this was downgraded... so it does seem like something is actually wrong here.
There was a problem hiding this comment.
Please update to use the same SHA #version format used for the previous version.
.github/workflows/ci.yml
Outdated
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: umbrelladocs/action-linkspector@a0567ce1c7c13de4a2358587492ed43cab5d0102 #v1.3.4 | ||
| - uses: umbrelladocs/action-linkspector@v1.2.5 |
There was a problem hiding this comment.
Please update to use the same SHA #version format used for the previous version.
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Why this should be merged
The latest version of golangci-lint (which is on homebrew/nix) uses v2, which is not compatible with the v1 schema
How this works
updates our schema using
golangci-lint migrate.How this was tested
Ran
golangci-lint --fixNeed to be documented in RELEASES.md?
Yes