Issue using golangci-lint due to breaking change in v1

[kylebrandt]

... I think. I am mighty confused by go modules.

In golangci-lint I am getting the following problem with their v1.21.0 tag:

# github.com/golangci/golangci-lint/pkg/golinters
/go/pkg/mod/github.com/golangci/[email protected]/pkg/golinters/wsl.go:43:6: unknown field 'AllowCaseTrailingWhitespace' in struct literal of type wsl.Configuration

This build worked before on the same commit of my code. I noticed my builds are using different versions of WSL even though they grab the same version of golintci-lint.. The build had worked with wsl v1.2.5. However, now by build is get v1.2.7 of wsl.

I suspect that go modules believes this is fine, because v1.2.7 should be backwards compatible to v1.2.5 if you are not using any new fields according to semantic versioning.

However, when I git diff v1.2.6 v1.2.7 wsl.go I see that the exported configuration property AllowCaseTrailingWhitespace was removed/renamed to CaseForceTrailingWhitespaceLimit, which is a breaking a change:

diff --git a/wsl.go b/wsl.go
index 8a1f45b..ea8d4eb 100644
--- a/wsl.go
+++ b/wsl.go
@@ -50,21 +50,9 @@ type Configuration struct {
        //  }
        AllowMultiLineAssignCuddle bool
 ...
-       AllowCaseTrailingWhitespace bool
+       // If the number of lines in a case block is equal to or lager than this
+       // number, the case *must* end white a newline.
+       CaseForceTrailingWhitespaceLimit int

So I believe that for Go modules, that change would have need to have been v2.x.x and using a new a package folder with v2 at the end of the folder path (or module declaration) as per https://github.com/golang/go/wiki/Modules#releasing-modules-v2-or-higher

[bombsimon]

@kylebrandt Thanks for the report!

This confuses me as well... Especially since there’s no new release of golangci-lint. But you are right either way, this is in fact a breaking change. Feel free to propose a solution. Do you think I should revert golangci-lint and re-release wsl as v2.x? Any idea how to solve this without a new pr in golangci-lint repo?

I’ll look into this first thing tomorrow to find a quick solution to this problem. I’m sorry for any inconvenience this resulted in.

[kylebrandt]

I'm not really sure of the best way forward. In terms of not touching the golangci-lint and making it so golangci-lint's existing version will work again - I think/maybe/could be totally wrong:

Restore the AllowCaseTrailingWhitespace option with its old behavior alongside the new option and release this as v1.2.8. This makes v1.2.7 a bad release I guess, but then builds would just be grabbing the latest (1.2.8) (if I understand correctly) and things will work again.

You have some logic in your code that makes these options exclusive if need be. Adding a new Field to the config should be fine I believe, because when the struct is imported it just isn't doing anything with the new field, but no existing code will be trying to reference a property on the struct that doesn't exist.

Edit: Oh, but to follow semvar I think it would be v1.3.0 -> "MINOR version when you add functionality in a backwards compatible manner" as per https://semver.org/ . So maybe the correct thing would be to do v1.2.8 removing the new feature, and restoring the old. And then v1.3.0 for the new functionality alongside the old. But I don't think the minor version matters for how go modules work? not positive on that. If the minor version doesn't impact go modules and everything works the way I think it does, and you can do the new feature side by side with the old, then just go straight to v1.3.0.

[bombsimon]

Thank you so much for your help and input!

I'll try to solve this as quick as possible. I know it doesn't matter now that it's broken but I thought one of the main reasons that golangci vendors and commits all the modules was to avoid this. Just to be sure, how do you do when you build golangci-lint? If I go to the v1.21.0 tag and build it manually I don't have this problem since the old version of wsl is vendored. I also tried the proposed Docker and curl installations/runs from the documentation:

# Docker
docker run --rm -v $(pwd):/app -w /app golangci/golangci-lint:v1.21.0 golangci-lint run -v

# Curl instllation
curl -sfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh| sh -s -- -b $(go env GOPATH)/bin v1.21.0

This works fine. Even though it's not the recommended way I tried to just install it with go get:

go get -u github.com/golangci/golangci-lint/...

This seems to be working fine for me as well so I tried to check out the v1.21.0 tag and remove the vendor package and run go install ./... to see what happened and I still cannot reproduce this issue. 🤔

So I definitely agree with you that this is bad and that my tagging should follow proper semver but right now I want to ensure that this is working and to see if it's fixed I would like to be able to reproduce the issue.

I'll try a fresh install of a different OS and see if I can reproduce this.

EDIT: Oh, I see now in your Makefile how golangci-lint is installed. I'll try that and see what happens. I know that this isn't really the right solution to my mistakes but according to golangci-lint documentation the recommended way to install golangci-lint for a given tag at GOPATH is:

curl -sfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.21.0

If you're OK introducing a dependency (curl).

[bombsimon]

Ok so I can reproduce the issue if I clone grafana-plugin-sdk-go (or just in any other module) executes the (not recommended) go get command.

I wish I completely understood how go modules work and why/who/where the latest version is being fetched because with the current go.mod this is my output:

# go get -u github.com/golangci/golangci-lint/cmd/[email protected] 2>&1 | grep wsl
go: downloading github.com/bombsimon/wsl v1.2.5
go: extracting github.com/bombsimon/wsl v1.2.5
go: finding github.com/bombsimon/wsl v1.2.7
go: downloading github.com/bombsimon/wsl v1.2.7
go: extracting github.com/bombsimon/wsl v1.2.7

So obviously I get the v1.2.5 tag which is later overwritten for some reason.

Anyway, I can now reproduce this issue and if I tag golangci-lint with release @mater this is working because it's compatible with v1.2.7. I think I'll start with creating a new release which can compile and tag it v1.2.8. It might not be 100% 1:1 backwards compatible but the changed features does somewhat perform the same thing. I'll do this to give me some time to figure out a proper release path and in the end do a proper PR to golangci-lint with a non breaking/major/minor release.

[bombsimon]

So everything seems to be working properly now with the v1.2.8 release I made.

# cd $(go env GOPATH)/src/github.com/grafana/grafana-plugin-sdk-go \
  && make lint \
  && echo $?
go vet ./...
[...]
/root/go/bin/golangci-lint run ./...
/root/go/bin/revive -exclude ./vendor/... -formatter stylish -config scripts/configs/revive.toml ./...
/root/go/bin/gosec -quiet -exclude=G104,G107,G108,G201,G202,G204,G301,G304,G401,G402,G501 -conf=scripts/configs/gosec.json ./...
0

@kylebrandt Again sorry for this and thank you for helping me out! I see that you made this commit to be able to move forward which I missed initially. I'm glad you found a way to not be blocked by me.

I think that proper way to do would be to re-release master on a new release (v2.0.0 since there are breaking changes) and then revert the commit in golangci-lint and create a new one with the proper tag. All this depends on how quickly I can get this done and that golangci-lint doens't create any new releases. This might not be the best solution but I'm having a hard time to see why I would keep v1.2.7 or v1.2.8 since they're in practice bad releases not following semver.

[kylebrandt]

Well I'm dumb. It is the -u flag on go get in my makefile. grafana/grafana-plugin-sdk-go#18 (comment)

The issue still stands for proper semvar. But sounds like less of a rush - apologies for confusion. Still groking modules myself :-/

[bombsimon]

@kylebrandt Haha wow, I missed that too and I even read the comment without actually taking in what he asked.

You are right and have nothing to apologise for. I'm glad you noticed it this early and with such a small change, I feel equally dumb not able to produce proper semantic versioning.

I've sent an email to one of the collaborators on the golangci-lint project to ask what he think is the proper way forward. I've prepared a new branch reverting the update to v1.2.7 and adding a proper update to v2.0.0 instead.

[bombsimon]

@kylebrandt revert + proper tag now merged in golangci-lint. Sadly I can’t fix the bad releases but you should not be able to get the same issue no matter how you install or if you use the -u flag.

Thank you for noticing and reporting this and again, sorry for any inconveniences. At least it taught me a bit about go modules. :)