| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 89
Setup stuffs #702
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
Setup stuffs #702
Conversation
I didn't actually change anything, but the lock file was mutated nonetheless. Package management is hard, ya know?!
jonallured
force-pushed
the
setup-stuffs
branch
from
5dca7ee to
3321292
Compare
bin/setup
Outdated
| # Exit if any subcommand fails | ||
| set -e | ||
|
|
||
| STEPS=7 |
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.
argh! fix incoming!!
| if [ "$actual_node_version" != "$expected_node_version" ]; then | ||
| printf "${CLEAR_LINE}${RED} Your node version must match before setup can continue${NO_COLOR}\n" | ||
| printf " You should:\n" | ||
| printf " $ nvm install $expected_node_version\n" |
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.
Perhaps the script should first check if the user has nvm installed at all?
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.
I considered this, but I wasn't sure if that tool was universal enough to check for. Happy to add a check since that's what I'm using, but didn't want to get into a rvm vs rbenv vs chruby type of thing. What do you think?
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.
That’s a good point, but I think that in this case everybody (at Artsy) is using nvm. My main issue is that it then lines up with the example nvm install … command, otherwise it could be confusing to a newcomer if the suggested command doesn’t actually work.
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.
Sounds good to me - added with c2f0878.
|
I love the idea of not actually running the install commands but only offering help 👍 |
Rather than present install instructions for this one, I'm simply pointing you at the README for the project. This way metaphysics isn't the source of truth about how to install nvm. 😄
|
One more question, how about mentioning the script in the README? |
I like to advise people to actually read these types of scripts. Even if you don't know bash, you can get a feel for what it's going to do to your machine and you should have some idea of what a script is going to do to your machine before you run it just in general. :laugh:
|
Cool, I've updated the README with 5d2acd8 that includes a nudge to actually read the script. I think it's a good practice to read scripts before you run them, so whenever I can encourage people to do that, I try to sneak it in. 😆 |
|
Hah, I like it 👍 |
Labels
3 participants
You never get a second chance to setup a project for the first time, so I went slow and noted where there were some rough edges. One pattern I've been using on Partner Success projects is a setup script that does some checking for dependencies and automates the obvious stuff, so this commit brings that to MP.
I hope you like it! ❤️