Add Go module support for pipelines. #1607

wlynch · 2019-11-22T20:35:34Z

Changes

This enables Go modules and removes the vendor directory from the
repository.

NOTE: highly recommend viewing this change with
git diff master -- ':!third_party' ':!vendor'

Notable changes:

Removes use of dep in favor of Go modules.
Removes vendor directory. As a result, switches license notice
generation to go-license.
Forks code generation scripts from respective k8s/knative sources to
allow module aware imports. (I want to fix this upstream sometime in
the future, but for now this is as best as we can do).

This should not result in any change of functionality.

This change is dependent on tektoncd/plumbing#121. The only difference is the removal of the replace line in go.mod. Otherwise this should be good to go.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

Update project to use Go modules.

wlynch · 2019-11-22T20:40:04Z

/hold
tektoncd/plumbing#121

vdemeester · 2019-11-26T08:16:39Z

Looks really good. The only "question"/"worry" I've got is on the removal of the vendor folder (at least for now). On cli we did keep the vendor folder for several reason (that may need to be re-discussed 😛)

Allows to do go build -mod=vendor … to build tkn without needing anything more that a git clone
On the same note as above, if for some reason I'm off the network or have a slow network, or github is messed-up (or my GOPROXY), I can still build it
Whenever we had problems with go mod (from 1.12 to 1.13), contributor could still work and hack on the cli as long as they didn't change dependencies
Allows "older" version to build (this is a bit weird as.. depending on what we are using from the standard library it might not be true anymore but..)
and a totally stupid reason : it makes the diff smaller 😹

All that said, I am fine with whatever we choose, but we should probably do the same on all project then.

/cc @chmouel @bobcatfish @imjasonh @dibyom

PS: I feel I am a bit biased when talking about go modules 😝

mnuttall · 2019-11-27T17:59:21Z

@vdemeester

On the same note as above, if for some reason I'm off the network or have a slow network, or github is messed-up (or my GOPROXY), I can still build it

Do you need to be online for every go build, or just the first one?

In general this looks like it should be 'better than dep on balance'. I'd be happy to move dashboard and webhooks over to go modules after a few weeks of waiting for pipeline to iron out any major gotchas. A cautious +1 from me.

wlynch · 2019-11-27T18:32:49Z

@vdemeester

On the same note as above, if for some reason I'm off the network or have a slow network, or github is messed-up (or my GOPROXY), I can still build it

Do you need to be online for every go build, or just the first one?

Just the first. Modules are cached locally at $GOPATH/pkg/mod. One thing to note though is that modules are fetched lazily, so if you do a clone then disconnect you may not have all dependencies locally. You would need to do something like go build ./... or go get ./... to have the go tool resolve and cache the dependencies.

imjasonh · 2019-11-27T19:31:04Z

I have a slight preference for keeping vendor/ as well for the reasons Vincent outlined. They also must be included in the released images, so having them all the time makes that easier.

wlynch · 2019-11-27T20:08:30Z

/test pull-tekton-pipeline-build-tests

wlynch · 2019-11-27T20:44:25Z

I have a slight preference for keeping vendor/ as well for the reasons Vincent outlined. They also must be included in the released images, so having them all the time makes that easier.

Is this for reciprocal licenses? If so, go-license takes care of this and puts code into third_party. We should verify even in the vendorless case this folder is exported properly though.

My preference is to omit the vendor directory.

it makes PR sizes more representative of the changes being made (e.g. no XXL PRs because a new dep was added)
it prevents future repo size creep from transitive dependencies. vendor is ~92% (53/58MB) of the on disk size of the pipelines repository. Although we can't remove this from the history, we can curb additional growth (as a somewhat exaggerated comparision, kubernetes/kubernetes vendor is 110MB). Not compelling, since CI tools would need to fetch all source anyway, perhaps this is a small optimization to not have to have 2 identical versions of the same dependency, but likely not critical.
you don't need to remember to pass in / export -mod=vendor to use the vendor directory.

The most compelling argument to keep vendor for me is compatibility with older Go versions, but vendor support exists since 1.11 (released 2018/08). I'm not sure about pipelines, but triggers requires Go >= 1.13, so I don't think we really need to worry about this too much.

imjasonh · 2019-11-27T21:43:04Z

Is this for reciprocal licenses? If so, go-license takes care of this and puts code into third_party. We should verify even in the vendorless case this folder is exported properly though.

Some dependencies (hashicorp/lru) require us to include the source code, not just the licenses.

wlynch · 2019-12-02T19:18:48Z

Is this for reciprocal licenses? If so, go-license takes care of this and puts code into third_party. We should verify even in the vendorless case this folder is exported properly though.

Some dependencies (hashicorp/lru) require us to include the source code, not just the licenses.

Yup! We're on the same page. =] go-licenses copies anything that is identified as reciprocal to third_party/.

ghost · 2019-12-05T14:25:14Z

Paging @wlynch @imjasonh @vdemeester have you spoken any further on this issue? As Jason mentioned offline the other day now's a good time to get moving on this since the latest release has just gone out and we're not scrambling to get a new one ready.

Given that go-licenses seems to do what we want with source code and license files I see a couple options:

Go ahead with removing /vendor. If we start getting signal that we've broken something or someone we can reintroduce it.
Continue ahead with the vendor directory but now using go mod instead of dep. Consider its complete removal if clearer consensus is reached.

Either way it'd be great to get on the blessed go module path.

wlynch · 2019-12-05T18:04:45Z

Paging @wlynch @imjasonh @vdemeester have you spoken any further on this issue? As Jason mentioned offline the other day now's a good time to get moving on this since the latest release has just gone out and we're not scrambling to get a new one ready.

Given that go-licenses seems to do what we want with source code and license files I see a couple options:

Go ahead with removing /vendor. If we start getting signal that we've broken something or someone we can reintroduce it.

Continue ahead with the vendor directory but now using go mod instead of dep. Consider its complete removal if clearer consensus is reached.

Either way it'd be great to get on the blessed go module path.

I'd like to submit this before more version changes/updates happen, so I'm going to proceed with #2. Like you said, we can continue discussing keeping the vendor directory after we switch to modules.

tekton-robot · 2019-12-05T18:05:32Z