Skip to content
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

WIP: Migrate to dep #4094

Closed
wants to merge 4 commits into from
Closed

WIP: Migrate to dep #4094

wants to merge 4 commits into from

Conversation

aledbf
Copy link
Member

@aledbf aledbf commented Dec 17, 2017

  • update development.md
  • add note about the go-bindata module (only remaining git module)
  • fix federation build

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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 Dec 17, 2017
@chrislovecnm
Copy link
Contributor

/assign @justinsb

I know you reached out to him on this one

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
We suggest the following additional approver: justinsb

Assign the PR to them by writing /assign @justinsb in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@aledbf aledbf force-pushed the migrate-to-dep branch 18 times, most recently from cc015a1 to a6127b6 Compare December 18, 2017 04:48
@aledbf
Copy link
Member Author

aledbf commented Dec 18, 2017

@justinsb ping

@chrislovecnm
Copy link
Contributor

chrislovecnm commented Dec 19, 2017

Is it actually working? I am impressed. I will test when I can.

@chrislovecnm
Copy link
Contributor

How are we going to not run into problems like this?

I am only trying to add aws go sdk to test infra, and they are using bazel and dep together. kubernetes/test-infra#6086 - it is a mess.

I am wondering if we need to wait for go_rules to mature a bit with dep.

Thoughts?

@aledbf
Copy link
Member Author

aledbf commented Dec 25, 2017

How are we going to not run into problems like this?

I don't know.
That being said I can commit (long term) to fix any issue with dep in this repository.

I am only trying to add aws go sdk to test infra, and they are using bazel and dep together. kubernetes/test-infra#6086 - it is a mess.

I cannot event use the current code from master

aledbf@me:~/go/src/k8s.io/test-infra$ planter/planter.sh bazel build //...
planter/planter.sh: 58: planter/planter.sh: UID: parameter not set

I am wondering if we need to wait for go_rules to mature a bit with dep.

Right, but removing 6 millions of lines from this repository and almost every git module is a huge win (personal opinion)

@chrislovecnm
Copy link
Contributor

chrislovecnm commented Dec 27, 2017

First off I appreciate the effort and how you have helped this project. Huge kudos on that.

Dep is a huge win if we can still use bazel, otherwise let sleeping dogs lie until bazel and dep start playing nicer together. I am not a fan of the submodules, but it works, and is stable.

Having bazel broken is a HUGE loss for the project. So if we can get them working together nicely I would be open to this. Touching out build system makes me a tad nervous. We still have some makefile issues fr the last set of changes.

Again appreciate the time an effort you took with this.

@aledbf
Copy link
Member Author

aledbf commented Dec 27, 2017

Dep is a huge win if we can still use bazel, otherwise let sleeping dogs lie until bazel and dep start playing nicer together. I am not a fan of the submodules, but it works, and is stable.

Which tasks require bazel? (just to make sure it works)

@chrislovecnm
Copy link
Contributor

chrislovecnm commented Dec 27, 2017

Require bazel none. We do have bazel build and test targets. Admittedly it is the work flow that I am nervous about ... Investigating in the #bazel channel as well.

@justinsb thoughts?

@aledbf
Copy link
Member Author

aledbf commented Dec 27, 2017

@chrislovecnm as this is a WIP 😉 the last commit starts the process to fix the dep <-> bazel issues

make bazel-gazelle
gazelle update-repos -from_file Gopkg.lock
gazelle update -external vendored
make bazel-build

@justinsb
Copy link
Member

That looks better than the approach I've been using - I'll have to check out your trick @aledbf

One thing we want to be sure of is that the versions don't change - I created a script hack/dep.py which cross-checks dep status against the k8s Godeps. I used the k8s release-1.9 branch and there were a lot of diffs. I think we want to lock almost of all our dependencies, which in practice means we're not really using dep package resolution - but that is not necessarily a bad thing!

That script is #4152

@chrislovecnm
Copy link
Contributor

Talking with the infra-test folks dep has some bugs, especially with prune.

I think we should discuss this on next office hours.

@k8s-github-robot
Copy link

@aledbf PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 4, 2018
@chrislovecnm
Copy link
Contributor

@aledbf / @justinsb we did not discuss this during office hours, meant to. This PR is going to be in rebase hell shortly. I am kinda on the fence about this one, but I will trust your guy's judgment.

What are the final thoughts on this? Go or no GO?

@aledbf
Copy link
Member Author

aledbf commented Jan 5, 2018

What are the final thoughts on this? Go or no GO?

I take this as an experiment. Not sure if make sense to merge this PR.
Can we leave this open just as a WIP until I can solve the bazel - dep issues?

@justinsb
Copy link
Member

Thanks for getting us to do this @aledbf :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. 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.

5 participants