Skip to content
This repository has been archived by the owner on May 6, 2022. It is now read-only.

Migrate from glide to dep for dependency management #1670

Merged
merged 7 commits into from
Jan 25, 2018

Conversation

carolynvs
Copy link
Contributor

@carolynvs carolynvs commented Jan 17, 2018

Closes #1665.

This migrates from glide to dep for dependency management. All locked revisions stayed the same except:

All glide commands have been updated to use dep, except for glide nv (which lists packages in the project, excluding vendor) which doesn't have an analog in dep because it's nicely supported in Go 1.9+ now via ./...

I have added make targets:

  • verify-vendor: Run during the verify target. Ensures that the vendor/ directory is in sync with the contents of Gopkg.lock.
  • test-dep: Run during the test target. Checks that when a downstream consumer of our generated client library uses dep to vendor Service Catalog, that the information contained in our Gopkg.toml is sufficient for them to compile.

NOTE: A big release of dep just landed today (v0.4.0) but this is using (v0.3.2). I recommend starting there and then upgrading after the dust has settled.

Tasks:

  • Needs dep support for vendoring code generation tools. I am looking into this. As a workaround, some people are adding extra steps in their Makefile, but I'd like to see dep support this scenario. See dep cannot vendor dependencies that do not have Go source code golang/dep#1306 for full details. The manifest is missing the following packages that were previously managed by glide:
    Warning: Unable to preserve imported lock master (a0ff256) for github.com/jteeuwen/go-bindata. The project was removed from the lock because it is not used.
    Warning: Unable to preserve imported lock  (e26fc85) for github.com/kubernetes/repo-infra. The project was removed from the lock because it is not used.
    Warning: Unable to preserve imported lock kubernetes-1.8.0 (42f0582) for k8s.io/code-generator. The project was removed from the lock because it is not used.
    Warning: Unable to preserve imported lock  (9e661e9) for k8s.io/gengo. The project was removed from the lock because it is not used.
    
    UPDATE: I am using required entries with a path to a main package to force dep to do what we want.
  • Must downstream consumers of the service catalog use a specific revision from k8s.io/kube-openapi? It was pinned in the glide configuration but it's not clear if a looser constraint would work, or if we are avoiding a particular bug. Answer: They only care about the client libraries, so no pins needed.
  • Same question as above for the dependencies of k8s.io/apimachinery. In this case, some of the pinned dependencies are transitive (not direct imports of service catalog), and are difficult to pin for downstream consumers. Answer: We will rely on the lock
  • Do we need to keep the glide configuration around for a transition period or can there be a hard cutover. I didn't find any projects using service catalog as a library who weren't also using dep. Answer: No one has responded on the mailing list, and the only consumer I have found already uses dep. So I'm going to try a hard cutover.
  • Update any scripting that previously used glide.
  • Update developer documentation and demonstrate expected dep tasks and expectations (e.g. always separate out committing vendor into a separate commit for easier reviewing, always run dep prune, etc).
  • Evaluate if we would like to reduce the vendor directory size by using dep prune (which very soon will be built into ensure and the manifest configuration). Answer: can't use prune yet because of our hack for dep cannot vendor dependencies that do not have Go source code golang/dep#1306 (first bullet above). It's removing files we use for dev tools, that aren't imported. Will make sure we consider this scenario for that issue

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 17, 2018
@MHBauer
Copy link
Contributor

MHBauer commented Jan 18, 2018

Nice list.

Are we picking up this llrb lib somewhere and we have missed it?

@carolynvs
Copy link
Contributor Author

Are we picking up this llrb lib somewhere and we have missed it?

As I understand it, it is coming from this: github.com/coreos/etcd -> github.com/google/btree -> github.com/petar/GoLLRB. If we don't want to pick it up I can add an ignore.

@carolynvs
Copy link
Contributor Author

Okay, I've pushed changes that vendor the code generators (the first item from my TODO list).

There was one dependency that I was unable to keep at the same revision as before: github.com/kubernetes/repo-infra. Maybe I could get it with more time but I wanted to make sure that we cared before going down a dependency rabbit hole. The short reason is that when I used that old revision, I ran into a problem with a dependency that wasn't previously vendored, https://github.com/bazelbuild/buildtools. So I went to master, which since they now use dep, made life easier. 😀

@carolynvs
Copy link
Contributor Author

Rebased with the recent bump to k8s 1.9.1. I have a couple open questions in the OP that need to be answered before working on this further.

@MHBauer
Copy link
Contributor

MHBauer commented Jan 20, 2018

Not sure I understand enough about how go dependencies work to be able to answer questions. Maybe we can go through some of the questions in the next meeting?

@carolynvs
Copy link
Contributor Author

Not sure I understand enough about how go dependencies work to be able to answer questions. Maybe we can go through some of the questions in the next meeting?

Sure!

@carolynvs
Copy link
Contributor Author

Per our sig meeting today, I will update this PR to add a build task, verifying that if someone uses dep to vendor service catalog's generated client, that it builds. This will help us know when we need to add extra constraints to the manifest without blindly pinning.

@carolynvs carolynvs force-pushed the dep-migration branch 5 times, most recently from e558d6a to b49e7f2 Compare January 23, 2018 23:32
@carolynvs
Copy link
Contributor Author

@duglin Is there a way to ignore a directory when running verify-links.sh? It is picking up markdown files in dep's cahe (GOPATH/pkg/dep) because of the way the go cache is handled (it is in the repo root under .pkg).

@duglin
Copy link
Contributor

duglin commented Jan 23, 2018

@carolynvs not right now but I can add a flag to specify a set of dirs to ignore. Let me try to find some time tonight...

@carolynvs
Copy link
Contributor Author

@duglin Thanks! ❤️

@duglin
Copy link
Contributor

duglin commented Jan 24, 2018

@carolynvs try adding "-s WORD" to the href checker cmd line and it should skip any file with WORD in its path.

@carolynvs
Copy link
Contributor Author

@duglin That worked perfectly, thank you!

@carolynvs carolynvs changed the title [WIP] Migrate from glide to dep for dependency management Migrate from glide to dep for dependency management Jan 24, 2018
Copy link
Contributor

@MHBauer MHBauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have to worry about flattening anymore? I don't remember if that was due to kube/kube specifically or if it's all fixed now.

Makefile Outdated
DEP_TAG=$$(git describe --abbrev=0 --tags) && \
git checkout $$DEP_TAG && \
go install -ldflags="-X main.version=$$DEP_TAG" ./cmd/dep; \
git checkout master # Make go get happy by switching back to master
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we dep do the dep stuff in docker? fyi @duglin

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be removed (get-dep and check-dep) aren't used, because I baked dep into the docker image.

I originally had them there to help someone developing without docker, but because the Makefile redefines the GOPATH, it doesn't work anyway.

@@ -111,6 +105,7 @@ else
scBuildImageTarget = .scBuildImage
endif

# Even though we migrated to dep, it doesn't replace the `glide nv` command
NON_VENDOR_DIRS = $(shell $(DOCKER_CMD) glide nv)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think some go tooling by default knows not to look at the vendor directory. Need to look at what this was used for.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you run go test ./... using 1.9+, it won't run the tests in vendor. I believe all go tools (not just test but say vet and fmt) are also well behaved.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. The only place we use this output is as input to go vet. We can investigate this later.

$(if $(realpath vendor/k8s.io/apimachinery/vendor), \
$(error the vendor directory exists in the apimachinery \
vendored source and must be flattened. \
run 'glide i -v'))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the flattening.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dep automatically flattens (i.e. removes nested vendor directories) so we don't need to defend against a forgotten flag anymore.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very nice

vendor/github.com/petar/GoLLRB/doc/*
contrib/examples/consumer/Gopkg.lock
contrib/examples/consumer
contrib/examples/vendor/*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

besides gollrb, are these to just avoid dealing with contrib? dep automagically finds them or something?

Copy link
Contributor Author

@carolynvs carolynvs Jan 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

contrib/examples/consumer is something that I added to test a downstream consumer using dep to vendor the Service Catalog client library. make test-dep runs dep to vendor our generated clientset, and ensures that the app can compile. I added those ignore lines so that after running that target, a bunch of generated test files don't show up when someone runs git status. 😀

That test-dep target helps us sanity check that the constraints in our Gopkg.toml aren't missing anything with respect to downstream consumers. Otherwise, we wouldn't find out for example that we need to be pinning to a particular revision, or avoiding a bug with a semver range until someone reported it to us.

When we use dep internally when working on service catalog, dep takes into account our Gopkg.lock where all of the dependencies are locked to an exact revision. When someone vendors us using dep, only Gopkg.toml is used. That's why I was asking a bunch of questions in the sig meeting on Monday about why in our glide.yaml so many projects were pinned to an exact revision. I wanted to figure out how much we had to manually manage in the Gopkg.toml and what we could "get away with" letting dep deal with in the Gopkg.lock file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh neat!

* Resolve conflict for code.cloudfoundry.org/lager
  code.cloudfoundry.org/lager is required to be at 0bfa98e by
  github.com/pivotal-cf/brokerapi
  https://github.com/pivotal-cf/brokerapi/blob/35946a0079bda144d0c9ed68df36899451f90209/Gopkg.toml#L26

* Require code generators and build tools
  I had to change the constraint for repo-infra from a pinned revision
  to latest on master. I was running into dependency hell otherwise. Can revisit if
  necessary.
* Ignoring large doc files in GoLLRB
* Add test-dep target
  Validates that a downstream consumer of our client library can use dep
* Add verify-vendor target
  Validates that what is in vendor/ is in sync with Gopkg.lock
* Install dep in the docker build image
We cannot completely remove glide because we are using
`glide nv`. If we had a requirement of Go 1.9+ maybe that could
be dropped, since the go tools in that version don't include
vendor anymore with `./...`, but until then it needs to stay.
They don't work because GOPATH is redefined in the Makefile
So people will just have to rely on the docker, or install dep themselves.
@carolynvs
Copy link
Contributor Author

I've rebased to address a merge conflict in the Makefile. Not sure why the travis build is stuck in the queue but hopefully it will hurry up and start soon. 😀

@MHBauer
Copy link
Contributor

MHBauer commented Jan 25, 2018

I would bet dollars to donus on cri-o being the culprit, but I think kube-incubator as an org is getting full.

Seemed to have failed on a go get, so I punched it.

@MHBauer
Copy link
Contributor

MHBauer commented Jan 25, 2018

LGTM

Copy link
Contributor

@MHBauer MHBauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, have tested. build works fine. Tests ran and passed.

Copy link
Contributor

@arschles arschles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's been plenty of time for everyone to weigh in on this PR, and I've seen lots of support for this move for a bit now. I am LGTMing this and merging.

Thanks for doing this @carolynvs 👍

@arschles arschles added the LGTM1 label Jan 25, 2018
@arschles
Copy link
Contributor

Adding an LGTM2 label for you @MHBauer, since you commented

@arschles arschles added the LGTM2 label Jan 25, 2018
@arschles arschles merged commit 1c45aef into kubernetes-retired:master Jan 25, 2018
@carolynvs carolynvs deleted the dep-migration branch January 25, 2018 22:27
@MHBauer
Copy link
Contributor

MHBauer commented Jan 25, 2018

whoopsy again.

@MHBauer
Copy link
Contributor

MHBauer commented Jan 25, 2018

also yay \o/

@carolynvs
Copy link
Contributor Author

Woooo! Hopefully everything doesn't light on fire after this. 😇

@jboyd01 jboyd01 added this to the 0.1.5 milestone Jan 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. LGTM1 LGTM2 size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants