Migrate to go module

[honnix]

Why

• everything of flyte is on go-1.13 and go module has become mature to use in production
• it seems to be the future
• one fewer tool (dep) to install and manage
• no more vendor folder
• I tried the migration and it was smooth migrate to go module datacatalog#21

[wild-endeavor]

We should definitely do this, it's just a question of when and how to get everyone on our team on the same page. It's pretty high priority I think, if only for conformity to major K8s/go projects. I'll try to look into it soon.

[honnix]

Awesome! Thanks.

[honnix]

To collect related PRs:

• migrate to go module datacatalog#21
• Migrate to go mod flytepropeller#38
• Migrate to go module flyteadmin#35
• Migrate to go module and remove old Hive integration tests flyteplugins#36
• migrate to go module flytestdlib#50
• incorporate latest boilerplate changes flytestdlib#53 (incorporate boilerplate change)
• migrate to go module flyteidl#27 (depends on migrate to go module flytestdlib#50 to be merged and released)

[honnix]

I think I have made everything work (or will be working). Shall we get it going before new dependency updates introduced (e.g. flyteorg/flyteadmin#37)?

[wild-endeavor]

Yes, we should. I want to look at the whole things one more time, across all the repos. Are there still issues with flytepropeller and flytestdlib? Or have those been resolved? I think the pflags change is still outstanding on our side.

[honnix]

Absolutely. All the problems have been resolved. Flyteidl needs a new build of pflags to work.

[honnix]

go module bugs and limitations are a bit more than i anticipated. :(

[wild-endeavor]

Discussed some more with @EngHabu on Friday. It seems that the primary goal of both your alternate change, and the change going into go 1.14 is the ability to install specific versions of go tools (like lint, enumer, mockery, etc.). If this is the case, can we not just avoid all this hassle by just pinning the version of each of the tools when running go get (and running it in an empty directory)?

[honnix]

Sure we can do that. The benefit having everything in one file is easier to track version changes and less error prone (think about go get foo@v1 in one script and go get foo@v2 in another). But yes this is a trade-off.

[wild-endeavor]

@jonathanburns would you be okay with that going into boilerplate?

[honnix]

During the work migrating from dep to go module, there are a few substantial issues that need to be documented.

There is a nice summary on GitHub worth reading as well because most of the issues I have also personally experienced.

Missing global flag to install tools for CI alike purpose

Very often support tools such as golangci-lint and mockery are needed to compile and apply checks, however due to cmd/go: offer a consistent "global install" command · Issue #30515 · golang/go and there is no good way to install such tools without modifying go.mod and go.sum.

In comparison, other languages such as Javascript has npm install -g.

A possible workaround is illustrated by flyteorg/flytestdlib#51. However things get worse when the need of forking comes, for example: https://github.com/lyft/flyteidl/pull/27/files#r359164560

Another way going forward is to add support tools in go.mod and go.sum as well. However this suffers from go mod tidy because it removes those since they are not referenced by any go code. Requiring contributors not to use go mod tidy doesn’t seem to be an option, and carefully reviewing go.mod and go.sum to make sure they are not deleted accidentally kinda defeats the purpose using go module.

There was an attempted fix https://golang.org/cl/203279 but was abandoned in favour of https://go-review.googlesource.com/c/tools/+/211538/ which addressed a variety of issues covered by cmd/go: flags to control changes to go.mod, go.sum · Issue #34506 · golang/go. With this fix, one could maintain two sets of go.mod and go.sum and use go get -modfile build/go.mod -sumfile build/go.sum golangci-lint for example. The fix is scheduled to be released as part of go 1.14 that has its first beta out https://github.com/golang/go/releases/tag/go1.14beta1 already.

This is a pretty good fix in my opinion! We could maintain two sets of go.mod and go.sum, one for real code module management, one for support tools. I think if we don't want to wait for go 1.14, we can start maintaining those two sets and cd to the dir manually to install. However that means the set for support tools needs to be hand crafted and one can not run go mod tidy. As an example flyteorg/flyteidl#33. A possible hack was described in golang/go#25922 to make sure go mod tidy not wipe out everything. Shown by flyteorg/flyteidl@8ba0a6a

Inconsistency changes of go.sum

This boils down to cmd/go: flags to control changes to go.mod, go.sum · Issue #34506 · golang/go, although it is still weird go mod tidy does not clean up accordingly.

As illustrated by flyteorg/flyteidl@bdd8117, go.sum file is changed in an inconsistent way: TravisCI wanted those file tree checksum (such as github.com/Azure/azure-sdk-for-go v10.2.1-beta+incompatible h1:/x4W7ZQV4PHJYnLUgKubojM8T+zlFEDdaBazAnA/QCY=) but not my computer TM. On my computer go mod tidy is fine with either having it or not. ¯_(ツ)_/¯ It turns out there is an additional which pflags || (go get github.com/lyft/flytestdlib/cli/pflags) executed in CI and that results change of go.sum.

The purpose of having two checksums is explained by Module authentication using go.sum.

Some part of go doesn’t not work with go module

As illustrated by this change, importer doesn’t work with go module and the official recommendation is to use golang.org/x/tools/go/packages instead.

There could be other things not working in a similar way, but hopefully we will not need to touch those.

Installing CLI provided by the module itself

Illustrated by this change

[wild-endeavor]

@honnix do you think you could change the idl pr one last time to just separate out the go gets into a separate script? Or alternatively make the change from this branch flyteorg/boilerplate#4 and resubmit.

[honnix]

@wild-endeavor I have a few open comments on flyteorg/boilerplate#4 . Should we get those addressed first?

Updated:

Tried in flyteorg/boilerplate#5

[EngHabu]

Awesome! I'm so glad we reached some conclusion here and that it's in boilerplate! are you in the process of updating all repos to use that?

[wild-endeavor]

I don't know if we have a solution to the pinning & replacement. At least in the brief amount of time I looked at it, I could not get go get to install the version of mockery in your fork @EngHabu

[honnix]

I tried that before but no success. @wild-endeavor

[honnix]

How is it going with this?

[honnix]

I did another round of rebasing and cleanup, as well as incorporating boilerplate changes. I think we should really get this going. It's painful to catch up master changes. :(

[wild-endeavor]

Yes, I know it's super painful to keep updating everything all the time. I'm very sorry. We've been super behind with our own review cycle. My only concern is that the boilerplate changes in the PR won't pick this up right? https://github.com/lyft/flyteidl/pull/27/files#diff-37aff102a57d3d7b797f152915a6dc16R31

@EngHabu and I think we should actually opt for https://github.com/lyft/flyteidl/pull/33/files. This is the most stable/flexible approach. As you mentioned, it's the same approach that will be part of 1.14, allows the replace statement construct, pin versions, and allows us to run go mod tidy if we want to without removing indirect tooling requirements.

If we're in agreement, I can help propagate the changes to boilerplate.

CC also @jonathanburns

[honnix]

I would vote for flyteorg/flyteidl#33 as well.

[wild-endeavor]

Just to mention here, flyteorg/boilerplate#4 was merged earlier today. @honnix could you help me pull these changes in? I've already pushed a couple commits to your forked PR of flyteadmin and flyteidl but would you mind doing the others?

(i messed up the flyteidl one a bit which is why there are four commits)

[honnix]

All PRs updated according to latest boilerplate and green now.

[honnix]

As we are approaching the end of this journey, I think it makes sense to summarise the changes.

tl;dr, this is all about simulating go get -modfile build/go.mod -sumfile build/go.sum golangci-lint which will be included in go-1.14.

Details

• There is a separated set of go.mod and go.sum offered by golang_support_tools
• tools.go is used to force go module keeping track of the packages, so that go mod tidy wouldn't wipe out everything
• download_tooling.sh is used by a Makefile target (make download_tooling) to download all the tools; to be extra careful, we first copy go.mod and go.sum to a temp folder and then install the tools from there
• Every time we bump of version of a tool, we will run go mod tidy to get new checksum in place

[wild-endeavor]

As of today, we have migrated all golang Flyte repos to use go mod. Thanks to @honnix for driving this change.

Boilerplate changes: flyteorg/boilerplate#4
The boilerplate code will now default to go.mod.

https://github.com/lyft/flytestdlib/releases/tag/v0.3.0 (was already on go mod as of v0.2.29)
https://github.com/lyft/flyteidl/releases/tag/v0.17.0
https://github.com/lyft/flyteplugins/releases/tag/v0.3.0
https://github.com/lyft/flytepropeller/releases/tag/v0.2.0
https://github.com/lyft/flyteadmin/releases/tag/v0.2.0
https://github.com/lyft/datacatalog/releases/tag/0.2.0

We’ve introduced a minor workaround to the boilerplate repo (and hence to all other go repos).  Please read through the thorough background discussion that @honnix posted around go modules in the GitHub issue (https://github.com/lyft/flyte/issues/129). In order to persist and standardize on a version of the common golang tools, we’ve added a second set of go.mod/sum files. These are listed in the boilerplate PR and basically separate your code from these tools. That is, your code likely uses golangci-lint but that doesn’t mean it should be part of your project’s go.mod files. Honnix’s comment has more information. This dual go.mod approach is also slated to be introduced more formally and rigorously in go 1.14.

This is a good overview of the new commands:
https://github.com/golang/go/wiki/Modules#daily-workflow

Note also that in most repos, we’re now pinning the version of client-go to something compatible with the Lyft fork of K8s (see flyteorg/datacatalog#21 (comment) for more information).