search

riverqueue / river

Fast and reliable background jobs in Go
https://riverqueue.com
Mozilla Public License 2.0
3.34k stars 89 forks source link

Upgrade to golangci-lint 1.60.1 #548

Closed brandur closed 3 weeks ago

brandur commented 3 weeks ago

Here, take the project to golangci-lint 1.60.1, which is now widely available on Homebrew and elsewhere. It comes with a number of breaking changes, so I figure it's not the worst idea to keep us on latest so that future upgrades are less painful.

All breakages are related to overflow checking in gosec getting more strict. The strictness is kind of useful in that it reminds us to check bounds with something like min(limit, math.MaxInt32), but it can't detect that a bounds check is done with min(...), which is kind of annoying because it means you need a nolint tag on all of these.

ccoVeille commented 3 weeks ago

A lot of nolint you add are caused by the gosec G115 that was added recently and that are currently being reviewed as it causes a lot of problems

https://github.com/securego/gosec/pull/1149#issuecomment-2302227236 Or here https://github.com/securego/gosec/issues/1185

I would wait a few days if I was you

Side remark,I thought the gosec G115 was only added in golangci-lint 1.60.2

brandur commented 3 weeks ago

Ah interesting. I'm glad they're reconsidering. There's some overflow check logic that's clearly just wrong (e.g. uint8 -> int64).

ccoVeille commented 3 weeks ago

Ah interesting. I'm glad they're reconsidering. There's some overflow check logic that's clearly just wrong (e.g. uint8 -> int64).

This one was a bug

https://github.com/securego/gosec/pull/1188

Length 2, so 8 was not caught

brandur commented 3 weeks ago

@bgentry Actually, can we move forward with this? Even if some of the gosec stuff is retooled, it's not like golangci-lint 1.60.2 is going to re-released. If they do anything, they'll roll forward to a new 1.60.3, at which point we can upgrade and remove some nolints if they're no longer necessary. This is basically blocking me from running lint locally because linting always fails on the top level package, so make lint can't descend into any of the submodules.

brandur commented 3 weeks ago

ty.

Yeah I checked the other projects and a couple fixes will be required, but they're similar to these ones and should only take a couple min.