Switch to go modules #188

bswartz · 2019-04-10T15:04:33Z

No description provided.

k8s-ci-robot · 2019-04-10T15:04:37Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bswartz
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: lpabon

If they are not already assigned, you can assign the PR to them by writing /assign @lpabon in a comment when ready.

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

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

bswartz · 2019-04-10T15:05:52Z

/assign lpabon

pohly · 2019-04-10T17:06:53Z

Is this still using the "vendor" directory?

I ran "make test" in /tmp/csi-test and got lots of "finding" and "downloading" messages:

go: finding golang.org/x/exp v0.0.0-20190121172915-509febef88a4
go: finding golang.org/x/net v0.0.0-20180724234803-3673e40ba225
go: downloading google.golang.org/grpc v1.19.0
go: downloading github.com/container-storage-interface/spec v1.1.0
go: downloading github.com/onsi/ginkgo v1.8.0
go: downloading github.com/golang/protobuf v1.3.1
go: downloading gopkg.in/yaml.v2 v2.2.2

I think we still want to maintain and use the "vendor" directory, to avoid having to reach out to all those repos during a CI build.

Also this:

PASS
mock driver started
build cache is disabled by GOCACHE=off, but required as of Go 1.12
Makefile:30: recipe for target 'test-sanity' failed
make: *** [test-sanity] Error 1

bswartz · 2019-04-10T19:08:31Z

Is this still using the "vendor" directory?

I think we still want to maintain and use the "vendor" directory, to avoid having to reach out to all those repos during a CI build.

From what I understand, go modules obsoletes the use of vendor directories. Now instead there is a "module cache" located at $GOPATH/pkg/mod which serves the same purpose the vendor directory used to serve. The benefit of the module cache is that it can store more than 1 version of each package, and it can be stored outside your git repo (dramatically reducing the size of most git repos). CI builds rely on module caches/mirrors like Athens to avoid hitting the internet.

bswartz · 2019-04-10T19:13:47Z

Thanks for reminding me that I forgot to delete the vendor directory with this change. That is done now.

msau42 · 2019-04-11T01:36:26Z

FYI, an overview of go modules: https://www.youtube.com/watch?v=OrMA16ASz2U

pohly · 2019-04-11T06:05:36Z

Ben Swartzlander <[email protected]> writes:

> Is this still using the "vendor" directory? > > I think we still want to maintain and use the "vendor" directory, to avoid having to reach out to all those repos during a CI build. From what I understand, go modules obsoletes the use of vendor directories.

No, not necessarily: https://github.com/golang/go/wiki/Modules#how-do-i-use-vendoring-with-modules-is-vendoring-going-away "The initial series of vgo blog posts did propose dropping vendoring entirely, but feedback from the community resulted in retaining support for vendoring." BTW, that FAQ also explains why I was seeing packages being downloaded: one has to explicitly enable the use of the "vendor" directory with -mod=vendor or GOFLAGS=-mod=vendor.

The benefit of the module cache is that it can store more than 1 version of each package,

That should also be true for the "vendor" directory.

and it can be stored outside your git repo (dramatically reducing the size of most git repos).

Has that been a problem? Just wondering.

CI builds rely on module caches/mirrors like Athena to avoid hitting the internet.

Do we have or will we have that when building in Prow?

bswartz · 2019-04-11T17:49:42Z

No, not necessarily: https://github.com/golang/go/wiki/Modules#how-do-i-use-vendoring-with-modules-is-vendoring-going-away "The initial series of vgo blog posts did propose dropping vendoring entirely, but feedback from the community resulted in retaining support for vendoring." BTW, that FAQ also explains why I was seeing packages being downloaded: one has to explicitly enable the use of the "vendor" directory with -mod=vendor or GOFLAGS=-mod=vendor.

Thanks for this helpful link. I didn't find that information in my reading about go modules.

The benefit of the module cache is that it can store more than 1 version of each package,
That should also be true for the "vendor" directory.
and it can be stored outside your git repo (dramatically reducing the size of most git repos).

Has that been a problem? Just wondering.

