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

Bundle localkube in the minikube binary as a blob, send that to the VM. #66

Merged
merged 7 commits into from
May 13, 2016

Conversation

dlorenc
Copy link
Contributor

@dlorenc dlorenc commented May 12, 2016

This PR changes the Makefile a bit to do proper dependency management.

It now builds localkube (via docker on Mac) before the minikube build, then packs it using go-bindata. Minikube uses native scp to transfer this file into the VM, and starts that binary instead of downloading it over the internet.

cc @luxas

Also cleanup the Makefile quite a bit. We not build localkube in docker on mac,
then use go-bindata to pack it up. Users will need go-bindata on their paths to build now,
though.
@@ -25,3 +25,5 @@ _testmain.go

/out
/.gopath

pkg/minikube/cluster/localkubecontents.go
Copy link
Member

Choose a reason for hiding this comment

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

nit: newline

- Install go-bindata automatically.
- Newlines
- Change to Go 1.6.
fmt.Fprint(w, "\x00")
}()

scpcmd := fmt.Sprintf("sudo /usr/local/bin/scp -t %s", remotedir)
Copy link
Member

Choose a reason for hiding this comment

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

What about if scp is located in another location?
Maybe use a shell or query which scp first.
(This isn't so important...)

@@ -154,6 +156,24 @@ func StartCluster(h sshAble, config KubernetesConfig) error {
return nil
}

func UpdateCluster(d drivers.Driver) error {
localkube, err := Asset("out/localkube")
Copy link
Member

Choose a reason for hiding this comment

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

Maybe move this to some constant? So we have only one place to update if we change the path in the future

@luxas
Copy link
Member

luxas commented May 13, 2016

This LGTM.
Some nits here and there but we'll fix these in followups

Merging to move forward

@luxas
Copy link
Member

luxas commented May 13, 2016

Ah, wait. Travis returned false information again.
Fix this issue first:

# k8s.io/minikube/pkg/minikube/cluster
../../../k8s.io/minikube/pkg/minikube/cluster/cluster.go:160: undefined: Asset
FAIL    k8s.io/minikube/cmd/minikube/cmd [build failed]

@dlorenc
Copy link
Contributor Author

dlorenc commented May 13, 2016

Ah, wait. Travis returned false information again.
Fix this issue first:

Ah whoops, I think I got it this time.

cp $(BUILD_DIR)/localkube-$(GOOS)-$(GOARCH) $(BUILD_DIR)/localkube
out/localkube: .gopath
ifeq ($(GOOS),linux)
CGO_ENABLED=1 go build -v -ldflags="-s" -o $(BUILD_DIR)/localkube ./cmd/localkube
Copy link
Member

Choose a reason for hiding this comment

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

The -v here makes it very verbose all the time.
Maybe add a VERBOSE flag to the makefile so we may control it?
I may do this later as well.

@luxas
Copy link
Member

luxas commented May 13, 2016

Seems like a gofmt failure

@dlorenc
Copy link
Contributor Author

dlorenc commented May 13, 2016

Seems like a gofmt failure

Had to add the generated file and the .gopath directory to the whitelist. Let's see if we get farther this time.

@luxas
Copy link
Member

luxas commented May 13, 2016

LGTM, merging...

@luxas luxas merged commit 6865446 into kubernetes:master May 13, 2016
@dlorenc dlorenc deleted the buildlocalkube branch May 14, 2016 19:04
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.

None yet

3 participants