Skip to content

Add option to Docker provisioner to build local images #2615

Merged
merged 2 commits into from Jan 14, 2014

4 participants

@jpfuentes2

Option would work like so:

Vagrant.configure("2") do |config|
  config.vm.provision "docker",
    images: %w( ubuntu ),
    build_images: [
      { path: "Dockerfile", args: "-t me/my_app" }
    ]
end

Please be aware this is my first time hacking on Vagrant and I was kind of rushed : )

I didn't see any tests regarding this provisioner on mitchellh/vagrant#2549. Please let me know if I need to add something.

Also, is there documentation other than the README for hacking on Vagrant? I'm wondering what the normal work flow is for testing this feature I added. Would I just create a Vagrantfile with my new feature and run bundle exec vagrant up?

Thanks!

@fgrehm fgrehm was assigned Dec 9, 2013
@fgrehm
Collaborator
fgrehm commented Dec 9, 2013

@jpfuentes2

Thanks! :) I was actually going to work on that but you've beat me to it, so I'm 👍 for the feature and I'm assigning it to myself for review. I'll try to have a look at it tomorrow but I'm not 100% sold on that DSL yet (see below for more).

As per testing the provisioner, there was originally some tests for it back in vocker but I decided to drop them since I knew the API would change and I was using another mocking framework over there. But will work on backporting them as soon as I have a chance.

re: Hacking on Vagrant, I think the README is all we've got for now, if someone else has any other pointers please raise your hand :P /cc @phinze @tmatilai @gildegoma

@mitchellh do you have any thoughts on the feature / DSL? This is what I had in mind when I first thought of this feature:

config.vm.provision "docker" do |d|
  d.build_image 'image:optional-tag', '<full path to Dockerfile on guest>', 
                                      args: '<build args>'
end

WDYT? (@jpfuentes2 feel free to share your thoughts too :)

@fgrehm fgrehm and 1 other commented on an outdated diff Dec 9, 2013
plugins/provisioners/docker/config.rb
@@ -20,6 +21,14 @@ def pull_images(*images)
@images += images.map(&:to_s)
end
+ def build_images(*images)
+ possible_options = @build_options.keys
@fgrehm
Collaborator
fgrehm added a note Dec 9, 2013

@jpfuentes2 before I forget, this variable doesn't seem to be used :)

@jpfuentes2
jpfuentes2 added a note Dec 9, 2013

Nice catch! I went through a few iterations and decided there was no need for a generic options hash. I'll remove it.

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

I'm 👍 on your suggested DSL. It seems like Vagrant prefers multiple method calls instead of passing an array so I think your plan is superior. It concisely represents common docker build usage and inclines users to always name their images which is something I do regardless.

@fgrehm
Collaborator
fgrehm commented Dec 10, 2013

Yeah, the idea of having the image tagged at all times is good to avoid having untagged images laying around and to allow us to do configuration merging from within Vagrant with less pain as they'll be stored as a Hash on the config object :)

So, I slept on this and figured out we probably need to do one more thing before bringing it in that is check whether the image is already available and skip building so that the provisioner is idempotent. On the best case scenario this doesn't represent an issue because Docker will reuse cached images, but if we have an ADD instruction along the way for example, further instructions won't be cached and a new image will be built on every vagrant provision run. Does that make sense?

Apart from that I think we should be good to go :)

@mitchellh and others, are you guys up for bringing this in on 1.4.x?

@jpfuentes2

check whether the image is already available and skip building so that the provisioner is idempotent

When using Chef/Puppet provisioners I can make changes to the configuration of those systems, run vagrant provision and see my new changes applied immediately without having to vagrant ssh and do manual invocations in the VM. If we skip building existing images then I would be forced to bump the tag version of the image to force the build. I'm not sure that would be expected behavior, would it?

It was my understanding that Vagrant stopped provisioning on every vagrant up because some people use Shell provisioners and those are generally not idempotent. Implied here is that Vagrant presumes non-idempotent provision behavior by default. However, users can vagrant provision to their hearts desire afterwards respective of configuration updates.

@fgrehm
Collaborator
fgrehm commented Dec 12, 2013

@jpfuentes2 Yeah, it makes sense but I'm still on the fence... gonna have to think about it a bit more.

We just need to be consistent at least, if we end up building images on every provisioner run, we should also pull them on every run as well instead of skipping it as we currently do.

ping @mitchellh

@goshakkk

👍 on this feature with @fgrehm's suggested DSL.

@mitchellh
Owner

LGTM. Great job!

@mitchellh mitchellh merged commit 579cc8e into mitchellh:master Jan 14, 2014

1 check passed

Details default The Travis CI build passed
@mitchellh
Owner

I'm updating it with the newer DSL now.

@fgrehm
Collaborator
fgrehm commented Jan 28, 2014

@mitchellh I noticed you ended up dropping the second parameter for always tagging the image that I proposed in favor of the args + -t option when you updated to the new DSL =(

Was that intentional?

@mitchellh
Owner

I think so, thinking back I think it was because you can just use "args" to do that. Thoughts?

@fgrehm
Collaborator
fgrehm commented Jan 28, 2014

Well, it depends on what are your thoughts on #2901, if we agree on building the image only if doesn't exist, I'd say it'll be nice to have the parameter in place so we can perform the checking.

Either way, as @jpfuentes2 pointed out above, I also believe it represents the most common usage for docker build :-)

@jpfuentes2

Yeah, I think it's worth considering my comment here: #2615 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.