tmatilai / vagrant-proxyconf

Vagrant plugin that configures the virtual machine to use proxies
MIT License
531 stars 74 forks source link

Detect chef provisioners by type instead of by name (multi-machine), fix #101 #103

Closed jperville closed 9 years ago

jperville commented 9 years ago

@tmatilai, here is my modest attempt at fixing #101 (where chef provisioner detection was broken in multi-machine Vagrantfiles). I have tested by hand that the PR works in both single-machine and multi-machine modes, with the following Vagrantfile:

# -*- mode: ruby -*-
# vi: set ft=ruby :

Vagrant.configure("2") do |config|
  # Every Vagrant virtual environment requires a box to build off of.
  config.vm.box      = 'opscode-ubuntu-14.04-chef'
  config.vm.box_url  = 'http://opscode-vm-bento.s3.amazonaws.com/vagrant/virtualbox/opscode_ubuntu-14.04_chef-provisionerless.box'

  provision = lambda { |chef|
    chef.add_recipe "apt::default"
  }

  if String(ENV['MULTIVM']) == 'true'
    config.vm.define "multi" do |machine|
      machine.vm.hostname = 'multi'
      config.vm.provision(:chef_solo, &provision)
    end
  else
    config.vm.hostname = 'single'
    config.vm.provision(:chef_solo, &provision)
  end
end

In single-machine mode:

jp440p:u $ MULTIVM=true vagrant ssh -- 'hostname && grep no_proxy /tmp/vagrant-chef-*/solo.rb'
multi
no_proxy "localhost,127.0.0.1,172.17.42.1,10.0.3.1,192.168.56.1,192.168.33.1,192.168.122.1,192.168.33.1,192.168.33.10.xip.io,192.168.33.10.xip.io:81"

In multi-machine mode:

jp440p:u $ MULTIVM=false vagrant ssh -- 'hostname && grep no_proxy /tmp/vagrant-chef-*/solo.rb'
single
no_proxy "localhost,127.0.0.1,172.17.42.1,10.0.3.1,192.168.56.1,192.168.33.1,192.168.122.1,192.168.33.1,192.168.33.10.xip.io,192.168.33.10.xip.io:81"

(without the PR, it would return nil as the value of the chef no_proxy in the multi-vm case).

Feel free to merge, close #101 and enjoy Christmas @tmatilai.

tmatilai commented 9 years ago

Unfortunately it seems that the type method came only in Vagrant 1.7, and we don't want to break compatibility with older versions yet.

Did the multi-vm setup work with Vagrant 1.6.5? If yes, we could maybe test if prov.respond_to?(:type) and use the correct attribute based on that.

jperville commented 9 years ago

Well spotted, I will investigate more during the week-end.

jperville commented 9 years ago

Here is my updated version of this PR, which is backward-compatible with Vagrant 1.6.5 (and probably older versions). After a quick check, it seems that the issue only applies to the chef proxy, so no related PR should be needed.

I have tested the following cases:

vagrant with fix without fix
1.6.5 pass pass
1.7.1 pass break

Note: results were identical in the single and multi machine cases.

jperville commented 9 years ago

According to mitchellh/vagrant#5069 Vagrant won't provide a backward-compatibility (it would mess the internals too much); plugins must be updated to handle the pre and post vagrant-1.7 cases.

Can you merge this @tmatilai and (if possible) issue an official 1.4.1 release with the recent fixes?

tmatilai commented 9 years ago

Merged! Thanks!

I'll be mostly offline for the next couple of weeks, but I'll release v1.5 as soon as I have an hour of internet with the laptop. =)

jperville commented 9 years ago

Thank you so much @tmatilai, looking forward to using the new version.