go.mod is out of sync a lot

[jmhodges]

I keep merging in master to a branch I'm making for a patch, and I've found that I'm consistently seeing the post-commit hooks causing changes to go.mod after a merge that (haven't been committed). It seems like maybe the CI jobs aren't detecting or folks aren't committing changes to go.mod somehow?

It would be cool to resolve this.

[jmhodges]

Currently, for instance, my local go.mod differs from master with:

-       github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
-       github.com/modern-go/reflect2 v1.0.1 // indirect

[sougou]

This has been bugging me a lot. Are we using go modules the wrong way? Is there a more stable way to do this? Any inputs welcome.

[jmhodges]

I think it's the use of go run commands without versions in the commit hooks. Everyone's machine is picking up different versions of dependencies for those packages. (That plus folks having different GO111MODULE settings?)

[gedgar]

FWIW, just running make build is enough to introduce those changes to go.mod. Per go mod tidy -v, those modern-go/ deps (and many others) appear to be unused:

$ go mod tidy -v
warning: ignoring symlink /home/gary/go/src/vitess.io/vitess/py-vtdb
unused github.com/Bowery/prompt
unused github.com/boltdb/bolt
unused github.com/bombsimon/wsl
unused github.com/cockroachdb/cmux
unused github.com/dchest/safefile
unused github.com/go-critic/go-critic
unused github.com/golangci/gocyclo
unused github.com/golangci/golangci-lint
unused github.com/golangci/revgrep
unused github.com/google/shlex
unused github.com/gostaticanalysis/analysisutil
unused github.com/mattn/go-isatty
unused github.com/modern-go/concurrent
unused github.com/modern-go/reflect2
unused github.com/pelletier/go-toml
unused github.com/securego/gosec
unused github.com/spf13/afero
unused github.com/spf13/cast
unused github.com/spf13/jwalterweatherman
unused github.com/spf13/viper
unused github.com/ugorji/go
unused github.com/uudashr/gocognit
unused mvdan.cc/unparam
unused sourcegraph.com/sqs/pbtypes

I admittedly don't have much experience with go modules but took a stab at cleaning things up in #5765

go.sum looks to be consistent after running both go mod tidy and make build, though there is a descrepancy in go.mod related to ugorji/go:

go mod tidy: github.com/ugorji/go/codec v1.1.7 // indirect make build: github.com/ugorji/go v1.1.7 // indirect

Again, this is outside my wheelhouse so I'll defer to someone who actually has experience with go modules :)

[jmhodges]

The make commands run various go run things, don’t they? And tidy won’t ever see those go run commands as things to add to go.sum. That would explain what you’re seeing, I believe

[jmhodges]

I'm poking at this some, and specifying versions for go run commands won't help because mod tidy still won't see those commands.

I think we'd be better off requiring that developers install certain commands in $PATH to work correctly. We maybe could a helper script to do so that ensures they don't accidentally fudge with go.mod when they install those tools.

[jmhodges]

Hrm, we already have a tools package that would be preventing some of this (since it's used to depend on our tooling), but we kind of undo its work by relying on go run, because it can update go.mod itself.

So, I'd recommend ripping out the go runs, start relying on the commands in $PATH for our hooks, and mention installing tools in more of the contributor documentation.

As sugar, it would be nice if the CI jobs could error if go.mod changes during the course of their run and instruct the developer to update go.mod in their patch.

(Currently, there's no useful for us solution in the Go tooling. In Go 1.14, they're adding a -g command to go get, go run, etc. that skips over go.mod entirely. That, of course, would mean if we keep the go runs but add -g, everyone would have different versions of the tools on their machines. But, in any case, it's moot because 1.14 is not out)

[dweitzman]

I haven't been working with the go mod stuff yet, but I notice that CI still doesn't have mod=readonly, which would create some degree of a security and maintainability problem

That was going to be deferred for a future PR when the original go.mod landed: https://github.com/vitessio/vitess/pull/5109#issuecomment-528034358