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.

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

Windows Proxy #63

Closed
wants to merge 15 commits into from
Closed

Windows Proxy #63

wants to merge 15 commits into from

Conversation

lawsonj2019
Copy link
Contributor

I've added the capability for Windows Guests to have their http_proxy, https_proxy, ftp_proxy, and no_proxy environment variables set via this plugin and the use of explicit config.win_proxy Vagrantfile entries, or through the use of the VAGRANT_WIN_HTTP_PROXY style environment variables.

@tmatilai
Copy link
Owner

Hi James,

This looks totally awesome, thanks!

I was thinking if instead of adding a new config, we could just use env_proxy. What do you think?

Even better would be to move configuration logic from the action class into capabilities and use env_proxy_conf for both windows and linux. But that would require much more intrusive refactoring, and that's out of scope for this PR.

@johnbellone
Copy link
Contributor

@lawsonj2019 Nice job!

I second using env_proxy_conf here.

@lawsonj2019
Copy link
Contributor Author

The env_proxy_conf would probably be the best in the long run. I wanted to keep the configuration separate to not trample on work you were doing, but if we could still use the env_proxy and keep the windows action for this then that would work very well in the short term.

@johnbellone
Copy link
Contributor

Ha, I spoke without actually looking at the code. The refactoring is definitely some effort. @tmatilai Perhaps that something to tackle for #58 as well? I have been meaning to get going on that.

@johnbellone
Copy link
Contributor

That is we merge this and refactor along with adding support to get us around the bug presented in #58. Still early here and not enough coffee :D.

@tmatilai
Copy link
Owner

I'm fine using a separate action for now, but would really like to avoid the extra config class. We already have quite a few...

I have many things and refactoring planned, but been way too busy at work lately. Hope to allocate some love for this project in a couple of weeks. So if @lawsonj2019 can change the code to use env_proxy config, I would merge this and prepare a release next weekend.

@lawsonj2019
Copy link
Contributor Author

I refactored the code to use the env_proxy config but ran into a couple of issues. The main one being that both the Windows and the Linux capabilities were being executed for the env config (on both OSes), probably due to the env_proxy config being enabled. This didn't appear to be too much of a problem on Windows, although a file was being written to the disk, but on Linux instances the Windows command to create the proxies raised an exception, as it wouldn't be able to execute CMD.
In the end I refactored again so that there is no separate action, but a single configure_env_proxy action that decides whether the vagrant machine is a Windows client and executes one path, or another OS and executes the existing path. It's a little more intrusive to the original code than the first pull request, but hopefully lines up a little better with your expectations.
I've tested this with Linux (Ubuntu) and Windows guests (from a Windows host machine with a proxy) with Vagrant 1.5.4 and it appears to be stable, but I'm sure you'll want to do a fair bit of consideration on this.

@tmatilai
Copy link
Owner

@lawsonj2019 thanks! I'll take a closer look on weekend.

It seems that in the first refactoring you renamed the capability to env_proxy_conf. So as that capability is thus supported on both linux and windows, both of the actions were happy to do their magic.

But I'll test and review later. Thanks again!

@tmatilai tmatilai closed this in 707ff4f Apr 27, 2014
@tmatilai
Copy link
Owner

Squashed and merged! I don't have time to set up a Windows guest right now, but looks good to me.

Btw, could we also add support for npm, git, etc. too? Would the commands work as is?

@lawsonj2019
Copy link
Contributor Author

Support for git, npm, etc.should be mostly fine with the addition of the environment variables. I think they both honour the environment variables. If not the same principles as the changes in the configure_env_proxy action could be applied to determine whether it is a Windows machine and apply any specific configuration.

Getting the git/npm executable path as part of the capability would need to be looked at, as this would be different on Windows - the util class in the capabilities would need to cater for Windows searching (unless you just assume git/npm/etc. are on the PATH).

@tmatilai
Copy link
Owner

IIRC the vagrant-windows plugin modifies which commands to be compatible. And at least pear won't support env vars. I think npm had some issues too.

Needs more investocation. Would you happen to know any trusted Packer templates to build a Windows box, preferably for VMware?

@lawsonj2019
Copy link
Contributor Author

Not on the VMWare front. We're looking at packer templates right now for Windows, so if we come up with anything relevant I will let you know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants