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

Move hyperkit and none drivers to pkg/drivers, share utils #1941

Merged
merged 1 commit into from
Sep 12, 2017

Conversation

r2d4
Copy link
Contributor

@r2d4 r2d4 commented Sep 10, 2017

Share most of the disk image setup between hyperkit and kvm drivers.
Move and remove a lot of shared configuration between all the in-tree
drivers: kvm, hyperkit, none.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 10, 2017
@codecov-io
Copy link

codecov-io commented Sep 10, 2017

Codecov Report

Merging #1941 into master will decrease coverage by 0.26%.
The diff coverage is 10.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1941      +/-   ##
==========================================
- Coverage   30.38%   30.11%   -0.27%     
==========================================
  Files          76       76              
  Lines        4687     4722      +35     
==========================================
- Hits         1424     1422       -2     
- Misses       3084     3120      +36     
- Partials      179      180       +1
Impacted Files Coverage Δ
pkg/minikube/machine/client_linux.go 26.66% <ø> (ø) ⬆️
pkg/drivers/hyperkit/network.go 77.58% <ø> (ø)
pkg/drivers/hyperkit/iso.go 0% <ø> (ø)
pkg/minikube/cluster/cluster_linux.go 0% <0%> (ø) ⬆️
pkg/drivers/drivers.go 10.76% <10.76%> (ø)
pkg/minikube/drivers/hyperkit/disk.go

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3180e2e...ebbc34d. Read the comment docs.

@r2d4 r2d4 mentioned this pull request Sep 11, 2017
Copy link
Contributor

@dlorenc dlorenc left a comment

Choose a reason for hiding this comment

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

Thanks for this cleanup!

return err
}
defer file.Close()
file.Seek(0, os.SEEK_SET)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you happen to know if we actually need this call? It never made sense to me that we would.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think its in case the file already exists and has contents. The magic boot2docker header that we write in the next lines needs to be the first bytes

if _, err := file.Write(tarBuf.Bytes()); err != nil {
return err
}
file.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

error check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

if err := createRawDiskImage(publicSSHKeyPath(d), diskPath, diskSize); err != nil {
return err
}
if shouldFixPermissions {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a problem with fixing permissions everytime? It should be a no-op I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah let me try that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

files, _ := ioutil.ReadDir(path)
for _, f := range files {
fp := filepath.Join(path, f.Name())
log.Debugf(fp)
Copy link
Contributor

Choose a reason for hiding this comment

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

Add some context to this log or delete it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

deleted

Share most of the disk image setup between hyperkit and kvm drivers.
Move and remove a lot of shared configuration between all the in-tree
drivers: kvm, hyperkit, none.
@r2d4 r2d4 merged commit e6da47f into kubernetes:master Sep 12, 2017
@r2d4 r2d4 deleted the shared-drivers branch September 12, 2017 21:57
aaron-prindle pushed a commit to aaron-prindle/minikube that referenced this pull request Sep 13, 2017
Move hyperkit and none drivers to pkg/drivers, share utils
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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants