kubernetes: use Go modules (vgo) instead of godep #65683
Conversation
These are all flagged by Go 1.11's more accurate printf checking in go vet, which runs as part of go test.
|
@rsc: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. 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. |
k8s-ci-robot
added
do-not-merge/release-note-label-needed
|
@rsc: PR needs rebase. 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. |
k8s-ci-robot
added
needs-ok-to-test
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rsc Assign the PR to them by writing 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 |
|
As noted above, this PR is not meant for actual review, and I don't plan to push this through myself - someone on Kubernetes core would be better positioned to deal with any nuances. The PR is mainly to support discussion with @sigma related to #64731, but others are welcome to take a look too, of course. I'd appreciate an /ok-to-test, just for my own curiosity: I've only been using |
|
/ok-to-test |
k8s-ci-robot
removed
the
needs-ok-to-test
|
I'm afraid this PR branch will have conflicts quite often when |
|
Explicitly adding hold. /hold |
k8s-ci-robot
added
the
do-not-merge/hold
|
/cc |
The go.mod files define the versions of dependency packages to use
during builds. They've been generated using the vgo tool from the
Godeps/Godeps.json files, so that the module definitions use the same
versions as already in the vendor directory.
In order to arrive at a module set for which all imports are valid
for all packages transitively needed by Kubernetes and their tests,
the module definitions diverge from Godeps/Godeps.json in two cases.
- github.com/matttproud/golang_protobuf_extensions is updated from
v0.0.0-20150406173934-fc2b8d3a73c4 to v1.0.1 (latest).
The important (and essentially only) difference is that
in the newer version, github.com/matttproud/golang_protobuf_extensions/pbutil's
test no longer imports github.com/golang/protobuf/proto/testdata,
which was deleted.
- k8s.io/kubernetes/pkg/volume/glusterfs imports github.com/heketi/heketi/client/api/go-client
and that package's test imports github.com/heketi/heketi/apps/glusterfs,
which imports github.com/heketi/heketi/executors/kubeexec, which imports
these packages that no longer exist:
k8s.io/kubernetes/pkg/client/clientset_generated/clientset
k8s.io/kubernetes/pkg/client/clientset_generated/clientset/typed/core/v1
k8s.io/kubernetes/pkg/client/unversioned/remotecommand
k8s.io/kubernetes/pkg/client/clientset_generated/clientset/fake
To fix this, I forked the pinned version of heketi/heketi and deleted the test,
and then I applied a local replacement in go.mod to fetch heketi/heketi
from rsc/heketi instead.
The tools.go file (which could be in a subdirectory) exists to provide imports
to packages that are important to keep covered by go.mod and to include
when preparing a vendor directory. This is basically the list from hack/godep-save.sh
plus commands in sub-modules needed during make.
This vendor tree was created by running: vgo mod -vendor for i in $(cd staging/src/k8s.io/; ls) do rm -rf vendor/k8s.io/$i ln -s ../../staging/src/k8s.io/$i vendor/k8s.io/$i done hack/update-bazel.sh The Go module definitions specify the same versions currently used by godep, so the actual source code versions in vendor are not changing. However, there are a few differences: - In general the transitive closure of a Go module's dependencies is defined to include tests of those dependencies, so that the dependencies can be tested in the specific version configuration defined by the main module. Apparently godep did not include these tests, so there are many new *_test.go files being added, along with their testdata/ directories, their go_test stanzas in BUILD files, and any of their own dependencies. - Apparently godep stripped import path comments when copying code to vendor. "vgo mod -vendor" does not. It should not be necessary any more. (There may have been a bug early on that made it necessary, but if so, that bug is long since fixed.) - Vgo's vendor dump is incorrectly missing files named AUTHORS*, CONTRIBUTORS*, COPYING*, LICENSE*, NOTICE*. This is a bug in vgo that will be fixed soon. The vendor tree in this commit works fine with "make && make test".
|
@rsc: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
| module k8s.io/kube-aggregator | ||
|
|
||
| require ( | ||
| k8s.io/api v1.12.0 |
There was a problem hiding this comment.
could we use a version string which never changes here, like "master" ? What do other projects do?
There was a problem hiding this comment.
You could for this repo, but when publishing to the place where clients of k8s.io/kube-aggregator would download the release, you'd want to make it have real versions, so that a client using kube-aggregator uses a new enough version of api etc for it.
There aren't a lot of other projects doing what Kubernetes is doing here.
There was a problem hiding this comment.
Was thinking about changing it to a real version when branching off for releases, instead of having the version already on master for the next release.
|
|
||
| replace k8s.io/apimachinery v1.12.0 => ../apimachinery | ||
|
|
||
| require github.com/gogo/protobuf v0.0.0-20170330071051-c0656edd0d9e |
There was a problem hiding this comment.
For the time being we still have to publish Godeps.json and vendor/ (for the master branch) of our staging repos. Godeps.json lists all packages, not only the repos. The publishing-bot uses that as a trigger to rebuilt the pruned vendor/ dir.
So in theory we could recreate Godeps.json (for our libraries' consumers who are still on old non-vgo tools) during the publishing run. But we now lack the trigger. Running godep is awefully slow without this optimization on each commit during publishing.
Hence, I would prefer to keep Godeps.json around for the time being, even if it is generated somehow from go.mod.
| k8s.io/client-go v1.12.0 => ../client-go | ||
| ) | ||
|
|
||
| require ( |
There was a problem hiding this comment.
How will this be maintained? Will vgo sync do that?
| k8s.io/apimachinery v1.12.0 | ||
| ) | ||
|
|
||
| replace ( |
There was a problem hiding this comment.
if I understand correctly, we only have to remove this block during publishing of the staging repos. Then the file is valid in a normal non-monorepo world. 👍
There was a problem hiding this comment.
Yes, probably you'd want to remove this as part of the export of this code into its own repos. But note that it only applies if you checked out those repos and built them directly. Other projects using the repos as dependencies will ignore replace & exclude lines (we don't want to give dependencies that much control over a larger build).
|
@rsc Thanks for taking a wack at this. vgo side question, k8s does minor releases but changes the Go API in each minor release. How would people handle importing k8s as a vendored project via vgo? |
|
@mattfarina, I don't fully understand the k8s dev cycle etc, but I think generally the k8s team needs to decide/confirm what they intend to guarantee to users about stability and then apply version numbers accordingly to express that.
|
This updates github.com/matttproud/golang_protobuf_extensions to a released version. There's no significant change in the code itself, and the corresponding tests (which are not vendored) behave better with vgo (see details in kubernetes#65683).
Before this change, heketi vendors kubernetes/kubernetes, at an old version (1.6.4). There are a few issues with that: - 1.6.4 is old, and unsupported, so it's not super useful - vendoring kubernetes/kubernetes is not supported - it gets in the way of vgo adoption (see kubernetes#65683 for details) This change introduces a new version of heketi that's: - much more recent (v7.0.0 instead of v4.0.0+) - uses only kubernetes/{api,apimachinery,...} As such it should ease vgo transition, and generally cleanup our dependency graph.
|
FYI I'm attempting to resolve the issues with the dependencies misbehaving in step 1 |
This change makes use of github.com/sigma/heketi branch k8s-standalone, which is a stripped down, k8s-specific version of github.com/heketi/heketi with: - only useful client-code, which sidesteps the issue of heketi improperly vendoring k8s (which prevents vgo from working, see kubernetes#65683) - uses only Apache 2 licensed code (which addresses partially kubernetes/sig-release#223) For details, see sigma/heketi@d481979
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. update github.com/matttproud/golang_protobuf_extensions to 1.0.1 **What this PR does / why we need it**: This updates github.com/matttproud/golang_protobuf_extensions to a released version. There's no significant change in the code itself, and the corresponding tests (which are not vendored) behave better with vgo (see details in #65683). **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: **Special notes for your reviewer**: **Release note**: ```release-note NONE ```
This updates github.com/matttproud/golang_protobuf_extensions to a released version. There's no significant change in the code itself, and the corresponding tests (which are not vendored) behave better with vgo (see details in kubernetes#65683).
|
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
k8s-ci-robot
added
the
lifecycle/stale
|
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
k8s-ci-robot
added
lifecycle/rotten
|
Rotten issues close after 30d of inactivity. Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
|
@fejta-bot: Closed this PR. In response to this:
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. |
|
Shouldn't this be reopened? |
DO NOT MERGE
This is a demonstration PR to show what it might look like to update
k8s.io/kubernetes to use Go modules, as planned for inclusion in Go 1.11
and implemented by the current vgo prototype. I do not intend to revise
this PR myself for merging into Kubernetes. Instead it is meant as a guide
for anyone on the Kubernetes team who wants to move Kubernetes from
godep to Go modules.
After this sequence of commits,
make && make teststill succeeds.But now so does:
Note that the
vgo testcommand does not needhack/run-in-gopath.sh.Note also that the transcript above checks out kubernetes outside GOPATH,
and not even in a directory named kubernetes, and the vgo commands
still work.
The
vgocommands work directly without a special GOPATH for three reasons:The root go.mod declares the import path of that directory,
so that vgo can tell that $HOME/k8shack holds k8s.io/kubernetes.
The go.mod files define the specific dependency versions to use
in the build. That information used to be expressed by putting the
right source files into
vendor, but thevgocommands are actuallyignoring
vendorentirely. (Usingvgo -getmode=vendorwould usevendor.)The substitution of staging/src/k8s.io/* into the build can now be
expressed directly in the go.mod file instead of having to do it with
a special GOPATH or symlinks in
vendor.The staging redirects set up a convention where k8s.io/kubernetes
v1.X.Y declares a dependency on k8s.io/thing v1.X.Y and then replaces
that version with the copy in ./staging/src/k8s.io/thing. Similarly
staging/src/k8s.io/thing v1.X.Y depends (if needed) on
k8s.io/thing2 v1.X.Y, again replaced with ../thing2 (different
relative path because now we're talking about something in staging
depending on something else in staging).
When k8s.io/thing v1.X.Y is used by some outside project, the go.mod
file's require will be processed, so that the outside project will
make sure to use k8s.io/thing2 v1.X.Y, but the replace statement will
not be - the outside project will fetch k8s.io/thing2 from the thing2
repository, not some local directory. I know that k8s.io/client-go is
on v8.0.0 so clearly it has more of a history of backwards-incompatible
changes, and this PR's use of v1.12.0 is clearly wrong and needs an
adjustment of some kind . But the redirects can support any version
convention, including no convention at all. The point is how easy it was to
support the staging directory.
I'm going to copy the commit message detail into this PR text so that
everything is in one place for anyone landing on this PR.
(The printf-related changes are copied from PR 65649, because
I am using Go 1.11 with vgo, and those changes avoid some new test failures.)
Step 1: add Go module definitions (3837edb):
The go.mod files define the versions of dependency packages to use
during builds. They've been generated using the vgo tool from the
Godeps/Godeps.json files, so that the module definitions use the same
versions as already in the vendor directory.
In order to arrive at a module set for which all imports are valid
for all packages transitively needed by Kubernetes and their tests,
the module definitions diverge from Godeps/Godeps.json in two cases.
github.com/matttproud/golang_protobuf_extensions is updated from
v0.0.0-20150406173934-fc2b8d3a73c4 to v1.0.1 (latest).
The important (and essentially only) difference is that
in the newer version, github.com/matttproud/golang_protobuf_extensions/pbutil's
test no longer imports github.com/golang/protobuf/proto/testdata,
which was deleted.
k8s.io/kubernetes/pkg/volume/glusterfs imports github.com/heketi/heketi/client/api/go-client
and that package's test imports github.com/heketi/heketi/apps/glusterfs,
which imports github.com/heketi/heketi/executors/kubeexec, which imports
these packages that no longer exist:
To fix this, I forked the pinned version of heketi/heketi and deleted the test,
and then I applied a local replacement in go.mod to fetch heketi/heketi
from rsc/heketi instead.
The tools.go file (which could be in a subdirectory) exists to provide imports
to packages that are important to keep covered by go.mod and to include
when preparing a vendor directory. This is basically the list from hack/godep-save.sh
plus commands in sub-modules needed during make.
Step 2: update vendor using vgo (b7f2bed)
This vendor tree was created by running:
The Go module definitions specify the same versions currently used by godep,
so the actual source code versions in vendor are not changing. However, there
are a few differences:
In general the transitive closure of a Go module's dependencies is defined
to include tests of those dependencies, so that the dependencies can be
tested in the specific version configuration defined by the main module.
Apparently godep did not include these tests, so there are many new *_test.go files being added,
along with their testdata/ directories, their go_test stanzas in BUILD files,
and any of their own dependencies.
Apparently godep stripped import path comments when copying code to vendor.
"vgo mod -vendor" does not. It should not be necessary any more.
(There may have been a bug early on that made it necessary,
but if so, that bug is long since fixed.)
Vgo's vendor dump is incorrectly missing files named
AUTHORS*, CONTRIBUTORS*, COPYING*, LICENSE*, NOTICE*.
This is a bug in vgo that will be fixed soon.
The vendor tree in this commit works fine with "make && make test".
Step 3: remove old Godeps files.
They're not needed anymore.
A real PR would need to update hack/godep*.sh and perhaps other scripts
(maybe hack/run-in-gopath.sh would be simplified).