I think that for most projects, the size of the vendor directory exceeds the size of the project's code by a factor of 10 or even 100. In practice this means that you'll hit any scaling problems 10-100x sooner than you otherwise would. For small projects this doesn't matter much, but I still don't like being wasteful.

CI builds rely on module caches/mirrors like Athena to avoid hitting the internet.

Do we have or will we have that when building in Prow?

That's a good question. Since k8s is switching to go modules fairly aggressively I assume some kind of solution is in place, but I haven't looked at the specifics.

I will read up on how to keep the vendored stuff because I'd like to add go module support with minimal disruptions.

pohly · 2019-04-11T18:24:14Z

Ben Swartzlander <[email protected]> writes:

>> CI builds rely on module caches/mirrors like Athena to avoid hitting the internet. > Do we have or will we have that when building in Prow? That's a good question. Since k8s is switching to go modules fairly aggressively I assume some kind of solution is in place, but I haven't looked at the specifics.

I asked on Slack. Catherine replied: "Currently everyone using modules in kubernetes-land is still vendoring, I think. We do not currently have a cache." (https://kubernetes.slack.com/archives/C09QZ4DQB/p1554994915357100) Ben and Steve confirmed that there is currently no cache. Let's keep vendoring the files until we know for sure that dropping them won't have a negative impact on the CI builds. CI builds happen in $GOPATH with go 1.12, so they should be using the "vendor" dir automatically. Note that we have a CI test which runs "test-vendor". The build rules for that currently assume that "dep" is used, and the check runs entirely locally. Writing a similar check for "go mod" will be a bit tricky. I don't know whether "go mod verify" can be used without actually reaching out to all repos. If so, then that would negate the purpose of having the vendor directory.

bswartz · 2019-04-11T18:54:33Z

I'll fix this. Thanks for the additional info

bswartz · 2019-04-12T15:26:47Z

@pohly PTAL

pohly · 2019-04-12T18:32:34Z

You need to use a different trick for turning of test result caching:

diff --git a/hack/e2e.sh b/hack/e2e.sh
index c182b1d..e3a8c6c 100755
--- a/hack/e2e.sh
+++ b/hack/e2e.sh
@@ -54,8 +54,8 @@ runTestAPI()
 	local pid=$!
         trap 'cleanup $pid $1' EXIT
 
-	GOCACHE=off go test -v ./hack/_apitest/api_test.go && \
-	GOCACHE=off go test -v ./hack/_embedded/embedded_test.go
+	go test -count=1 -v ./hack/_apitest/api_test.go && \
+	go test -count=1 -v ./hack/_embedded/embedded_test.go
 )
 
 runTestAPIWithCustomTargetPaths()
@@ -66,7 +66,7 @@ runTestAPIWithCustomTargetPaths()
 
 	# Running a specific test to verify that the custom target paths are called
 	# a deterministic number of times.
-	GOCACHE=off go test -v ./hack/_apitest2/api_test.go -ginkgo.focus="NodePublishVolume"
+	go test -count=1 -v ./hack/_apitest2/api_test.go -ginkgo.focus="NodePublishVolume"
 )
 
 runTestWithCustomTargetPaths()

But even then the test-vendor test will fail. You need to rebase or merge with master to get that test. Or we can run it in the CI:

/test pull-kubernetes-csi-csi-test

pohly · 2019-04-12T18:37:48Z

You can try out a different implementation of the test-vendor check in this PR. That'll trigger a test-subtree failure, which will prevent accidentally merging that modified PR. The actual test-vendor change then has to come in via csi-release-tools. release-tools/README.md has a command for pushing the modification into a csi-release-tools fork for submission as a PR.

pohly · 2019-04-12T18:39:00Z

During the transition period, test-vendor will have to be done so that it checks whether repo uses dep or go mod (can be done by checking for Gopkg.toml) and then do the right version of the check.

pohly · 2019-04-12T18:41:15Z

I checked that your branch can still be pulled into a project that uses dep for vendoring. It might not be using the dependency information, though.

msau42 · 2019-05-08T14:12:39Z

Would one way to check be try loading all the pkgs and see if that results in a diff?

pohly · 2019-05-08T15:02:26Z

Would one way to check be try loading all the pkgs and see if that results in a diff?

