chore: added formatting check to circle-ci

[FUSAKLA]

Signed-off-by: Martin Chodur [email protected]

Added check for code formatting to circle-ci

Changes

• added check to circle-ci config
• added to formatting suggestion to contribution.md

[bwplotka]

LGTM, but can we add proto as well.. BTW I thought we are doing this already (:

[FUSAKLA]

One more thing.. I ran the make format and commited it and it still failed.

I suppose that it's caused by some different versions?

for example in circle-ci vs my setup the issue was

[bwplotka]

mhmhm. That's might be golang version issue ):

[bwplotka]

which could make it quite difficult to contribute for people if we require certain golang version.

However it might be the case that go1.11 vs go1.10 differs only, and seems like supporting go1.11 and above could work.

[bwplotka]

Ignore me, it just a matter of pinning goimports as we pin deps I think (:

Do you know how to do it? It's quite easy - just mimick what dep have in Makefile (: Let me know if you have questions!

[FUSAKLA]

hmm looking at the Makefile and goimports is fetched using

@go get -u golang.org/x/tools/cmd/goimports

I wasn't able to find any mention on goimport versions.. it looks like it's not versioned at all?

But I ran the make format after mounting the repo into two containers golang:1.10 and golang1.11 and the difference is exactly what I saw before in my case. And I checked and circle-ci uses go1.10 and I'm running golang1.11.3.

From those findings I'd suspect the formatting to depend on golang version as you said earlier. But I'm struggling to find any more info about this.

How to reproduce:

WARNING: stash or commit your changes before running this

docker run -it -v $PWD:/go/src/github.com/improbable-eng/thanos -w /go/src/github.com/improbable-eng/thanos golang:1.10 make format
git diff >  golang1.10.patch
git reset --hard HEAD
docker run -it -v $PWD:/go/src/github.com/improbable-eng/thanos -w /go/src/github.com/improbable-eng/thanos golang:1.11 make format
git diff >  golang1.11.patch
git reset --hard HEAD
diff --color  golang1.10.patch  golang1.11.patch

[bwplotka]

What about this? I think this will help: #737

Can you review? (:

[FUSAKLA]

Thanks, I totally overlooked the fetch_go_bin_version function 🤦‍♂️

But still, running in clean containers should have download the same revisions and still there were differences. But I'll rebase on your PR and try.
Thanks

[bwplotka]

Are you sure you used pinned version for make format as well?

[FUSAKLA]

Yes, I ran also directly the goimports-<commithash> and the same happens in the clean docker files as I posted before.

After some digging I found out that there were changes in formatting in 1.10 and 1.11 (for example see golang/go#26098 ).

Also https://tip.golang.org/pkg/go/format/ states

Note that formatting of Go source code changes over time, so tools relying on consistent formatting should execute a specific version of the gofmt binary instead of using this package. That way, the formatting will be stable, and the tools won't need to be recompiled each time gofmt changes.

I'm trying to find out if I goimports uses directly gofmt somehow or just some library so I can fix that dependency possibly.

[FUSAKLA]

So.. goimports and gofmt uses the go/format package which is part of the golang itself so bounded to the golang version unfortunately.

The Makefile.common in Prometheus already had this issue and the outcome was to support formatting with currently supported golang version which if is upgraded the formatting is upgraded also. See prometheus/alertmanager#1504 (comment)

Possibly formatting using the docker as I posted before is pretty easy also but that assumes docker installed.

[FUSAKLA]

Gee, I thought this will be just adding one line of yaml :D

[bwplotka]

Thanks for great research! This is so annoying...

So options:

1. I could try to get exact version of goimports in my pinning PR instead... but that would mean we need to download prebuild images based on architecture and OS you are developing for... quite annoying for people ): And there are no prebuild goimports package and it will add quite a complexity to the makefile etc...
2. Docker option might work, but again, complexity and as you said, for simple one line change you end up installing docker :/ tha't is no-go.
3. Some idea would be to stick to Go 1.11 (we might end up with this anyway, when with go mods) but as claimed in description, golang can check the fmt in future...
4. Eliminate all the places of inconsistency. It seems like the only change is with indent on long variable names. I think adding just newline would fix this ;p But that might be short-term fix and quite annoying if devs will hit this in future..
5. Downloading certain Golang (or even just go/format package) and build goimports with proper go/format. Quite complicated but doable I think.
6. Just don't add this check. Again this proven to be super annoying.

I think 4 or 3 might good for quick win here, but not sure yet.

5. one is doable, but hacky, worth to research?

[FUSAKLA]

2. could be optional for users with docker or add check if docker is installed
3. would require effor every time we upgrade the go version we stick to
4. I saw those workarounds also in those issues :) that's probably not long term solution

I'm looking at building goimports with specific go/format version now but with no success.

Downloading go sources for given version, extracting the go/format to vendor folder of the tools repo in go/format and building the the goimports but it seems to be ignoring it. Is there any chance of making this work? Next option is building expected go version but that seems to be huge overhead.

I'm not sure if I'm going the right direction. Will see I'll be off for few days now so hopefully will bet back to it asap.

[bwplotka]

Well, we could fork goimports basically if neccessary ):

[FUSAKLA]

I'd rather avoid forking if possible from the mIntainance point of view.
I'll try and see if I can make it work otherwise I'd personally go with one supported development version and formatting supported with the docker workaround. Or drop the formatting comletelly.

[jojohappy]

I think we could not run this command, because there is no protoc binary in CI container. In my container, I also installed the protocol buffer implementation from https://github.com/google/protobuf and ran make proto.

[bwplotka]

true, let's fix this in separate PR

[FUSAKLA]

I can download certain version of x/tools than download sources of go1.11 and extract from it src/go and vendor it in the x/tools/imports and x/tools/cmd/goimports. This MIGHT theoretically work but src/go contains also src/go/build which has some expectations from the go build command which is still from go1.10. So I'd have to build also the go tool itself and even if that would work it would be still fragile regarding any dependency changes or whatever. At that point I decided it's definitely not worth the effort and complexity (at least from my point of view) and gave up.. :/

I'd decide between:

• lock go development version to one (1.11) + support for docker if installed
• drop the formatting checks (in that case I'd vote for removing it at least from the default make target)

If anyone has other idea.

[bwplotka]

Let's abort it for now... When we moved to go modules we would need to pin to 1.11+ so we can decide later.

Thanks for this deep research @FUSAKLA!

[adrien-f]

Looking at other projects about this issue in golang/go#26228 you can find some debated arguments on those issues:

• Bump golang to 1.11.0 moby/moby#37358
• Workaround for gofmt change in Go 1.11 containerd/containerd#2436

And based on https://tip.golang.org/doc/go1.11:

Note that these kinds of minor updates to gofmt are expected from time to time. In general, systems that need consistent formatting of Go source code should use a specific version of the gofmt binary. See the go/format package documentation for more information.

[FUSAKLA]

Eventually I found the https://github.com/alecthomas/gometalinter which could possibly solve our issue.

It already vendors in all the linters (check out the list there is loads of them), runs them in parallel and allows to configure them all using one JSON. The only downside it that it only checks the code. It doesn't modify it. Also installation is by downloading prebuilt binaries of those linters.

GOMETALINTER_INSTALL_SCRIPT=https://raw.githubusercontent.com/alecthomas/gometalinter/master/scripts/install.sh
curl -sL $GOMETALINTER_INSTALL_SCRIPT | BINDIR=/go/bin TAG=v2.0.12 sh
gometalinter --deadline 10m --disable-all --enable=goimports [--enable possibly many other checks]

This is all that is needed to check consistently on multiple golang versions.
As mentioned before this would just tell you it's not properly formatted but at that point you already have also prebuilt binaries of those linters so you can than run them by yourself.

[adrien-f]

Could we not use that gometalinter in the CI/pre-commit step and still use the individual tools installed by this as make commands so we could still fix the detected issues 🤔 ?

[FUSAKLA]

Yes we can use those pre-built binaries as standalone commands and don't use the gometalinter.
The only thing it adds is the unified configuration and concurrency.
(also it provides those binaries in one tgz having cca 45MB)

[bwplotka]

(also it provides those binaries in one tgz having cca 45MB)

Yea, downloading them might be better, but the problem is that.. gometalinter does not use goimports at the very end anyway? enable=goimports looks like it uses goimports in underlying code... but did not research it.

[FUSAKLA]

closing this in favor of #1227