enforce no vendored tests in test-infra

[BenTheElder]

This makes hack/verify-bazel.sh and hack/update-bazel.sh enforce that there are no *_test.go files in vendor/*.

See also: bazel-contrib/rules_go#725, golang/dep#944
At some point this should be solved upstream (by pretty much the same thing, but in dep), but until then we can make sure anyone who touches vendor runs our update script.

fixes #5331

[BenTheElder]

/cc @cblecker @xiangpengzhao

[cblecker]

Tested locally. Works for me.
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, cblecker

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

Needs approval from an approver in each of these OWNERS Files:

• OWNERS [BenTheElder]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

[ixdy]

I kinda feel like putting this in the update-bazel script is misplaced. Should we have a go dep wrapper instead?

[cblecker]

it's not exactly the clearest, but because updating bazel is required anytime you run dep (it blows away all BUILD files in vendor/), it's not the worst place either.

[BenTheElder]

Maybe? hack/update-bazel.sh seemed like our "run after changing the build" which also fit "run after changing vendor/"

[BenTheElder]

We can't really enforce that anyone uses our dep wrapper as well, so we'd still need to have a verify somewhere.

[BenTheElder]

Oh hmm @cblecker your comment hadn't loaded (??), that sums up what I was thinking more concisely 🙃

[krzyzacy]

it works for me, as the current workflow is always
dep ensure
dep prune
./hack/update-bazel.sh

Or maybe we can also make use of the hack/update-all.sh if it should not be in update-bazel.sh?

[cblecker]

/hold
Preventing merge if there's discussion

[xiangpengzhao]

Also tested locally and works for me. I like the short command bazel build //... I'm not sure which way to make this work is better though :)

[BenTheElder]

We could have another pair of scripts (verify-vendor and update-vendor?), and then maybe roll those in to an {update,verify}-build which calls both the vendor scripts and the bazel scripts? It seemed simple to roll these into the bazel scripts since the presubmit will remind you to run `./hack/update-bazel.sh` and removing `.go` files will always require running this. I think we should come up with something along these lines and go with it, `bazel build -- //... -//vendor/..` is not great for newcomers especially and I don't think `dep` will be fixed very quickly.

…

On Wed, Nov 8, 2017 at 6:22 PM, Peter (XiangPeng) Zhao < ***@***.***> wrote: Also tested locally and works for me. I like the short command bazel build //... I'm not sure which way to make this work is better though :) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#5411 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA4Bq62ivPfTDubSlBPbxBnBlqN98qWTks5s0mH7gaJpZM4QXFmu> .

[BenTheElder]

Meanwhile travis passed 5 hours ago and never updated the GitHub status ... #1342

[BenTheElder]

I just ran into this again, does anyone have any thoughts on this?

[fejta]

If we do not want vendored code to include tests, we should have a presubmit test that enforces this

[BenTheElder]

If we do not want vendored code to include tests, we should have a presubmit test that enforces this

This PR makes the verify-bazel script / presubmit check that since changing vendor changes the BUILD and makes update-bazel remove them before fixing the BUILD (since you'd have to do those together anyhow).

[fejta]

SGTM what do you want thoughts about?

[BenTheElder]

This was previously LGTM and then /hold-ed for further discussion on where the fix should live, but in the meantime local test/build has been broken for test-infra. I want to either merge this or see an alternative suggested & merged.

[cblecker]

Personally, I'm fine with this living here until such time as this is wrapped into dep. Perhaps a more detailed comment on why we are doing it? Or link back to this PR?

[BenTheElder]

Personally, I'm fine with this living here until such time as this is wrapped into dep. Perhaps a more detailed comment on why we are doing it? Or link back to this PR?

I added comments and a ref back to this PR let's
/hold cancel
And then once dep is fixed we can remove these.

[sdboyer]

We could have another pair of scripts (verify-vendor and update-vendor?), and then maybe roll those in to an {update,verify}-build which calls both the vendor scripts and the bazel scripts?

just followed a little link chain back from a dep issue and saw this bit, and thought it might be helpful for y'all to see this nascent plan around dep in CI. (comments are very welcome!)

[cblecker]

Thanks @sdboyer !