Skip to content
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.

Sign up for GitHub

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

Fixes #182 and #131 #211

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fixes #182 and #131 #211

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
09ef519
Fix for devopsgroup-io/vagrant-hostmanager#182 devopsgroup-io/vagrant…
amontalban Jul 8, 2016
5244562
Merge remote-tracking branch 'upstream/master'
amontalban Aug 11, 2016
f2f2bcc
Merge remote-tracking branch 'upstream/master'
amontalban Jan 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/vagrant-hostmanager/action/update_all.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ def call(env)
@app.call(env)

# update /etc/hosts file on active machines
if @machine.config.hostmanager.manage_guest?
if @config.hostmanager.manage_guest?
env[:ui].info I18n.t('vagrant_hostmanager.action.update_guests')
@global_env.active_machines.each do |name, p|
if p == @provider
Expand All @@ -41,7 +41,7 @@ def call(env)
end

# update /etc/hosts files on host if enabled
if @machine.config.hostmanager.manage_host?
if @config.hostmanager.manage_host?
env[:ui].info I18n.t('vagrant_hostmanager.action.update_host')
@updater.update_host
end
Expand Down
24 changes: 24 additions & 0 deletions lib/vagrant-hostmanager/config.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,30 @@ def initialize
@ip_resolver = UNSET_VALUE
end

def merge(other)
super.tap do |result|
@enabled = false if @enabled == UNSET_VALUE
other.enabled = false if other.enabled == UNSET_VALUE
result.enabled = [@enabled, other.enabled].any?

@manage_host = false if @manage_host == UNSET_VALUE
other.manage_host = false if other.manage_host == UNSET_VALUE
result.manage_host = [@manage_host, other.manage_host].any?

@manage_guest = true if @manage_guest == UNSET_VALUE
other.manage_guest = true if other.manage_guest == UNSET_VALUE
result.manage_guest = [@manage_guest, other.manage_guest].any?

@ignore_private_ip = false if @ignore_private_ip == UNSET_VALUE
other.ignore_private_ip = false if other.ignore_private_ip == UNSET_VALUE
result.ignore_private_ip = [@ignore_private_ip, other.ignore_private_ip].any?

@include_offline = false if @include_offline == UNSET_VALUE
other.include_offline = false if other.include_offline == UNSET_VALUE
result.include_offline = [@include_offline, other.include_offline].any?
end
end

Copy link

Choose a reason for hiding this comment

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

Being familiar with how a number of other plugins accomplish this, I'm pretty certain it's only supposed to be needed for complex types such as hashes, arrays, or attributes that are based on multiple settings. So I would have thought that only @aliases would have needed to be merged, and perhaps this is masking some other issue?

Copy link
Author

Choose a reason for hiding this comment

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

Hi @electrofelix,

Thanks for your feedback, based on your comment I see that I forgot to add a merge for @aliases, will try to update the PR ASAP.

Regarding the PR itself I'm not a Ruby developer (Nor developer at all) but this patch is to fix the issue when you configure vagrant-hostmanager in the base box and you just do:

vagrant init my/basebox
vagrant up

With this patch you can configure vagrant-hostmanager in the Vagrantfile included in the box and when you do vagrant up it will merge configurations (In case you have configured it in the local Vagrantfile).

Hope it clarifies why I tried to do this.

Thanks!

Copy link

Choose a reason for hiding this comment

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

Fully understand there is an issue around it not merging. The files merged are supposed to be from the box, from the current path and from the users global space ~/.vagrant.d/Vagrantfile.

What I'm saying is that this works out of the box for simple objects for other vagrant plugins, so likely missing something else about how vagrant-hostmanager has been coded in that it's not working by default for the simple true/false settings.

Should only require custom merging for the additional complex data elements that cannot necessarily merge correctly. I'd certainly think it's worth looking closer at the config file that is being used in the snipet below for lib/vagrant-hostmanager/util.rb since taking the config section directly from env.vagrantfile looks suspicious to me.

def finalize!
@enabled = false if @enabled == UNSET_VALUE
@manage_host = false if @manage_host == UNSET_VALUE
Expand Down
2 changes: 1 addition & 1 deletion lib/vagrant-hostmanager/util.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ module Util
def self.get_config(env)
# config_global has been removed from v1.5
if Gem::Version.new(::Vagrant::VERSION) >= Gem::Version.new('1.5')
env.vagrantfile.config
env.vagrantfile.config[:hostmanager]
Copy link

Choose a reason for hiding this comment

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

I suspect this function is part of the problem, in that it's not actually retrieving the correct config block. Rewriting this as the following might solve the problem fully, but I'd need to test to confirm:

def self.get_config(env)
    if Gem::Version.new(::Vagrant::VERSION) >= Gem::Version.new('1.5')
        env[:machine].config
    else
        env[:machine].env.config_global
    end
end

Then change all calls to pass in:

@config = Util.get_config(env)

Using the env that is passed to call(app, env) as opposed to passing in env[:machine].env. This should return the merged config object correctly scoped to have merged not only from individual Vagrantfiles in the correct order, but additionally any provider override blocks defined.

else
env.config_global
end
Expand Down