Added install script & documentation for installing this repository. #3
Conversation
… including Bash & ZSH shells. Needs testing.
| @@ -0,0 +1,74 @@ | |||
| #!/usr/bin/env bash | |||
| set -eu | |||
|
|
|||
There was a problem hiding this comment.
Even though this script does not pipe anything, I would recommend adding
set -o pipefail
|
|
||
| # Ensure PHP is installed | ||
| if ! command -v php > /dev/null 2>&1; then | ||
| echo "PHP is not found, installation aborted!" |
There was a problem hiding this comment.
If this is an error it should be redirected to STDERR
| # Ensure PHP is installed | ||
| if ! command -v php > /dev/null 2>&1; then | ||
| echo "PHP is not found, installation aborted!" | ||
| exit 1 |
There was a problem hiding this comment.
As there is more than one exit scenario I would prefer to see a different exit code than 1
| exit 1 | ||
| fi | ||
|
|
||
| # Ensure Composer is installed/updated |
There was a problem hiding this comment.
Instead of a comment it might be better to have a function named ensureComposerInstalled
There was a problem hiding this comment.
I tried to keep it simple, top down. But I agree as well... 👍
|
|
||
| # Ensure Composer is installed/updated | ||
| if ! command -v composer > /dev/null 2>&1; then | ||
| echo "*** Installing Composer ***" |
There was a problem hiding this comment.
Instead of having a personal standard for messages I would prefer to see a convention being used. (For instance the Heroku output formats).
There was a problem hiding this comment.
Yeah... Since we don't have a convention for it yet... Nevertheless, nice link, will "borrow" some. 😉
| @@ -0,0 +1,74 @@ | |||
| #!/usr/bin/env bash | |||
| set -eu | |||
There was a problem hiding this comment.
I would prefer seeing the more verbose set -o errexit as that is a bit more understandable than -e (and set -o nounset rather than -u). It would also be helpful to add a comment after each flag for developers less familiar with bash.
| php -r "copy('https://getcomposer.org/installer', 'composer-setup.php');" | ||
|
|
||
| if [ \ | ||
| "$(curl -S https://composer.github.io/installer.sig)" = \ |
| "$(php -r \"echo hash_file\('SHA384', 'composer-setup.php'\);\")" \ | ||
| ]; then | ||
| php composer-setup.php --quiet --filename=composer | ||
| sudo mv composer /usr/local/bin/composer |
There was a problem hiding this comment.
Not entirly sure about the sudo, then again, chowning would probably also require sudo, so... meh.
There was a problem hiding this comment.
It has been decided:
@8BallTweets Should we use `sudo mv`?
— 422 - Unprocessable (@Potherca) October 25, 2016
@potherca - Yes
— Ask the Magic 8 Ball (@8BallTweets) October 25, 2016
| if [[ ! -f "$HOME/.profile" ]]; then | ||
| touch "$HOME/.profile" | ||
| fi | ||
| if grep -q -F "export COMPOSER_HOME=$HOME/.composer" "$HOME/.profile"; then |
There was a problem hiding this comment.
These two ifs belong in an else. If we've just created the file it does not make sense to grep it.
|
|
||
| # Source the .profile to pick up changes | ||
| # shellcheck source=/dev/null | ||
| source "$HOME/.profile" |
There was a problem hiding this comment.
This could have unintended side-effects, depending on what the user has (been told by other programs to) put in their .profile
There was a problem hiding this comment.
It "should not", but I like the don't assume approach. Replacing it with a success message at the end including an instruction would make more sense I guess. 👍
There was a problem hiding this comment.
You could also just run the export COMPOSER_HOME=$HOME/.composer and/or export PATH=$PATH:\$COMPOSER_HOME/vendor/bin in the relevant if to have the same effect.
There was a problem hiding this comment.
Regarding your last comment... actually, you can't...
|
I would strongly suggest reading the bashstyle for some general pointers (including "all code should go in a function"). |
|
@Potherca Updated PR. |
…ot recommended, forbidding it could give problems in CI builds (e.g. TravisCI & Bitbucket Pipelines).
|
👍 🚢 |
| Using Composer (preferred method): | ||
|
|
||
| ```bash | ||
| composer require --dev "dealerdirect/qa-tools:*" |
There was a problem hiding this comment.
The star is not needed composer require --dev "dealerdirect/qa-tools would just grab the latest version. (Taking constraints into account).
| "…": "*" | ||
| }, | ||
| "require-dev": { | ||
| "dealerdirect/qa-tools": "*" |
There was a problem hiding this comment.
We might want to add a version number rather than "no constraint"
…tructions, in case of require-dev
No description provided.