landrush-ip integration

[Werelds]

This PR integrates landrush-ip into Landrush as discussed in pull request #122 which is superseded by this pull request. This is a fix for issue #114.

Currently only Linux is supported, further OS support will follow once this PR and the API it proposes is accepted.

It currently consists of 2 commits (one for the base integration, one for Cucumber tests). It adds two new config settings: config.landrush.exclude = [/regex1/, /regex2/, ...] and config.landrush.interface = 'nic' as described in the updated README.

[bexelbie]

Can we get an explicit example/docs around a situation that doesn't specify any specific nic or any excludes, ala just this:

config.landrush.enabled = true
config.landrush.tld = 'landrush-acceptance-test'

[Werelds]

I think the README is pretty clear about it? It explicitly says to use the interface option if the automatic detection fails and that the default exclude consists of loopback, tunnel and Docker interfaces. On guests that have no tunnels or Docker installed that will make no difference, whereas on the ones that do it allows Landrush to actually get the correct IP. Those interfaces could never be used as the target for the DNS anyway.

I'm happy to add an extra example in, personally I don't think it's necessary though.

[bexelbie]

I feel like features/landrush-ip.feature made it unclear. Maybe a paragraph there describing how auto-detection will work?

[Werelds]

I'll add that to the README then. It's basically just a one-liner though: Landrush picks the last IP found, minus any excluded interfaces (obviously).

The ones in the Cucumber feature test are a little different, as we don't want to provision a VM to install some tunnels and/or Docker just to test the feature, that's why I used fixed IPs there and did more explicit excludes.

[bexelbie]

sounds good

[hferentschik]

@Werelds this is looking awesome. It applied nicely on master and passes all tests and features.

Only comments I have are around the config property names and the way the Cucumber test is written. WDYT?

[Werelds]

Yeah I think we could make that work for the Cucumber tests; the only problem is that we'll then also have to implement the Then separately per test box and/or separate per box. I.e. for Windows it's very different than it is for Linux; but even on Linux we'd have to make sure to install ip or ifconfig, which is different in every distro...bringing us back to exactly what we're trying to avoid.

The thing is, changing it to DHCP makes no difference to the capability's mechanics, which is what we're testing here. The capability and binary have no knowledge up front about these IP addresses, only the test does. And in my opinion, that's exactly what a test needs to be/do: know exactly what goes in and what should come out.

[hferentschik]

the only problem is that we'll then also have to implement the Then separately per test box and/or separate per box. I.e. for Windows it's very different than it is for Linux;

The Cucumber tests are already not running against all machines. They won't run on Windows at all due to a bug in Aruba (work in progress to fix this) and they won't even work on all Linux variants. For example the step definition for verification of DNS on the guest is using dig which is for example not per default installed on CentOS (I have a little workaround to improve the situation, but still ...).

I don't think we can assume that these tests run for any odd box. I also think that we cannot add too many boxes per default since running the tests takes a lot of time. The important thing is that we have a framework where one can easily test end to end.

Returning to your test. I'd rather see the test using dhcp and a new step definition which makes the test and feature more explicit than trying to think to run against as many boxes as possible. I'd just choose a box (eg debian or ubuntu) where I know the tools I need to do the step verification are installed out of the box.

[Werelds]

Processed the suggested changes above @hferentschik and @bexelbie (although I see I mentioned the wrong issue number in the commit message :P).

landrush-ip has some experimental Windows support, but I didn't include that yet because I think it's better to focus on Linux first. Plus, I need someone with more experience running all sorts of Windows boxes to give it a whirl. I'm not sure how the machine communication works exactly; whether it's guaranteed to be PowerShell or not etcetera.

[hferentschik]

Merged :-) Thanks for hanging in there. I think this is a big improvement for Landrush and I really like how it shaped up in the end.

[hferentschik]

BTW, I took the liberty to squash the commits in this case

[Werelds]

No problem!

I think it might be a good idea to transfer Werelds/landrush-ip to vagrant-landrush/landrush-ip as well, so that it sits in the correct organisation and control is where it belongs.

[hferentschik]

@Werelds, so here is something I did not realize. The current approach does require that the user does a 'vagrant plugin install landrush-ip' as well, right? For some reason I thought this would somehow happen automatically, but it doesn't. It works in development, because we put landrush-ip into the plugin group, but at runtime when you use Landrush as a installed plugin of your Vagrant installation you need the landrush-ip plugin as well, right?

Given that landrush-ip is not optional, it seems odd that the user needs to install both. I was hoping I could make landrush-ip a direct dependency of landrush and somehow "load" the landrush-ip plugin directly, but that does not seem to work. I am not quite sure how to handle this. @Werelds Any ideas?

[Werelds]

That's a good point actually. I have no idea whether that's even possible; perhaps better to ask this on the Vagrant repository? I would've thought that having it in the plugins group would also install it when not in development.

If it's not possible (yet) we might be able to install it in the background via Vagrant's API perhaps?

[hferentschik]

That's a good point actually. I have no idea whether that's even possible

I think I found a way. See pull request #206. Commit https://github.com/vagrant-landrush/landrush/pull/206/commits/b223f4d20eacfcff5eb4f134ca96d42242b3fd32. That seems to work. So landrush-ip is a actual dependency of landrush and I require it in the top level file.

I would've thought that having it in the plugins group would also install it when not in development.

That was my thinking as well ;-) But the plugin group is only a development thing.

[Werelds]

That'll do I guess!