New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch test-infra to use go modules #10760

Merged
merged 9 commits into from Jan 18, 2019

Conversation

@fejta
Copy link
Contributor

fejta commented Jan 15, 2019

Similar to kubernetes/org#359
/hold

  • Create //:go target, as well as //:update-minor and //:update-patch targets for updating deps.
  • Get kazel, misspell and golint from @io_kubernetes_build, @com_github_client9_misspell and @com_github_golang_lint repos
  • Run go mod init to create k8s.io/test-infra module
  • Make hack/update-deps.sh use go mod instead of dep
    • Runs go mod tidy && go mod vendor to manage vendor dir
    • Runs //:gazelle -- update-repos --from-file=go.mod to manage go_repositories()
    • Manually prunes vendor dir
    • Now requires bazel to run (but ensures we're using the right golang version)
  • Make hack/update-codegen.sh run generators manually
    • go get to install any missing generators
    • No longer depend on a pruned vendor/k8s.io/code-generator/generate-groups.sh script

Required three fixes to build and test //... -//vendor/...:

  • The misspell fix above (nothing imports this, so go mod tidy deletes it
  • logrus no longer prints a space before ending the line (brittle //prow/entrypoint/run_test.go)
  • OAuthClient.Exchange() now requires an ...AuthCodeOption argument

/assign @cjwagner @stevekuznetsov @spiffxp @BenTheElder

deprecated-update
exit 0
fi
find vendor -name BUILD -exec rm '{}' +

This comment has been minimized.

@ixdy

ixdy Jan 15, 2019

Member

I'm guessing go mod tiny doesn't prune these?

This comment has been minimized.

@ixdy

ixdy Jan 15, 2019

Member

also minor nit: could combine these with find vendor -name BUILD -o -name BUILD.bazel -exec '{}' +

This comment has been minimized.

@ibrasho

ibrasho Jan 15, 2019

Contributor

go mod tidy is quite different from dep prune. It ensures that go.mod and go.sum are up-to-date with modules on disk. It mainly adds missing modules, and remove unused ones.

This comment has been minimized.

@fejta

fejta Jan 17, 2019

Author Contributor

Done

@ibrasho

This comment has been minimized.

Copy link
Contributor

ibrasho commented Jan 15, 2019

Same comment from the PR to k/org:
Can't we remove the vendor/ directory from the repository now?

@ibrasho

This comment has been minimized.

Copy link
Contributor

ibrasho commented Jan 15, 2019

Also, just in case the repo is within GOPATH, GO111MODULE=on should be added somewhere to force using modules.


rm -rf vendor
bazel run //:go -- mod tidy
bazel run //:go -- mod vendor

This comment has been minimized.

@ibrasho

ibrasho Jan 15, 2019

Contributor

Can we make these:

GO111MODULE=on bazel run //:go -- mod tidy
GO111MODULE=on bazel run //:go -- mod vendor

or export GO111MODULE=on earlier in this script.

This comment has been minimized.

@fejta

fejta Jan 17, 2019

Author Contributor

Done

@fejta fejta force-pushed the fejta:mod branch from e6e7b5c to 83de22f Jan 17, 2019

@fejta

This comment has been minimized.

Copy link
Contributor Author

fejta commented Jan 17, 2019

Can't we remove the vendor/ directory from the repository now?

I think we should do that later

@BenTheElder

This comment has been minimized.

Copy link
Member

BenTheElder commented Jan 17, 2019

Also, just in case the repo is within GOPATH, GO111MODULE=on should be added somewhere to force using modules.

please do not do that! IIRC this will almost definitely break kubetest + hack/e2e.go in k/k.

EDIT: obviously fine within the update scripts etc (see comment below), but I don't think all of our tools are ready for that. Kubernetes tools still depend heavily on GOPATH and we need to break that dependency cleanly 😅

@BenTheElder

This comment has been minimized.

Copy link
Member

BenTheElder commented Jan 17, 2019

let's make this a smooth and gradual transition and not break all the things 😬 forcing that mode on outside of the update tools or deleting vendor is a bit much for the first PR.

Show resolved Hide resolved go.mod Outdated

@fejta fejta force-pushed the fejta:mod branch 2 times, most recently from 6e53d7e to 35e0634 Jan 17, 2019

@fejta

This comment has been minimized.

Copy link
Contributor Author

fejta commented Jan 17, 2019

Anyone wanna LGTM?

@ixdy

This comment has been minimized.

Copy link
Member

ixdy commented Jan 17, 2019

can you update the docs (docs/dep.md) as well?

@fejta

This comment has been minimized.

Copy link
Contributor Author

fejta commented Jan 17, 2019

update the docs (docs/dep.md)

Done


ensure-in-gopath() {
if [[ "$PWD" != "$GOPATH/src/k8s.io/test-infra" ]]; then
echo Sadly, $(basename "$0") most run inside GOPATH=$GOPATH, not $PWD >&2

This comment has been minimized.

@ixdy

ixdy Jan 18, 2019

Member

nit: most -> must

This comment has been minimized.

@fejta

fejta Jan 18, 2019

Author Contributor

Done


codegen-init() {
echo "Ensuring generators exist..." >&2
local ver=b1289fc74931d4b6b04bd1a259acfc88a2cb0a66

This comment has been minimized.

@ixdy

ixdy Jan 18, 2019

Member

where does this version come from? should it be more visible?

This comment has been minimized.

@fejta

fejta Jan 18, 2019

Author Contributor

The version we're using now. No

codegen-init() {
echo "Ensuring generators exist..." >&2
local ver=b1289fc74931d4b6b04bd1a259acfc88a2cb0a66
which deepcopy-gen &>/dev/null || go get k8s.io/code-generator/cmd/deepcopy-gen@$ver

This comment has been minimized.

@ixdy

ixdy Jan 18, 2019

Member

this makes no guarantees that the right version of each of these is installed in $PATH.

This comment has been minimized.

@fejta

fejta Jan 18, 2019

Author Contributor

Correct

This comment has been minimized.

@BenTheElder

BenTheElder Jan 18, 2019

Member

hrm, I suspect this might bite later?

This comment has been minimized.

@fejta

fejta Jan 18, 2019

Author Contributor

True, and easy enough to fix later with a new commit? Aka we could just delete the which || prefix and always run it, we could put it into bazel, etc.

Think it is fine to deal with that later

bazel run //:go -- mod tidy
}

gen-deepcopy() {

This comment has been minimized.

@ixdy

ixdy Jan 18, 2019

Member

WTB: bazelified versions of these

}

gen-informer() {
echo "Generating infromer..." >&2

This comment has been minimized.

@ixdy

ixdy Jan 18, 2019

Member

informer

This comment has been minimized.

@fejta

fejta Jan 18, 2019

Author Contributor

Done

-not -iname "LICENSE*" \
-not -iname "AUTHORS*" \
-not -iname "CONTRIBUTORS*" \
-not -iname "LICENSE*" \

This comment has been minimized.

@ixdy

ixdy Jan 18, 2019

Member

this one is duplicated?

also we should probably keep COPYING files.

This comment has been minimized.

@ixdy

ixdy Jan 18, 2019

Member

and NOTICE files it seems

This comment has been minimized.

@ibrasho

ibrasho Jan 18, 2019

Contributor

If we want to replicate dep's pruning logic, here are the regexes for files kept after pruning:

  • license*
  • licence*
  • copying*
  • unlicense*
  • copyright*
  • copyleft*
  • *authors*
  • *contributors*
  • *legal*
  • *notice*
  • *disclaimer*
  • *patent*
  • *third-party*
  • *thirdparty*

Also, this is the list of source code extensions that Go build can read:

  • *.go
  • *.c
  • *.cc
  • *.cpp
  • *.cxx
  • *.m
  • *.h
  • *.hh
  • *.hpp
  • *.hxx
  • *.s
  • *.swig
  • *.swigcxx
  • *.syso

This comment has been minimized.

@ibrasho

ibrasho Jan 18, 2019

Contributor

If we can convert this to a Go script 😎 , we can import gps from dep and still apply the same logic (especially deleteing non-Go files).

This comment has been minimized.

@fejta

fejta Jan 18, 2019

Author Contributor

Done

As for making the pruning perfect (as opposed to functional) let's save that for future PRs and/or creating a reusable version that lives in kubernetes/repo-infra

@fejta fejta force-pushed the fejta:mod branch from 70787f7 to 49ef6fe Jan 18, 2019

@BenTheElder
Copy link
Member

BenTheElder left a comment

The codegen stuff seems like it could cause some fun in the future around which version is used but otherwise LG
/lgtm
/hold

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 18, 2019

LGTM label has been added.

Git tree hash: 8332b1d86ccc73bb2b9cf9dd031ce10d77b7a9e0

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 18, 2019

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

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

@BenTheElder

This comment has been minimized.

Copy link
Member

BenTheElder commented Jan 18, 2019

I think we can leverage the tricks here golang/go#25922 to sort out tool dependencies further

@stevekuznetsov

This comment has been minimized.

Copy link
Contributor

stevekuznetsov commented Jan 18, 2019

Go changes here LGTM

@fejta

This comment has been minimized.

Copy link
Contributor Author

fejta commented Jan 18, 2019

/hold cancel

@k8s-ci-robot k8s-ci-robot merged commit 645ca4a into kubernetes:master Jan 18, 2019

12 checks passed

cla/linuxfoundation fejta authorized
Details
pull-test-infra-bazel Job succeeded.
Details
pull-test-infra-gubernator Skipped
pull-test-infra-lint Job succeeded.
Details
pull-test-infra-verify-bazel Job succeeded.
Details
pull-test-infra-verify-codegen Job succeeded.
Details
pull-test-infra-verify-config Job succeeded.
Details
pull-test-infra-verify-deps Job succeeded.
Details
pull-test-infra-verify-gofmt Job succeeded.
Details
pull-test-infra-verify-govet Job succeeded.
Details
pull-test-infra-verify-labels Skipped
tide In merge pool.
Details

@fejta fejta deleted the fejta:mod branch Jan 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment