search

vagrant-landrush / landrush

A Vagrant plugin that provides a simple DNS server for Vagrant guests
MIT License
668 stars 80 forks source link

Issue #321 Adding enable_route_localnet guest capability #336

Closed hferentschik closed 5 years ago

hferentschik commented 5 years ago

Fix for issue #321

hferentschik commented 5 years ago

@ianmiell, what do you think about this? I had a crack at this issue. I don't think one really has to use the capabilities mechanism here, but I wanted to stick to the theme. WDYT?

ianmiell commented 5 years ago

I'm afraid I ran out of time, and now work somewhere where I've not got permission to contribute (and have less spare time). If memory serves I thought caps might be a bit overkilll.

On Wed, Nov 28, 2018 at 11:07 AM Hardy Ferentschik notifications@github.com wrote:

@ianmiell https://github.com/ianmiell, what do you think about this? I had a crack at this issue. I don't think one really has to use the capabilities mechanism here, but I wanted to stick to the theme. WDYT?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/vagrant-landrush/landrush/pull/336#issuecomment-442408909, or mute the thread https://github.com/notifications/unsubscribe-auth/AGrczZGHeB5vGF-LkF0ZqQl8h7UGhU1eks5uzm5cgaJpZM4Y3LJF .

hferentschik commented 5 years ago

I'm afraid I ran out of time, and now work somewhere where I've not got permission to contribute (and have less spare time).

no worries

If memory serves I thought caps might be a bit overkilll

Sure, it is probably not required. Definitely works without as well. I am a bit undecided on this one. The main reason to go with this approach was to stick with the existing structure of the code. As it stands now, the existing linux capabilities could be combined. The advantage would probably be a more compact and less indirect code. I don't think that using capabilities is otherwise a performance issues or similar. The advantage of the capabilities is that in the case one needs at some stage to differentiate for any of these actions between different Linux distributions, one can easily do so.

Given that the current pull request works, I am wondering whether it makes sense to merge it. Then in a follow up issue one could decide whether one would like to combine the existing Linux capabilities.

tristanbes commented 5 years ago

Hello,

Since this patch is required to be able to use landrush with 18.04, is that possible you tag a version please ? @hferentschik