If we do that for each PR, then it negates the purpose of having the sources vendored, because each build then needs to access all remote repos to do the checking.

We could try to skip it in Prow under certain specific situations:

not handling a PR or
the fabricated merge commit leaves go.mod and go.sum unchanged

For normal developers, make test will always do the check, so they are warned if they forget to vendor. We can include a message that tells them what to do, too.

Sounds doable, albeit a bit complex.

pohly · 2019-05-10T15:35:29Z

Here's a modified test-vendor check which implements that idea:

test-vendor:
	@ echo; echo "### $@:"
	@ if [ -f Gopkg.toml ]; then \
		echo "Repo uses 'dep' for vendoring."; \
		case "$$(dep version 2>/dev/null | grep 'version *:')" in \
			*v0.[56789]*) dep check && echo "vendor up-to-date" || false;; \
			*) echo "skipping check, dep >= 0.5 required";; \
		esac; \
	  else \
		echo "Repo uses 'go mod' for vendoring."; \
		if [ "$${JOB_NAME}" ] && \
                   ( [ "$${JOB_TYPE}" != "presubmit" ] || \
                     [ $$(git diff "${PULL_BASE_SHA}..HEAD" -- go.mod go.sum vendor release-tools | wc -l) -eq 0 ] ); then \
			echo "Skipping vendor check because the Prow pre-submit job does not change vendoring."; \
		elif ! GO111MODULE=on go mod vendor; then \
			echo "ERROR: vendor check failed."; \
			false; \
		elif [ $$(git status --porcelain=2 -- vendor | wc -l) -gt 0 ]; then \
			echo "ERROR: vendor directory *not* up-to-date, it did get modified by 'GO111MODULE=on go mod vendor':"; \
			git status -- vendor; \
			git diff -- vendor; \
			false; \
		fi; \
	 fi;

However, when I try that with go 1.12.1, I do get a test failure:

### test-vendor:
Repo uses 'go mod' for vendoring.
ERROR: vendor directory *not* up-to-date, it did get modified by 'GO111MODULE=on go mod vendor':
On branch go-modules
Your branch is up-to-date with 'NetApp/go-modules'.
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   vendor/modules.txt

no changes added to commit (use "git add" and/or "git commit -a")
diff --git a/vendor/modules.txt b/vendor/modules.txt
index 49f67fc..13b15ec 100644
--- a/vendor/modules.txt
+++ b/vendor/modules.txt
@@ -95,6 +95,7 @@ google.golang.org/grpc/codes
 google.golang.org/grpc/reflection
 google.golang.org/grpc/status
 google.golang.org/grpc/connectivity
+google.golang.org/grpc/keepalive
 google.golang.org/grpc/balancer
 google.golang.org/grpc/balancer/roundrobin
 google.golang.org/grpc/credentials
@@ -109,7 +110,6 @@ google.golang.org/grpc/internal/envconfig
 google.golang.org/grpc/internal/grpcrand
 google.golang.org/grpc/internal/grpcsync
 google.golang.org/grpc/internal/transport
-google.golang.org/grpc/keepalive
 google.golang.org/grpc/metadata
 google.golang.org/grpc/naming
 google.golang.org/grpc/peer
release-tools/build.make:124: recipe for target 'test-vendor' failed
make: *** [test-vendor] Error 1

My version of go wrote the entries in a different order. I therefore only allow the test to be skipped if release-tools is also unmodified, otherwise we might end up updating Go without running the test.

What do you think? Shall we put that into release-tools from where @bswartz can pull it into this PR?

I created a PR, let's discuss there: kubernetes-csi/csi-release-tools#19

How a repo does vendoring is detected based on the presence of Gopkg.toml. The vendor check with `dep` was all done locally, but the corresponding check for `go mod` requires network access. The check therefore gets skipped when running in the Prow CI in situations where we are sure that it isn't needed (for example, in a periodic job).

pohly · 2019-05-11T15:10:37Z

@bswartz which version of go did you use to create the vendor directory?

I am a bit worried that re-doing the vendoring with 1.12.1 leads to a small difference (see above).

build.make: allow repos to use 'go mod' for vendoring

