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

Test for npm availability in sudo context #59

Closed
wants to merge 2 commits into from
Closed

Test for npm availability in sudo context #59

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Apr 3, 2014

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

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

This may result in:

  • which succeeds for the ssh user (mostly vagrant)
  • machine.communicate.sudo('npm do something') does not
    succeed because sudo target user does not have the same
    PATH variable configured.

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

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

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

This may result in:
- which succeeds for the ssh user (mostly vagrant)
- machine.communicate.sudo('npm do something') does not
  succeed because sudo target user does not have the same
  PATH variable configured.

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
Copy link
Owner

tmatilai commented Apr 3, 2014

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
Copy link
Author

ghost commented Apr 3, 2014

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
Copy link
Owner

tmatilai commented Apr 3, 2014

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 tmatilai closed this in 7893da2 Apr 3, 2014
@tmatilai
Copy link
Owner

tmatilai commented Apr 3, 2014

Merged, thanks!

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.

1 participant