Sync state with hosts file in blocks

[pbitty]

Pull request #45 added logic to remove destroyed machines from the hosts file. However, it seems that it only works if you have include_offline = true.

When include_offline is false, the machine being destroyed is not returned by get_machines in hosts_file.rb:63.

This got me thinking of how to clean up the hosts file without any knowledge of machines and names that we may have put in there in the past.

I'm thinking about the idea of delineating a block in the hosts file that belongs to a specific vagrant environment. To sync with the hosts file, we would only have to replace the whole block instead of replacing specific lines. This way we always push the current machine state, without needing to know about what to remove.

Something like this:

127.0.0.1 localhost
...
## vagrant-hostmanager-start id: bf507131-14ba-4c27-805d-f6c67bdd9580
10.0.0.1 machine1 alias1.example.com
10.0.0.2 machine2 alias2.example.com
10.0.0.3 machine3 alias3.example.com
## vagrant-hostmanager-end

This would require a way to uniquely identify this block. One way this could be done is to generate a UUID and store it in env[:local_data_path], which is where vagrant keeps state attached to a specific Vagrantfile.

@smdahlen @bdcribbs , what do you think of this?

[pbitty]

With these changes, the hosts file looks like this after booting up the machines in the test folder:

127.0.0.1 localhost

## vagrant-hostmanager-start id: 67bb5a2b-fd53-4aa0-9f5c-27adefc621c3
## Vagrantfile location: C:/Users/pbitty/Dropbox/dev-projects/smdahlen-vagrant-hostmanager/test
10.0.5.2    fry test-alias
10.0.5.3    bender 
## vagrant-hostmanager-end

When all machines are destroyed, the whole block goes away.

What do you guys think?

[pbitty]

@smdahlen Are you ok with me merging this? I'm asking because it's a bit of a significant change.

[smdahlen]

I'll take a look tomorrow morning and let you know.

[pbitty]

Hi @smdahlen, I am eager to merge this. I've been using it on my own branch it's working well. If you don't have time to test it out, could you tell me if you agree with the concept?

This would fix issues where old entries end up left behind in the /etc/hosts (see #58), sometimes blocking new entries that use the same IP.

[smdahlen]

@pbitty Sorry, just got around to review. I think the concept is sound and think it is worth merging. After the merge, I'll go ahead and push out a new gem. Probably worth a bump on the minor version.

[elyast]

@pbitty @smdahlen I have one issue with implementation, I don't like the idea to have in /etc/hosts on cloud machines path to my local Vagrantfile, imagine I'm sharing this with some other colleagues and his git repo is in different place, basically the info line will be changing depending on which user is invoking hostmanager.

I wasn't sure why the id is really needed since header and footer should be enough. One problem could be if:

• someone wouldn't commit .vagrant/hostmanager/id, but he has committed .vagrant/machine/aws/id e.g.
• other user would invoke hostmanager for the very same machine
• since id would be different, we would end up with duplicated /etc/host

[pbitty]

@elyast The path to the Vagrantfile is definitely not necessary. I put it there to make it easier to know where the vagrant-hostmanager block came from. This is really only useful in the host machine's /etc/hosts file.

The ID is actually necessary, but only on the host machine. If you have multiple vagrant setups on your machine, this allows vagrant-hostmanager to manage the different independent blocks in the hosts file.

Maybe what we need is to have slightly different behavior for editing /etc/hosts in the guest and in the host machines. For the guest machine, we'd have no ID or Vagrantfile path, as you suggested.

Does that sound like a reasonable solution? I am willing to make that change.

Let me know what you think.

[elyast]

@pbitty Sounds reasonable :)

[pbitty]

@elyast Sorry for the delay, been a busy week.
I've made the changes, and also broke up the code into smaller methods. Would you be up for reviewing & testing this change? It would be nice to have a second pair of eyes on it.
Once I've merged this I can get through the rest of the PRs.

[elyast]

Sure do I will let you know

[elyast]

Hi, I merged your branch to my fork and it works fine.

I also looked at the code, and I think it's fine, it's maybe a little bit deep level of invocation, but on the other hand methods are small and clean.

Thanks

[pbitty]

@elyast Thanks for the test and feedback.

I agree the level of invocation is a bit deep, but I'll leave refactoring for when more changes are required. :)