Skip to content
This repository has been archived by the owner on Apr 21, 2019. It is now read-only.

Migrate from glide to golang/dep and also bump vendor #237

Merged
merged 15 commits into from Mar 1, 2018

Conversation

shashidharatd
Copy link
Contributor

@shashidharatd shashidharatd commented Feb 19, 2018

This PR does following things.

  • Migrate from glide to golang/dep as dependency management tool
  • Bump the k8s dependency to v1.10.0-alpha.3 and adapt the usage in federation
  • Refactor the test codes to not use k8s.io/kubernetes/test/e2e/framework (as it is one huge package and lot many unnecessary stuff gets imported. So pulled in only necessary funcs into a reusable package within federation).
  • Few other miscellaneous stuff.

/cc @kubernetes/sig-multicluster-pr-reviews
/assign @irfanurrehman

p.s: Even though the PR is huge, most of it is either generated code or vendored code. So request to review commit by commit which is much easier.

@k8s-ci-robot k8s-ci-robot added sig/multicluster Categorizes an issue or PR as relevant to sig-multicluster. 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. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 19, 2018
@shashidharatd shashidharatd force-pushed the golang-dep branch 2 times, most recently from 6a5e563 to 4ecc2ab Compare February 20, 2018 05:21
@shashidharatd
Copy link
Contributor Author

To get dep working with federation repo, we mostly followed the notes from @justinsb here and couple more tweaks. So here are the details:

  • Used required block to declare dependencies that are used by build and test scripts but not imported directly by code in the repo.
  • Reused the script which compares version in Godeps.json of k/k with the dependencies resolved by dep, output as below which helps in fixing the versions in Gopkg.toml and be compatible with k/k.
root@xxx-laptop:~/gopath/src/k8s.io/federation# ./hack/dep.py 
# update needed: github.com/chai2010/gettext-go c6fed771bfd517099caf0f7a961671fa8ed08723 vs bf70f2a70fb1b1f36d90d671a72795984eab0fcb
[[override]]
  name = "github.com/chai2010/gettext-go"
  revision = "c6fed771bfd517099caf0f7a961671fa8ed08723"
# Missing dep github.com/petar/GoLLRB, currently at version branch master
# Missing dep k8s.io/api, currently at version kubernetes-1.10.0-alpha.3
# Missing dep k8s.io/apiextensions-apiserver, currently at version kubernetes-1.10.0-alpha.3
# Missing dep k8s.io/code-generator, currently at version kubernetes-1.10.0-alpha.3
# Missing dep k8s.io/client-go, currently at version kubernetes-1.10.0-alpha.3
# Missing dep google.golang.org/appengine, currently at version branch master
# Missing dep k8s.io/apimachinery, currently at version kubernetes-1.10.0-alpha.3
# Missing dep github.com/coredns/coredns, currently at version v1.0.5
# Missing dep k8s.io/apiserver, currently at version kubernetes-1.10.0-alpha.3
# Missing dep k8s.io/metrics, currently at version kubernetes-1.10.0-alpha.3
# Missing dep k8s.io/kubernetes, currently at version v1.10.0-alpha.3

Note: Some of these packages in k8s.io/ does not have a record in Godeps.json in k/k. It would help to publicise these repo versions in releases. But anyway they are all tagged with release version.

  • The version for github.com/chai2010/gettext-go in Godeps.json breaks dep ensure. So using the same version as used in kops fixed the issue.
  • For bazel, as with kops, we had to delete all the BUILD files from vendor directory and regenerate them. It worked except for 2 issues which had to be manually resolved.
    • The version 3.2.13 for github.com/coreos/etcd which we used, has symlinks in cmd, which does not work with bazel. So had to be removed manually.
    • boilerplates are required for code auto-generation but does not have go code, here. So vendoring them was not possible automatically with dep. probably once this issue is addressed, we should be able to use it properly.

/cc @mattfarina, @marun. request to PTAL. esp bffc47b

@irfanurrehman
Copy link
Contributor

@shashidharatd I was having a look at different commits. One thing that strikes me as a future problem is copying and maintaining the e2e framework into federation repo. I agree that is reduces the overall vendor size, but I would recommend to let the folks responsible for maintaining the k8s e2e fw, still maintain it. We can try to pitch our case for having that portion of code out of k/k for it to be easier to be used by other projects.

@shashidharatd
Copy link
Contributor Author

@irfanurrehman, I agree and concur with your opinion. But importing the k8s.io/kubernetes/test/e2e/framework brings in lot of unwanted packages and also need to import the generated code. Our usage of this framework is little and does not change much in the longer run. It makes sense for us to maintain it ourselves. Also going forward we shall refactor the framework to only things which we need.

@irfanurrehman
Copy link
Contributor

Agree. Thanks for doing this @shashidharatd.
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 1, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: irfanurrehman, shashidharatd

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 [irfanurrehman,shashidharatd]

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

@k8s-ci-robot k8s-ci-robot merged commit 5b2fc9f into kubernetes-retired:master Mar 1, 2018
@shashidharatd
Copy link
Contributor Author

Thanks @irfanurrehman

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. sig/multicluster Categorizes an issue or PR as relevant to sig-multicluster. 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

3 participants