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

added localkube and dependencies #39

Merged
merged 3 commits into from
May 4, 2016
Merged

Conversation

ethernetdan
Copy link
Contributor

Closes #7 by having Redspread contribute localkube to a Kubernetes maintained repository. The plan is currently to deprecate localkube as it's own project and focus our development efforts on minikube.

We continued using Godeps to vendor the dependencies though I am very interested in switching to Glide. In the short time I've spent maintaining localkube have found managing Kubernetes as a dependency can be challenging with Godeps.

While I added compiling the localkube binary to the build script, the minikube cli still uses the Redspread bundled version of localkube (#15).

A ported version of kube2sky is being used here as the original package does not allow external access. I am highly in favor moving to a centrally maintained when that becomes available, there is a PR in to open up access (kubernetes/kubernetes#25061).

@dlorenc
Copy link
Contributor

dlorenc commented May 4, 2016

Looks great! One small request though, could you update the README.md to show the change to ./out/minikube instead of ./minikube in the instructions?

I'll have to think a bit about the folder structure, but this is probably fine for now. I was hoping to have the top level "cli" folder be the code for the minikube CLI, and have other top-level folders handle other things (I'm working on one for iso/ which will contain the code to build our VM .iso file).

But this layout has some advantages too. I'll think about it a little more, but we can clean that up later.

@luxas
Copy link
Member

luxas commented May 4, 2016

Great!

A note/proposal on folder structure:
cli -> pkg/minikube(/cli)
k8s -> pkg/(localkube/)dns/kube2sky
localkube -> pkg/localkube

At least, we should keep command line parsing etc. in cmd/ and all logic in pkg/

@ethernetdan
Copy link
Contributor Author

@dlorenc updated README, good catch

@luxas seems like a good approach. I'm thinking that we keep the structure as is and address that in another PR, if that sounds reasonable

@dlorenc
Copy link
Contributor

dlorenc commented May 4, 2016

@luxas seems like a good approach. I'm thinking that we keep the structure as is and address that in another PR, if that sounds reasonable
Show all checks

SGTM, I like that layout as well. Getting this in unblocks some other work, so I think we should proceed and change the layout in a followup.

@dlorenc dlorenc merged commit 5f529ec into kubernetes:master May 4, 2016
@luxas
Copy link
Member

luxas commented May 4, 2016

Yay! Thanks @mfburnett and @ethernetdan!
And yeah, it's better to proceed with the folder structure etc. in other PRs

@vishh
Copy link
Contributor

vishh commented May 4, 2016

Woot! Thanks @ethernetdan !

jimmidyson pushed a commit to jimmidyson/minikube that referenced this pull request Sep 2, 2016
…true --router=true

Incorporate changes suggested by @jimmidyson
jimmidyson added a commit to jimmidyson/minikube that referenced this pull request Sep 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants