Skip to content

Add Go wrapper support for different disk formats and drivers.#147

Merged
rn merged 1 commit intomoby:masterfrom
dlorenc:disks
Jul 12, 2017
Merged

Add Go wrapper support for different disk formats and drivers.#147
rn merged 1 commit intomoby:masterfrom
dlorenc:disks

Conversation

@dlorenc
Copy link
Copy Markdown
Contributor

@dlorenc dlorenc commented Jul 11, 2017

This PR adds simple support for pre-configured disks of different types. Specifically, I'd like to be able to use the go wrapper to create VMs with attached qcow2 and sparsebundle disk types.

Apologies in advance for sending this PR without discussion on an issue first. I'm more than happy to refactor this or approach it in some other way.

Signed-off-by: dlorenc lorenc.d@gmail.com

Comment thread go/hyperkit.go Outdated
a = append(a, "-s", fmt.Sprintf("%d:0,virtio-blk,%s", nextSlot, p.Path))
// Default the driver to virtio-blk
driver := "virtio-blk"
if p.Driver == "" {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be a != ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

D'oh. That's what I get for manually copying the changes back out of my vendor directory...

Comment thread go/hyperkit.go Outdated

// Add on a format instruction if specified.
if p.Format != "" {
arg += fmt.Sprintf(",format=%s", p.Format)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think this can be just arg += ",format=" + p.Format and therefore avoid the fmt.Sprintf.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

@dlorenc
Copy link
Copy Markdown
Contributor Author

dlorenc commented Jul 12, 2017

Thanks for the review! Any idea if the cirlcleci failure is related to my change?

@ijc
Copy link
Copy Markdown
Collaborator

ijc commented Jul 12, 2017

clang: error: no such file or directory: '/Users/distiller/.opam/system/lib/io-page/io_page_unix.a'
clang: error: no such file or directory: '/Users/distiller/.opam/system/lib/io-page/libio_page_unix_stubs.a'
make: *** [build/hyperkit.sym] Error 1

Looks like one for @djs55.

@dlorenc if you could squash while he takes a look that'd be great. Your changes LGTM by themselves.

@djs55
Copy link
Copy Markdown
Collaborator

djs55 commented Jul 12, 2017

@dlorenc I think you're missing a build fix in 4afda33 -- rebasing to latest master should hopefully fix the CI.

Signed-off-by: dlorenc <lorenc.d@gmail.com>
@dlorenc
Copy link
Copy Markdown
Contributor Author

dlorenc commented Jul 12, 2017

@djs55 and @ijc thanks! squashed and rebased.

@dlorenc
Copy link
Copy Markdown
Contributor Author

dlorenc commented Jul 12, 2017

Looks like the master rebase did fix CI. Thanks!

Copy link
Copy Markdown
Member

@rn rn left a comment

Choose a reason for hiding this comment

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

Thanks

@rn rn merged commit 62fb993 into moby:master Jul 12, 2017
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.

4 participants