Problem
I have written the following Bash script which includes an Nginx WordPress bootstrapper: A program establish WordPress apps fast on a Debian-Nginx environment.
It utilizes CertBot for HTTPS, SSHguard for protection from BFA (see Nginx conf), WP-CLI for creating a WP app dir and conf file fast, MySQL CLI to create a dbstack and a local function named rse to restart the server and giving proper permissions to the app dir.
#!/bin/bash
domain="$1" && test -z "$domain" && exit 2
environment() {
read -sp "Please enter the app DB root password: " dbrootp_1 && echo
read -sp "Please enter the app DB root password again:" dbrootp_2 && echo
if [ "$dbrootp_1" != "$dbrootp_2" ]; then echo "Values unmatched. Please try again." && exit 2 fi
read -sp "Please enter the app DB user password: " dbuserp_1 && echo
read -sp "Please enter the app DB user password again:" dbuserp_2 && echo
if [ "$dbuserp_1" != "$dbuserp_2" ]; then echo "Values unmatched. Please try again." && exit 2 fi
}
environment
wordpress() {
rm -rf "$drt"/"$domain"/ 2>/dev/null
wp core download --path="$drt"/"$domain"/ --allow-root
wp config create --path="$drt"/"$domain"/ --dbname="$domain" --dbuser="$domain" --dbpass="$dbuserp" --dbhost="localhost" --allow-root
}
wordpress
nginx() {
rm "$s_a/$domain.conf" 2>/dev/null
rm "$s_e/$domain.conf" 2>/dev/null
cat <<-EOF > "$s_a/$domain.conf"
server {
root ${drt}/${domain}/;
server_name ${domain} www.${domain};
location ~* .(jpg|jpeg|png|gif|ico|css|js|ttf|woff|pdf)$ {expires 365d;}
location "/wp-login.php" {access_log "/var/log/httpd/wordpress-logins.log";}
}
EOF
ln -sf "$s_a"/"$domain".conf "$s_e"
rse
}
nginx
certbot() {
certbot --nginx -d "$domain" -d www."$domain"
rse
}
certbot
database() {
cat <<-EOF | mysql -u root -p"$dbrootp_1"
DROP USER IF EXISTS "$domain"@"localhost";
DROP database IF EXISTS "$domain";
CREATE USER "$domain"@"localhost" IDENTIFIED BY "$dbuserp_1";
CREATE DATABASE "$domain";
GRANT ALL PRIVILEGES ON "$domain".* TO "$domain"@"localhost";
EOF
}
database
finalize() {
echo "Change http to http2 in your Nginx app conf and run rse"
}
finalize
Note
Before executing the script I declared in the end of $HOME/.bashrc:
drt="/var/www/html"
s_a="etc/nginx/sites-available"
s_e="etc/nginx/sites-enabled"
Solution
My answer is going to focus on your use of shell functions and global variables. There aren’t any actual syntax errors in your script AFAICT, but your misuse of functions in this script is dangerously error-prone.
There are two main reasons for using a function in bash (or any language, really).
The first is to parameterise a frequently performed operation – e.g. by passing arguments to it rather than hard-coding values (or relying on side-effects). Optionally it should return a result to the caller (in shell, by printing something to stdout to be captured with command substitution. Or sometimes by changing a variable in the environment – but see below about that)
You’re not doing that.
Worse, you’re making your functions rely on global variables. This makes your functions (and your scripts) dependent on side-effects of previous variable definitions – which may still hold old values from previous runs (and you forgot to update them for the current run) or, worse, may be empty because they haven’t been initialised yet in the current shell. This will inevitably lead to catastrophic data-loss one day when you forget to set a value for “$drt” and/or “$domain” and/or your many other global variables.
Better to pass values as parameters when and as needed. Don’t trust that a crucial variable happened to have been set some time in the past – i.e. be explicit, not implicit.
As a rule, functions should be self-contained and not rely on variables that may or may not be defined outside of their scope. They should also define variables as being local to them so that they explicitly don’t use (or worse, change) a global variable that happens to have the same name as the function’s variable.
Anyway, an example of how your wordpress function could be written:
wordpress() {
local drt="$1"
local domain="$2"
local dbpass="$3"
# should do more checking here, this is the bare minimum.
# it avoids running `rm -rf /` if both $drt and $domain
# are empty, and running `rm -rf "$drt/"` if $domain is empty.
[ -z "$drt" ] && error 1 '$drt is empty. aborting!'
[ -z "$domain" ] && error 1 '$domain is empty. aborting!'
[ -z "$dbpass" ] && error 1 '$dbpass is empty. aborting!'
# should check exit status of each command run here and take
# appropriate action on any failures. Figure out what you want
# to happen if any of the following fail and implement it.
rm -rf "$drt"/"$domain"/ 2>/dev/null
wp core download --path="$drt"/"$domain"/ --allow-root
wp config create --path="$drt"/"$domain"/ --dbname="$domain"
--dbuser="$domain" --dbpass="$dbpass" --dbhost=localhost
--allow-root
}
You’d call it as wordpress "$drt" "$domain" "$dbpass". It uses a function called error:
error() {
local ec msg
ec="$1" ; shift
msg="$*"
[ -n "$msg" ] && echo "$msg" >&2
# don't exit if $ec==0 - just return after printing a warning to stderr
[ "$ec" != 0 ] && exit "$ec"
}
The second reason is to make the structure of the script easier to follow. For example, instead of having a lot of shell code for each pattern matched in a case statement (which makes it difficult to see at a glance what the cases are and what each one does), write a function for each case and call it – with or without arguments.
You’re not doing that either – you’re using each function as soon as you define it. That’s both weird and pointless….especially so since your functions don’t take arguments.
A more usual way of defining and using functions would be something like:
function1 () { ... ; }
function2 () { ... ; }
function3 () { ... ; }
function4 () { ... ; }
main () {
function1
function2
function3
function4
}
main
Note: using amain function allows you to define and call functions in any order, and allows you to define your functions (other than main() itself) at the end of the script so that the main code comes first. Otherwise you can get errors in situations where, e.g., your main non-function script code calls function2 before it is defined.
as noted above, each function call can, or should, have explicit arguments passing in the values they will use.
BTW, for the case statement mentioned above:
case "$var" in
1) function1 ;;
2) function2 ;;
3) function3 ;;
4) function4 ;;
*) error 1 'Unknown case' ;;
esac
That case statement would be a lot harder to read if each case had 5 or 10 or more lines of code.
@cas has already pointed out the most critical issues.
I add a few minor points on top of that.
Syntax errors
A semicolon is missing before the fi on this line and another similar line:
if [ "$dbrootp_1" != "$dbrootp_2" ]; then echo "Values unmatched. Please try again." && exit 2 fi
Usability
The script is not very user-friendly.
For example,
when the entered passwords don’t match,
it prints “Please try again”, and exits.
I would expect “try again” to mean that I can enter the passwords one more time, and not that I have to rerun the script one more time.
If I accidentally mistyped the 2nd password for the DB user,
and I’m forced to re-enter the DB root passwords too,
I’ll be very unhappy with the script.
A also have a pickle with this prompt:
read -sp "Please enter the app DB root password: " dbrootp_1 && echo
As a user, I find the many blanks after the : weird. I would find one blank natural.
Don’t repeat yourself
The input of DB root and DB user passwords are very similar,
and a good candidate to parameterize and extract to a reusable function.
Something like this (thanks @cas for the > /dev/tty tip):
readpw() {
local pw1 pw2 label=$1
while :; do
read -sp "Please enter the $label password: " pw1 && echo
read -sp "Please enter the $label password again: " pw2 && echo
[ "$pw1" = "$pw2" ] && break
echo "The values don't match. Please try again."
done > /dev/tty
echo "$pw1"
}
dbuserpw=$(readpw "app DB user")
dbrootpw=$(readpw "app DB root")
Note that the value of $(...) is everything that was written on stdout, minus trailing newlines. The > /dev/tty is to prevent output on stdout that we don’t want to capture by $(...) (thanks @cas!).