-
Notifications
You must be signed in to change notification settings - Fork 329
Don't update the user's shell config files if VOLTA_HOME and PATH already contains what we need. #990
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
Conversation
…eady contain what we need.
charlespierce
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dsully! Looks mostly good, just a couple of small tweaks to make sure things work as expected and are easier to debug later on.
Also, would you mind creating a follow-on issue to add a --force or similar flag? It's not necessary for this PR, but would be good to track the need for something like that in case we want to have the ability to force it through even given the environment appears to be properly set up.
|
Looks like the changes need to have |
Co-authored-by: Charles Pierce <charlespierce@users.noreply.github.com>
|
Done & done! |
charlespierce
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more small change (that may need cargo fmt run again afterwards).
charlespierce
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks! The CI failures are unrelated and fixed in #993, once that's merged I'll merge this and then confirm that everything passes post-merge.
This will prevent changes when these settings are defined in a different, but compatible way to what volta does.