Pin golangci-lint to v2.4.0

[philippschulte]

Change summary

Bump golangci-lint from v2.1.0 to v2.1.6 and also format the workflow file.

All Submissions:

• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?

[anthony-gomez-fastly]

Are there any notable changes associated with this bump? is there anything stopping us from going up to the latest release (2.4.0) ?

[anthony-gomez-fastly]

LGTM, do we need to have a changelog entry for this since it's technically a breaking change?

[kpfleming]

Yep, this changes the name of a non-internal package.

[kpfleming]

Since the CI history for this PR is gone, is there a link I can refer to in order to understand why this package name change was necessary?

[philippschulte]

@kpfleming the reason for this change is that I gave that package to route requests for ngwaf depending on scope a stupid name:

philipp@philipp-XPS-15-9500:~/src/fastly/go-fastly$ make lint
==> Running golangci-lint
fastly/ngwaf/v1/common/pathbuilder.go:1:9: var-naming: avoid meaningless package names (revive)
package common
        ^
1 issues:
* revive: 1
make: *** [Makefile:55: lint] Error 1

I was running a newer version of golangci-lint locally and noticed those changes. Unfortunately as you correctly pointed out this results in a breaking change which I like to avoid to all cost. Therefore, I have opened #756 to avoid situations like this in the future. Please keep in mind that a CI history goes away when you weigh in on #756. That's why I prefer to pin versions to be able to reproduce the issue and to know all the time what version runs in CI in order to be able to adjust my local instance accordingly.

[kpfleming]

In this case I would prefer to silence the 'revive' warning for this package name and just accept that it's not optimal, rather than causing a breaking change and having to adjust all the code which uses these interfaces in go-fastly.

[cee-dub]

Our code needs more internal/* packages to avoid this kind of headache.

[philippschulte]

In this case I would prefer to silence the 'revive' warning for this package name and just accept that it's not optimal, rather than causing a breaking change and having to adjust all the code which uses these interfaces in go-fastly.

@kpfleming since we will introduce a breaking change with the new release due to #755 I would like to move forward with this PR! The changes in terraform are straight forward and we only introduce a breaking change here. I don't want to apply the bad naming into the cli code base for your planed ngwaf work. I would like to avoid silencing the linter. Please reconsider. Thanks!

[kpfleming]

I'm fine with merging this as-is: normally I would not be willing to make a breaking change just because a linter claims that a package name is 'generic' but no actual problem is caused by that name. I do agree (as I noted in the other PR) that we should keep golangci-lint pinned to a specific version, but have a CI job which lets us evaluate the 'latest' version when we want to and then decide how to accommodate new suggestions/warnings it raises.

[rcaril]

LGTM