tmatilai / vagrant-proxyconf

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

Test for npm availability in sudo context #59

Closed ghost closed 10 years ago

ghost commented 10 years ago

npm proxy provider runs @machine.communicate.sudo(command).

cap provider verifies availability of npm with `which' NOT using sudo.

This may result in:

This may result in the following error message breaking the Vagrant environment completely:

The following SSH command responded with a non-zero exit status. Vagrant assumes that this means the command failed!

npm config set proxy http://some.configured.proxy.here:8080/

Stdout from the command:

Stderr from the command:

bash: line 2: npm: command not found

tmatilai commented 10 years ago

Oh, I see, good catch!

But the proposed solution feels a bit suboptimal, as it skips the configuration altogether in your case. I think a better approach would be to let the cap return the path from which npm and use it in the configuration action. What do you think?

ghost commented 10 years ago

I thought of that but didn't have the time here to implement that including tests and everything. This might be true for other proxy configuration as well and this in turn might require a general purpose which-path-to-variable solution used for any proxy configuration which uses the machine.communicate.sudo(command) call.

tmatilai commented 10 years ago

He, understood. So lets go with a minimal first aid fix and add the more complete solution to the backlog.

But please change the cap like this and probably fix the tests accordingly:

machine.communicate.test('which npm', sudo: true)

Or I can do it later today if you are busy.

tmatilai commented 10 years ago

Merged, thanks!