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

Make the creation of a symlink an overridable default behaviour #514

Merged
merged 3 commits into from
Aug 30, 2014

Conversation

stelcheck
Copy link
Contributor

The creation of a symlink kind of caused quite a bit of headaches because we build systems where NVM is installed (and managed) by superusers and used by a whole bunch of normal (or system) users.

This PR would allow us, on such system, to at least disable a feature that although doesn't technically hurt us or have much of an effect, generates errors on screen at login time or run time, like so:

$ nvm use v0.123.456
rm: cannot remove `/opt/nvm/current': Permission denied
Now using node v0.123.456

(I know, I am using the future of Node :D)

@ljharb
Copy link
Member

ljharb commented Aug 28, 2014

Since nvm actually could be used on separate shells simultaneously, I think I'm actually going to disable this feature by default. Whether I leave it in or not at all is a separate question.

However, in the situation you describe, for npm install -g to work, users need permissions on $NVM_DIR, so the proper setup for this would include granting permissions to all users on /opt/nvm/current.

@ljharb
Copy link
Member

ljharb commented Aug 28, 2014

Could you add tests for this behavior?

(I'm glad your PR has the default be "on", and I'd prefer to think about changing the default in a future commit)

@stelcheck
Copy link
Contributor Author

In my use case, I do not want to allow anyone except superusers to install packages globally - but as I see it, it would be entirely possible to allow anyone to do package installs by giving the proper rights on the correct folder, so that pattern (superusers manage node versions, any user can npm install -g) would be doable too I think.

I left the current behaviour on by default because this is what the current behaviour is - so I thought it would be better not to break backward compatibility with the currently released version here.

As for tests, I am not sure how I could make it testable - unless I bail out explicitly when I fail to remove and re-create the symlink. Would that work for you? If so, I'll do that and write tests around nvm use.

@ljharb
Copy link
Member

ljharb commented Aug 29, 2014

@stelcheck See https://github.com/creationix/nvm/blob/master/test/fast/Running%20%22nvm%20use%20x%22%20should%20create%20and%20change%20the%20%22current%22%20symlink - a sibling test that asserts that when the env var is set, the symlink is not created, should cover it.

The test would need to assert/ensure that the symlink didn't exist prior to the test, as well.

@stelcheck
Copy link
Contributor Author

Just made the test suite for that. Please let me know if its good enough or if I need to fix something.

@ljharb
Copy link
Member

ljharb commented Aug 29, 2014

Nice, thanks! Interesting style. I'll review this and merge it tonight/tomorrow.

@ljharb ljharb self-assigned this Aug 29, 2014
ljharb added a commit that referenced this pull request Aug 30, 2014
Make the creation of a symlink an overridable default behaviour
@ljharb ljharb merged commit b61445c into nvm-sh:master Aug 30, 2014
ljharb added a commit that referenced this pull request Sep 3, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature requests I want a new feature in nvm!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants