WIP: Add golanci/golint to github actions

[Waterdrips]

Add golangci linting to ensure consistent code standards.

Signed-off-by: Alistair Hey [email protected]

[Waterdrips]

adding error checking may have uncovered a bug - going to take a look a little later

[amnonbc]

Not very useful to panic in this situation.
An error when closing a response.Body can be ignored - the client has gone anyway.

This is an example where using golangci-lint does not improve the code,
but makes it unnecessarily convoluted.

The Go stdlib wil notpass golangci-lint.

[amnonbc]

This PR does not improve the code.
The choices made by golangci are questionable.

I would run go vet, but not golangci.

Also I would remove support for any versions < 1.15

[oxisto]

This PR does not improve the code.
The choices made by golangci are questionable.

I would run go vet, but not golangci.

I agree with you, that golangci should be taken with a grain of salt. So maybe it would make sense to start with a go vet approach, fix the most "critical" code smells and go from there. Would you mind preparing such a PR with just go vet for now? We can then see how much the 'delta' is going from go vet to golangci.

Also I would remove support for any versions < 1.15

This is something that we need to be very careful about in my opinion. The original project did not have a go.mod file and we also do not have one (for now, see #17 for a longer discussion on that), until we go for a v4 branch. So it is quite hard for us to understand what the minimum required go version was for most people using it. We want to provide a smooth transition path and will probably continuously phase out older Go versions as we introduce new PRs that rely on new language features / constructs.

[amnonbc]

This PR does not improve the code.
The choices made by golangci are questionable.
I would run go vet, but not golangci.

I agree with you, that golangci should be taken with a grain of salt. So maybe it would make sense to start with a go vet approach, fix the most "critical" code smells and go from there. Would you mind preparing such a PR with just go vet for now? We can then see how much the 'delta' is going from go vet to golangci.

We should fix all the warning from go vet.

And I would argue that golangci should be abandoned, as it includes too many questionable
tests, and has a very low signal to noise ratio.

Also I would remove support for any versions < 1.15

This is something that we need to be very careful about in my opinion. The original project did not have a go.mod file and we also do not have one (for now, see #17 for a longer discussion on that), until we go for a v4 branch. So it is quite hard for us to understand what the minimum required go version was for most people using it. We want to provide a smooth transition path and will probably continuously phase out older Go versions as we introduce new PRs that rely on new language features / constructs.

The current Go version is 1.16. 1.17 is now in public beta and should be released within a month or two.
1.15 is still maintained. And anything below 1.14 is no longer supported (even for security fixes).
Nobody should be using a version of Go below 1.15.
So I do not understand what you mean by "minimum version requirement". Anybody using an obsolete version
of Go should upgrade. There is no excuse not to.
And if anyone is not prepared to upgrade the Go toolchain, then why would they want to upgrade their jwt library?
And why should we support such irrationality?

[oxisto]

This PR does not improve the code.
The choices made by golangci are questionable.
I would run go vet, but not golangci.

I agree with you, that golangci should be taken with a grain of salt. So maybe it would make sense to start with a go vet approach, fix the most "critical" code smells and go from there. Would you mind preparing such a PR with just go vet for now? We can then see how much the 'delta' is going from go vet to golangci.

We should fix all the warning from go vet.

And I would argue that golangci should be abandoned, as it includes too many questionable
tests, and has a very low signal to noise ratio.

Also I would remove support for any versions < 1.15

This is something that we need to be very careful about in my opinion. The original project did not have a go.mod file and we also do not have one (for now, see #17 for a longer discussion on that), until we go for a v4 branch. So it is quite hard for us to understand what the minimum required go version was for most people using it. We want to provide a smooth transition path and will probably continuously phase out older Go versions as we introduce new PRs that rely on new language features / constructs.

The current Go version is 1.16. 1.17 is now in public beta and should be released within a month or two.
1.15 is still maintained. And anything below 1.14 is no longer supported (even for security fixes).
Nobody should be using a version of Go below 1.15.
So I do not understand what you mean by "minimum version requirement". Anybody using an obsolete version
of Go should upgrade. There is no excuse not to.
And if anyone is not prepared to upgrade the Go toolchain, then why would they want to upgrade their jwt library?
And why should we support such irrationality?

I am not trying to start a heated discussion about versioning policies here, but explain a little bit the reasoning behind my remarks.

Me, personally: I am a big favour of always updating as soon as possible and I am a big fan of dependabot, renovate, etc. to always pull in the newest versions (decent test coverage needed of course).

Me, as one of the maintainers of this library: I try to not be opinionated about the usage of this library or the constraints the users might have with regards to versions. The goal was (and still is) to provide a 1:1 drop-in replacement for https://github.com/dgrijalva/jwt-go while fixing some of the known issues. Since we are taking "over" existing work, we wanted to follow the ideas of the original author (and other contributors) as close as possible. Without reading their minds, that is a hard thing to do, so a lot of it is some guess work or trying to read what was written in the various documentation files. We have done that with the v3.2.1 release.

What I meant with "minimum version requirement" is that the jwt-go library never specifically mentioned any minimum Go version, especially because no go.mod file is present. From the point of view of the code, the minimum version that still compiles is 1.7. I agree with you, that it is not a "good" thing to do, to run this old Go version. But once again, I am trying to not be opinionated about it.

Now, as said before, moving forward, we will most likely drop older versions, mainly because we want to use features of the newer language version. We also do not want to discourage people from creating PRs with newer versions. So this is a tricky thing and it will probably take some time to be properly adjusted.

I hope that clarifies my standpoint here a little bit better.

[amnonbc]

Go is an opinionated language, and the Go community is opinionated when it comes to supported versions.
There is zero benefit in using obsolete versions or supporting them.
More concretely, continuing to support 1.14 and earlier prevents us merging
dgrijalva/jwt-go#455 which fixes the library's pathologically slow handling of
ECDSA verification.

[ripienaar]

It's common to support only the 2 most recent major releases of Go.

I can't pretend to have studied all of these vulnerabilities - but Go do have a fair few vulnerabilities in the langauge and I'd rather signal our support of newer versions via go mod.

We made a release that fixes the vulnerability and provided a drop in replacement, seems reasonable to tighten things up moving forward to me

[ripienaar]

Regarding linters, go vet is decent others is noisy and questionable, used to like staticcheck but even that is turning into a rubocop like cancer causing constant churn in code bases for no good reason.

[amnonbc]

It's common to support only the 2 most recent major releases of Go.

I can't pretend to have studied all of these vulnerabilities - but Go do have a fair few vulnerabilities in the langauge and I'd rather signal our support of newer versions via go mod.

We made a release that fixes the vulnerability and provided a drop in replacement, seems reasonable to tighten things up moving forward to me

Yes, we have to take security very seriously in a JWE library.
We can not base the implementation on Go versions that no longer get security fixes.

[lggomez]

Regarding linters, go vet is decent others is noisy and questionable, used to like staticcheck but even that is turning into a rubocop like cancer causing constant churn in code bases for no good reason.

I agree that linters an be a PITA but I like to see the static analysis side of it as many of their analyzers aren't style only and help catch bugs before even showing at runtime. How bureaucratic the process is is up to the linter's configurability and the codebase's configuration

That said, I'd like to continue with some static analysis step (be it golangci-lint or staticcheck) but with a custom configuration for this project that suits the codebase and marks the really important findings

[mfridman]

Historically I've used/encountered staticcheck in most projects, my vote goes to staticcheck. But I don't have a strong opinion... golangci-lint is just fine too.

One way or another, having something is probably better than nothing at least to conform to some "style", which we'll probably figure out with time.

[oxisto]

Historically I've used/encountered staticcheck in most projects, my vote goes to staticcheck. But I don't have a strong opinion... golangci-lint is just fine too.

One way or another, having something is probably better than nothing at least to conform to some "style", which we'll probably figure out with time.

I played around with staticcheck in another PR (#44). Looks ok so far, but it seems that not even the mismatched documentation strings are caught with just this approach; which is weird because I thought that go vet would catch it.

Update: this was a configuration issue, the comment checks (which I really like) were not enabled by default

[Waterdrips]

closing because it was contentious.