pohly · 2019-05-15T12:24:01Z

/assign

pohly

@bswartz can you update release-tools to the latest master, as described in release-tools/README/md?

That should get rid of the test error.

bswartz · 2019-05-16T20:09:00Z

@bswartz which version of go did you use to create the vendor directory?

I am a bit worried that re-doing the vendoring with 1.12.1 leads to a small difference (see above).

I can't remember which go version I used. I currently have 1.12.4, and I'm going to switch to 1.12.5. I'll do what you suggest in your latest comment though.

bswartz · 2019-05-17T13:23:53Z

@pohly PTAL

pohly · 2019-05-17T14:50:01Z

make test-vendor passed without problems now for me and in TravisCI. I used go 1.12.1, so it looks like any 1.12.x version is fine when checking or updating dependencies (one, two, all, right? 😄).

/hold

Before merging let's have a chat how we'll roll this out to more repos.

pohly · 2019-05-17T19:54:22Z

https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/kubernetes-csi_csi-test/188/pull-kubernetes-csi-csi-test/1129120593698885632 shows that the test-vendor test ran, as intended.

pohly · 2019-06-03T17:41:02Z

Discussed today in the CSI WG. The conclusion was to first finish the sidecar releases for 1.15 and do more experiments with "go mod" support in other repos (like csi-driver-host-path and csi-lib-utils).

One question which needs to be answered: what happens if a repo switches to "go mod" and some other repo which still uses "dep" pulls that repo in? Will "dep" be able to apply the constraints specified in go mod?

pohly · 2019-07-29T09:34:18Z

Looks like Kubernetes is going to rely on proxy.golang.org for caching dependencies: kubernetes/test-infra#13645

This means we could stop vendoring source code and just rely on that same cache?

msau42 · 2019-07-29T14:12:03Z

Sounds fine to me. We just need to set the GOPROXY env and update to go 1.13?

pohly · 2019-07-29T15:37:07Z

Michelle Au <[email protected]> writes:

Sounds fine to me. We just need to set the GOPROXY env and update to go 1.13?

The GOPROXY env var is only needed for go 1.12, 1.13 will enable it by default. If I read the test-infra issue correctly, all (?) Prow jobs will automatically get GOPROXY set, so we might not have to do anything.

bswartz · 2019-07-29T17:29:53Z

I'm sure someone has already thought about this, but using a proxy like that will dramatically increase reliance on network resources at build time compared to the vendoring approach. I'm surprised there's no plan to at least try to use a proxy that will be local to the build machines.

pohly · 2019-07-29T17:50:05Z

I'm surprised there's no plan to at least try to use a proxy that will be local to the build machines.

Have a look at kubernetes/test-infra#13645. It has been discussed and the conclusion was that for all practical purposes, proxy.golang.org is local - it's apparently all hosted on Google Cloud. If that changes, then setting GOPROXY differently can still take care of that once the need arises.

bswartz · 2019-07-29T17:52:52Z

@pohly Thanks I forgot that golang is actually a google project and so it's unsurprising the proxy is hosted in the same place as the kubernetes build infra. Now I just need to figure out how to run my own proxy for my own builds.

bswartz · 2019-10-09T15:39:42Z

#224

…-notes-docs SIDECAR_RELEASE_PROCESS.md: Update release-notes syntax

k8s-ci-robot requested review from jsafrane and saad-ali April 10, 2019 15:04

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 10, 2019

k8s-ci-robot assigned lpabon Apr 10, 2019

bswartz force-pushed the go-modules branch from f9e46e2 to 8ebf600 Compare April 10, 2019 19:12

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 10, 2019

bswartz force-pushed the go-modules branch from 8ebf600 to b4958c2 Compare April 11, 2019 19:07

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 11, 2019

bswartz force-pushed the go-modules branch from b4958c2 to 477c106 Compare April 12, 2019 14:25

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 12, 2019

bswartz force-pushed the go-modules branch from 477c106 to f07bb01 Compare April 12, 2019 15:42

pohly mentioned this pull request May 10, 2019

build.make: allow repos to use 'go mod' for vendoring kubernetes-csi/csi-release-tools#19

Merged

