Update gengo to use go mod.

[n3wscott]

As a replacement for lavalamp#1 and #144.

Fixes #143

The move to go 1.13 seems to have solved the previous issues found in go mod 1.12.

Signed-off-by: Scott Nichols [email protected]

[k8s-ci-robot]

Welcome @n3wscott!

It looks like this is your first PR to kubernetes/gengo 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/gengo has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[n3wscott]

/retest

not sure how to get travis to run again.

[k8s-ci-robot]

@n3wscott: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

not sure how to get travis to run again.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[n3wscott]

Looks ready for test (if there are prow tests), travis passed now.

[lavalamp]

I'd love to keep this repo 100% shell-free :/

Is there any alternative to adding this? @liggitt might have an idea?

[liggitt]

I like the approach of super-minimal makefiles (e.g. one-line target definitions, no external file includes) that just call go commands, e.g.:

make test == go test ./…
make deps == go mod tidy && go mod vendor

It technically is still shell-adjacent, but with those two rules it tends to be less of a dumping ground for crazy stuff.

Also, is it necessary to vendor?

[n3wscott]

Not needed to vendor but I think it was originally, I can double check.

[n3wscott]

a fake vendor directory does not work in go 1.14, it confuses go mod to only look in vendor and not the cache. I moved the fake package to testdata.

[liggitt]

I got it to work! I added a replace directive to allow it to find the sub-module.

This should get trimmed off when something deps on this package.

That still seems like it shouldn't be necessary. I'd expect test data to be completely isolated from the dependencies of the module.

See https://github.com/kubernetes-sigs/clientgofix/blob/v0.3.0/pkg/fix_test.go#L38-L42 for an example of exercising various $GOPATH/module vendor/no-vendor scenarios in test cases

[n3wscott]

I got it to work! I added a replace directive to allow it to find the sub-module.
This should get trimmed off when something deps on this package.

That still seems like it shouldn't be necessary. I'd expect test data to be completely isolated from the dependencies of the module.

See https://github.com/kubernetes-sigs/clientgofix/blob/v0.3.0/pkg/fix_test.go#L38-L42 for an example of exercising various $GOPATH/module vendor/no-vendor scenarios in test cases

but the point of this test is that there is a package that is out of tree that the test is solving for, so go mod needs to know where to look. Previously it looked in vendor as that is a magic extension of $GOPATH, but that option is not available for go 1.14 as the existence of a vendor dir will limit the $GOPATH to only ./vendor by default. The replace directly allows go mod to locate the package in ./testdata.

[n3wscott]

Here is the test that uses the package fake/dep

gengo/parser/local_parse_test.go

Line 25 in e0e292d

if _, err := b.importBuildPackage("fake/dep"); err != nil {

go mod also does not see this as an import because package fake/dep is not used in the go code, only this test call. So this means it only needs to replace to point to a local go.mod file.

Without the replace line, the test fails:

    TestImportBuildPackage: local_parse_test.go:26: unable to import "fake/dep": package fake/dep is not in GOROOT (/usr/local/Cellar/go/1.14.2_1/libexec/src/fake/dep)
--- FAIL: TestImportBuildPackage (0.04s)

This is expected because of the vendor gotchyas with go 1.14

[n3wscott]

Ping on this. RFAL

[lavalamp]

I'm OK with this. fake is only in the replace directives, not in the e.g. go.sum file. Not vendoring makes this much more palatable :)

Jordan, any more thoughts?

[liggitt]

I really don't want fake test modules or dependencies expressed in the go.mod file downstreams of this repo will be consuming. I added a commit to https://github.com/liggitt/kubernetes-gengo/commits/move-go-mod which lets us run this test scenario without a replace in the actual go.mod

[lavalamp]

Yeah, that looks better.

…

On Tue, May 12, 2020 at 2:06 PM Jordan Liggitt ***@***.***> wrote: I really don't want fake test modules or dependencies expressed in the go.mod file. I added a commit to https://github.com/liggitt/kubernetes-gengo/commits/move-go-mod which lets us run this test scenario without a replace in the actual go.mod — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#178 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAE6BFXFIZE4UCI7KVTU7G3RRG25JANCNFSM4MZ4UCEQ> .

[n3wscott]

ok thanks @liggitt I merged your update into this PR to drop the replace.

[lavalamp]

/lgtm
/approve

Thanks!!

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lavalamp, n3wscott

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [lavalamp]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment