search

tmatilai / vagrant-proxyconf

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

Fix proxy disablement #176

Closed codylane closed 5 years ago

codylane commented 5 years ago

Fixes the following issues:

Also I've went through each proxy provider and made sure it works and adjusted anything that was broken either in enabling the proxy and or disabling it. I've added configuration and capabilities as required to get all the proxy things to start working again and I believe I've also added tests where needed to ensure it works as desired. I also overhauled the Vagrantfile.example to make it work with newer versions of Vagrant.

My ruby is rusty and it's not my main language of choice and I'm not familiar with all the rubyisms but I think I've been able to introduce the correct working order without breaking this library too much. Please let me know if you have any questions or concerns about all this. This took a while to sort out and I hope that it helps others as well.

tmatilai commented 5 years ago

Wow, thanks a ton!

As the pull request is so huge, combines many different changes, and I've forgot the codebase, it will take me a while to go through it. Haven't really been using Vagrant nor this plugin in a couple if years now...

At the first glimpse, is it not changing the behaviour of config.*.enabled as described in the readme? The idea of that is to skip touching the machine at all (as it is difficult to avoid loading Vagrant plugins when installed). Setting config.*.http etc. to empty strings or false should indeed clear the configuration.

As an unrelated note, the Travis build has been broken for a long time. I think we can finally drop support (or at least tests) for ancient Vagrant versions. I should have an unfinished branch for fixing the build somewhere. I'll take a look.

codylane commented 5 years ago

Hi @tmatilai - Thank you and I'm glad to help. Thank you for creating this plugin! I apologize for the largish size of the pull request. I wanted to make sure that I took the time to make sure all the proxyconf plugins for different provisioners still worked.

At the first glimpse, is it not changing the behaviour of config.*.enabled as described in the readme? The idea of that is to skip touching the machine at all (as it is difficult to avoid loading Vagrant plugins when installed). Setting config.*.http etc. to empty strings or false should indeed clear the configuration.

I thought so too and I hope that I'm not missing anything but I updated the Vagrantfile in such a way that you can try individual pieces like disabling docker or apt and see if the other plugins work both in configuring and disabling the proxies. There are boolean values in the Vagrantfile in my PR branch that allow you to test the behavior. I tried really hard not introduce change unless I needed to fix something and if I did, I tried to write a test around the behavior. For what it's worth, I did remove "def config" lines from each of the action/*.rb files in favor of using the base.rb version which seems to support both methods as mentioned in the README. Please do let me know if you'd like me to change something or if you have questions about something I did, I'd be glad to fix or discuss.

I too was worried about the Travis stuff and all the versions listed in the travis.yml and I didn't want to remove anything until talking with you. Thanks again and I do hope that we can get this merged in.

On a side note, I'm curious and if you don't mind me asking if you are not using Vagrant anymore what are you using?

tmatilai commented 5 years ago

OK, excellent. I'll try to dive into this. As said, I've lost touch to the code base, so I'll ask dump questions. And of course it's easiest to nitpick non-relevant style issues, so bear with me. =)

I too was worried about the Travis stuff and all the versions listed in the travis.yml and I didn't want to remove anything until talking with you. Thanks again and I do hope that we can get this merged in.

Travis build is now fixed in master. So you could try to rebase on top of it.

On a side note, I'm curious and if you don't mind me asking if you are not using Vagrant anymore what are you using?

I mostly used Vagrant for testing configuration management (namely Chef) stuff. The last 4 or 5 years I've mostly worked on cloud infra, where the built "units" are much more layered and simple, and the need for configuration management as such has been mostly vanished. Normally a couple of lines of shell scripts in Packer or Docker build is enough. Also on my workstation Docker has been enough and very seldom I need real virtual machines.

codylane commented 5 years ago

I still need to rebase and cherry pick up you Gemfle and .travis.yml changes. I'll do that tomorrow. Thanks again for the review, I look forward to resolving these issues as quickly as I can.

Ok I've cherry picked and rebased your Gemfile and travis.yml files.