Merge pull request kubernetes-csi#19 from pohly/go-mod-vendor

0399988

build.make: allow repos to use 'go mod' for vendoring

k8s-ci-robot assigned pohly May 15, 2019

pohly requested changes May 15, 2019

View reviewed changes

bswartz added 2 commits May 16, 2019 16:23

Merge commit '03999882f676a65ab71e11686c3b26b830d7700f' into go-modules2

db7f198

Switch to go modules

e78a2eb

bswartz force-pushed the go-modules branch from e811b7f to e78a2eb Compare May 16, 2019 20:24

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 17, 2019

pohly mentioned this pull request Sep 26, 2019

Switch to go modules kubernetes-csi/csi-lib-utils#33

Merged

pohly mentioned this pull request Oct 8, 2019

Go modules #224

Merged

bswartz closed this Oct 9, 2019

stmcginnis pushed a commit to stmcginnis/csi-test that referenced this pull request Oct 9, 2024

Merge pull request kubernetes-csi#188 from pwschuurman/update-release…

890b87a

…-notes-docs SIDECAR_RELEASE_PROCESS.md: Update release-notes syntax

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to go modules #188

Switch to go modules #188

bswartz commented Apr 10, 2019

k8s-ci-robot commented Apr 10, 2019

bswartz commented Apr 10, 2019

pohly commented Apr 10, 2019

bswartz commented Apr 10, 2019 •

edited

Loading

bswartz commented Apr 10, 2019

msau42 commented Apr 11, 2019

pohly commented Apr 11, 2019 via email

bswartz commented Apr 11, 2019 •

edited

Loading

pohly commented Apr 11, 2019 via email

bswartz commented Apr 11, 2019

bswartz commented Apr 12, 2019

pohly commented Apr 12, 2019

pohly commented Apr 12, 2019

pohly commented Apr 12, 2019

pohly commented Apr 12, 2019

msau42 commented May 8, 2019

pohly commented May 8, 2019

pohly commented May 10, 2019 •

edited

Loading

pohly commented May 11, 2019

pohly commented May 15, 2019

pohly left a comment

bswartz commented May 16, 2019

bswartz commented May 17, 2019

pohly commented May 17, 2019

pohly commented May 17, 2019

pohly commented Jun 3, 2019

pohly commented Jul 29, 2019

msau42 commented Jul 29, 2019

pohly commented Jul 29, 2019 via email

bswartz commented Jul 29, 2019

pohly commented Jul 29, 2019

bswartz commented Jul 29, 2019

bswartz commented Oct 9, 2019

Switch to go modules #188

Switch to go modules #188

Conversation

bswartz commented Apr 10, 2019

k8s-ci-robot commented Apr 10, 2019

bswartz commented Apr 10, 2019

pohly commented Apr 10, 2019

bswartz commented Apr 10, 2019 • edited Loading

bswartz commented Apr 10, 2019

msau42 commented Apr 11, 2019

pohly commented Apr 11, 2019 via email

bswartz commented Apr 11, 2019 • edited Loading

pohly commented Apr 11, 2019 via email

bswartz commented Apr 11, 2019

bswartz commented Apr 12, 2019

pohly commented Apr 12, 2019

pohly commented Apr 12, 2019

pohly commented Apr 12, 2019

pohly commented Apr 12, 2019

msau42 commented May 8, 2019

pohly commented May 8, 2019

pohly commented May 10, 2019 • edited Loading

pohly commented May 11, 2019

pohly commented May 15, 2019

pohly left a comment

Choose a reason for hiding this comment

bswartz commented May 16, 2019

bswartz commented May 17, 2019

pohly commented May 17, 2019

pohly commented May 17, 2019

pohly commented Jun 3, 2019

pohly commented Jul 29, 2019

msau42 commented Jul 29, 2019

pohly commented Jul 29, 2019 via email

bswartz commented Jul 29, 2019

pohly commented Jul 29, 2019

bswartz commented Jul 29, 2019

bswartz commented Oct 9, 2019

bswartz commented Apr 10, 2019 •

edited

Loading

bswartz commented Apr 11, 2019 •

edited

Loading

pohly commented May 10, 2019 •

edited

Loading