Refactor: separate go-virtualbox package by riobard · Pull Request #70 · boot2docker/boot2docker-cli

riobard · 2014-03-15T00:41:57Z

I factored out interactions with VirtualBox into a separate package (https://github.com/riobard/go-virtualbox). Hopefully the code for boot2docker-cli is nice, clean and type-safe now.

Additionally, this could be the first step to allow another VM solution to be used (Hyper-V on Windows?) by hooking in another package (e.g. go-hyperv)

Make verbose and vbm pointers so we can do late binding and pass them to other packages.

gmlewis · 2014-03-15T04:35:24Z

Should this package be part of this project? It is difficult to review when it is elsewhere.

I usually use git-subtree to include 3rd-party dependency. I'm not sure how I'm supposed to do it here. Any suggestions?

I was thinking of just making a subdirectory within this project, something like:

vbx "github.com/boot2docker/boot2docker-cli/go-virtualbox"

That way we can keep all the dependencies within the same project.

Yep, that's exactly what you get with git-subtree :)

please, don't use git subtrees for this, follow the example that the docker project uses for dependencies.

given that the virtualbox module is the important sauce for boot2docker-cli, please do not move it out of this repository - otherwise we'll have to fork it to maintaine it.

I was hoping that by separating the go-virtualbox package out into its own repo, other people can use and improve it too, and we benefit as a result. The Docker approach seems to be put external dependencies into a vendor directory. Do you want me to add the dep into a similar folder?

if we separate out all the important parts of boot2docker-cli, then we have no way to maintain the code we're using. Please keep the important stuff - and go-virtualbox is important, here. Other people can use and improve it here too.

Its way too early to make it hard for the b2d-cli contributors to refactor the code.

OK, I'll move it here in a moment.

Reuse the flag package to parse the configuration profile so we do not need to use a separate pacakge. The trick is to transform the key=value pairs in the profile into string slice of the form --key=value, then insert the string slice between os.Args[0] and os.Args[1:]. The flag.Parse() will take care of the rest. Less work to do and more consistent results. Win-win! Additionally, changed the type of some flags to reflect its nature.

This is to wait for the VM to properly boot. Initial booting will take a bit longer due to the need to format disk and configure other settings, during which time the SSH daemon might not be reachable.

SvenDowideit · 2014-03-16T23:02:21Z

Why are you forking this library, and then into your own repo?

There are changes to the pflag package I made that hasn't been merged to the upstream yet. See ogier/pflag#4

in that case, please put the modified version into this repo - else b2d will have to do it all again if you're busy.

Do you want to make the pflag package part of the repo in the same way as the go-virtualbox package, or do you want it to be part of the 3rd-party vendor directory?

I would hope that the pflag PR gets merged (or fixed until it does..) and its not really core to what boot2docker is, so I'd vendor that

Makes sense. I'm not very sure how Docker manages the vendor directory. Could you explain a little bit what I should do to vendor it?

The pflag PR has been merged.

riobard · 2014-03-17T01:10:42Z

For the included virtualbox package, do we use absolute import path? e.g.

import vbx "github.com/boot2docker/boot2docker-cli/virtualbox"

I'm wondering where do you guys put your forked repo? Do you put it under $GOPATH/src/github.com/boot2docker/boot2docker-cli or do you put it under $GOPATH/src/github.com/{YOUR_USERNAME}/boot2docker-cli? I ran in to a problem in the latter case where I cannot import the included package (because the username is different) and wanted to know how you solve this problem. Thanks!

gmlewis · 2014-03-22T22:10:47Z

// A NATNet defines a network.

gmlewis · 2014-03-22T22:21:20Z

After addressing my comments as you see fit, LGTM.

SvenDowideit · 2014-03-24T04:01:04Z

LGTM

Refactor: separate go-virtualbox package

This reverts commit 0fbe0f7, reversing changes made to ea966f9.

Revert "Merge pull request #70 from riobard/refactor/vbx"

SvenDowideit · 2014-03-24T04:36:33Z

@riobard I don't know what went wrong - I suspect you will need to make a new PR, as github is a little primitive when things go bad

Revert "Revert "Merge pull request #70 from riobard/refactor/vbx""

…k-not-UTC virtual box / set hwclock in VM to not beeing UTC

riobard added 3 commits March 14, 2014 21:45

Factored out VirtaulBox details into another package

c345dcb

Fixed flag usage cases.

b4e268a

Fixed global flags

cf362fa

Make verbose and vbm pointers so we can do late binding and pass them to other packages.

gmlewis reviewed Mar 15, 2014
View reviewed changes

riobard added 13 commits March 15, 2014 10:11

Assorted fixes per code review

c51ca36

Waiting for VM to stop is handled by the go-virtualbox package

b4c2a49

Use NIC network and hardware constants

cd92a07

Print command executed in verbose mode

46c16e7

Fixed profile example in README.

7911865

Fixed default value for iso and dir flags.

922dd7b

Prefer shorter names.

1640ee1

Use forked pflag package with more data types

0f88388

Used forked pflag package.

7eb5b84

Fixed 3rd-party import url

f0d938f

Improved PFRule data types

68e16cb

Fixed: repetively connect to SSH port at interval

2209756

This is to wait for the VM to properly boot. Initial booting will take a bit longer due to the need to format disk and configure other settings, during which time the SSH daemon might not be reachable.

SvenDowideit reviewed Mar 16, 2014
View reviewed changes

Moved in the code from package go-virtualbox

0ce19b5

riobard added 2 commits March 16, 2014 22:44

Fixed import URL

3499c18

Use drive type and storage controller chipset constants

cecbcec

gmlewis reviewed Mar 22, 2014
View reviewed changes

Comment thread virtualbox/natnet.go Outdated

Copy link
Copy Markdown

Contributor

gmlewis Mar 22, 2014

Choose a reason for hiding this comment

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

// A NATNet defines a network.

riobard added 6 commits March 22, 2014 20:54

Assorted fixes for code review

c5f22c2

vbmOut/vbmOutErr return string instead of []byte

eb328fe

Fixed const declarations.

51f292b

Code review fixes

d819615

Minor fixes.

acfe8f1

Removed useless comments.

00f480e

SvenDowideit added a commit that referenced this pull request Mar 24, 2014

Merge pull request #70 from riobard/refactor/vbx

0fbe0f7

Refactor: separate go-virtualbox package

SvenDowideit merged commit 0fbe0f7 into boot2docker:master Mar 24, 2014

SvenDowideit added a commit to SvenDowideit/boot2docker-cli that referenced this pull request Mar 24, 2014

Revert "Merge pull request boot2docker#70 from riobard/refactor/vbx"

0c4effd

This reverts commit 0fbe0f7, reversing changes made to ea966f9.

SvenDowideit added a commit that referenced this pull request Mar 24, 2014

Merge pull request #71 from SvenDowideit/revert-vbm-70

788d1db

Revert "Merge pull request #70 from riobard/refactor/vbx"

SvenDowideit mentioned this pull request Mar 24, 2014

Revert "Merge pull request #70 from riobard/refactor/vbx" #71

Merged

riobard added a commit that referenced this pull request Mar 31, 2014

Merge pull request #75 from riobard/fix/revert

d88b216

Revert "Revert "Merge pull request #70 from riobard/refactor/vbx""

riobard mentioned this pull request Apr 17, 2014

Use Make to build binaries #88

Merged

SvenDowideit pushed a commit to SvenDowideit/boot2docker-cli that referenced this pull request Nov 10, 2014

Merge pull request boot2docker#70 from aheissenberger/hofix/vm/hwcloc…

6b9ffa7

…k-not-UTC virtual box / set hwclock in VM to not beeing UTC

Conversation

riobard commented Mar 15, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

riobard commented Mar 17, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmlewis commented Mar 22, 2014

Uh oh!

SvenDowideit commented Mar 24, 2014

Uh oh!

SvenDowideit commented Mar 24, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants