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

[WIP] Docker provisioner #2549

Merged
merged 14 commits into from Dec 4, 2013

Conversation

Projects
None yet
3 participants
@fgrehm
Collaborator

fgrehm commented Nov 27, 2013

So I started working on porting the code from vocker and although I haven't tested this yet I wanted to kick off the discussion about some aspects about it as the code from the plugin probably won't be merged on its current state.

vagrant docker command

Vocker provides a convenience command for interacting with the Docker installation running within the VM but I'm not sure it makes sense to have that as part of Vagrant core. Besides that, I personally haven't been using it as much as I thought I would.
If you guys think it makes sense I can bring it back in

vagrant-lxc specific code within the provider

I personally like to have isolated Docker instances inside other VMs and I found that "plain old lxc "gives me enough abstraction for that and it makes up for a really awesome alternative to VBox on Linux.

I found my way to make it work from within vagrant-lxc containers but I had to add a conditional on the provisioner code and I'm not sure if Vagrant's core should have provider specific logic within it. I'm happy to drop that as long as we have a way of specifying from the provider that it should be skipped in case the provisioner is used.

Running containers from the Vagrantfile

I'd love some feedback about the code that is already in place. It makes some assumptions about where to place container id files and it also creates the directories used as Docker volumes. While the automatic folder creation is a nice convenience, and I'm not 100% sure if that should be a responsibility of the plugin.

Provisioner specs

I have some tests written on the plugin but I noticed that there is no automated tests for other provisioners and I suppose that there is a reason for that. I'm happy to bring them back if you guys think it is worth

Docs

I haven't written any yet. Should we have that for 1.4 or should we wait a bit more as things are subject to change?


There's probably more stuff you guys will find on the code so please take your time to review :)
I plan to do some testing on it later on tonight or tomorrow and I'm up for doing whatever it is needed to get this into 1.4 :D

@mitchellh

This comment has been minimized.

Show comment
Hide comment
@mitchellh

mitchellh Nov 28, 2013

Member

This looks great! I agree that the command should not be included.

Other than that I'm super happy with the state of this. :)

Member

mitchellh commented Nov 28, 2013

This looks great! I agree that the command should not be included.

Other than that I'm super happy with the state of this. :)

@ghost ghost assigned fgrehm Nov 28, 2013

@thasmo

This comment has been minimized.

Show comment
Hide comment
@thasmo

thasmo Nov 28, 2013

So this provisioner will create docker images on UNIX-like platforms, but not Windows, right?

thasmo commented Nov 28, 2013

So this provisioner will create docker images on UNIX-like platforms, but not Windows, right?

@fgrehm

This comment has been minimized.

Show comment
Hide comment
@fgrehm

fgrehm Nov 28, 2013

Collaborator

@thasmo good catch, we need to add some check to prevent users from trying to run it on guests that are not linux 👍

Collaborator

fgrehm commented Nov 28, 2013

@thasmo good catch, we need to add some check to prevent users from trying to run it on guests that are not linux 👍

@fgrehm

This comment has been minimized.

Show comment
Hide comment
@fgrehm

fgrehm Nov 28, 2013

Collaborator

@thasmo BTW, it won't build the images with the current code, users will be able to pull / run images available on the public index

Collaborator

fgrehm commented Nov 28, 2013

@thasmo BTW, it won't build the images with the current code, users will be able to pull / run images available on the public index

@fgrehm

This comment has been minimized.

Show comment
Hide comment
@fgrehm

fgrehm Nov 29, 2013

Collaborator

Quick update: I've done some testing on this last night and things seems to be working fine for the lxc provider, I'll give it a go on a couple VBox VMs later on today just to make double check that things are ok over there too.

If someone could give it a go on a vmware machine it would be awesome :)

Collaborator

fgrehm commented Nov 29, 2013

Quick update: I've done some testing on this last night and things seems to be working fine for the lxc provider, I'll give it a go on a couple VBox VMs later on today just to make double check that things are ok over there too.

If someone could give it a go on a vmware machine it would be awesome :)

@mitchellh

This comment has been minimized.

Show comment
Hide comment
@mitchellh

mitchellh Nov 30, 2013

Member

@fgrehm I'm going to be away from a computer (except for issue responses and such) until early next week so I'll give it a try then. :)

Member

mitchellh commented Nov 30, 2013

@fgrehm I'm going to be away from a computer (except for issue responses and such) until early next week so I'll give it a try then. :)

@mitchellh

This comment has been minimized.

Show comment
Hide comment
@mitchellh

mitchellh Dec 3, 2013

Member

@fgrehm I made some changes here and there. Please check them out. I simplified it a bit. I'll be adding docs.

The best part though is all the core functionality worked great

Member

mitchellh commented Dec 3, 2013

@fgrehm I made some changes here and there. Please check them out. I simplified it a bit. I'll be adding docs.

The best part though is all the core functionality worked great

@fgrehm

This comment has been minimized.

Show comment
Hide comment
@fgrehm

fgrehm Dec 3, 2013

Collaborator

Thanks for the heads up, I'll have a look at it tomorrow :)

Collaborator

fgrehm commented Dec 3, 2013

Thanks for the heads up, I'll have a look at it tomorrow :)

@mitchellh

This comment has been minimized.

Show comment
Hide comment
@mitchellh

mitchellh Dec 4, 2013

Member

I think this is good for merge. Feel free to nitpick on master. :)

GREAT JOB @fgrehm!!! First major feature! :)

Member

mitchellh commented Dec 4, 2013

I think this is good for merge. Feel free to nitpick on master. :)

GREAT JOB @fgrehm!!! First major feature! :)

mitchellh added a commit that referenced this pull request Dec 4, 2013

@mitchellh mitchellh merged commit 146bc34 into master Dec 4, 2013

@mitchellh mitchellh deleted the f-docker-provisioner branch Dec 4, 2013

@thasmo

This comment has been minimized.

Show comment
Hide comment
@thasmo

thasmo Dec 4, 2013

Awesome! :)

thasmo commented Dec 4, 2013

Awesome! :)

@fgrehm

This comment has been minimized.

Show comment
Hide comment
@fgrehm

fgrehm Dec 4, 2013

Collaborator

🎆 🎆 AWESOME :) 🎆 🎆

I'll give it a spin later on tonight. Thanks for bringing it in 😃

Collaborator

fgrehm commented Dec 4, 2013

🎆 🎆 AWESOME :) 🎆 🎆

I'll give it a spin later on tonight. Thanks for bringing it in 😃

@fgrehm

This comment has been minimized.

Show comment
Hide comment
@fgrehm

fgrehm Dec 4, 2013

Collaborator

@mitchellh should we tag the provisioner as experimental for the 1.4 series just to make clear that there might be unknown issues and the api might change or you think that's somehow "implicit"? :P

@mitchellh should we tag the provisioner as experimental for the 1.4 series just to make clear that there might be unknown issues and the api might change or you think that's somehow "implicit"? :P

This comment has been minimized.

Show comment
Hide comment
@mitchellh

mitchellh Dec 4, 2013

Member

Haha, no. All of Vagrant 1.x is technically experimental until 2.0. Have faith! ;)

Member

mitchellh replied Dec 4, 2013

Haha, no. All of Vagrant 1.x is technically experimental until 2.0. Have faith! ;)

This comment has been minimized.

Show comment
Hide comment
@fgrehm

fgrehm Dec 4, 2013

Collaborator

👍

Collaborator

fgrehm replied Dec 4, 2013

👍

@fgrehm

This comment has been minimized.

Show comment
Hide comment
@fgrehm

fgrehm Dec 4, 2013

Collaborator

@mitchellh this is only true for "Debianoid" guests, should we add a note about that?

@mitchellh this is only true for "Debianoid" guests, should we add a note about that?

This comment has been minimized.

Show comment
Hide comment
@mitchellh

mitchellh Dec 4, 2013

Member

I think the error message when it doesn't work should be clear enough. Or, I hope.

Member

mitchellh replied Dec 4, 2013

I think the error message when it doesn't work should be clear enough. Or, I hope.

This comment has been minimized.

Show comment
Hide comment
@fgrehm

fgrehm Dec 4, 2013

Collaborator

No worries, let's see how it goes :)

Collaborator

fgrehm replied Dec 4, 2013

No worries, let's see how it goes :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment