2
\$\begingroup\$

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"
\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$

@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!).

\$\endgroup\$
  • \$\begingroup\$ I prefer to return values from functions like dbuserpw=$(readpw "app DB user") rather than through some $REPLY variable. \$\endgroup\$ – 200_success Mar 7 '18 at 19:36
  • 3
    \$\begingroup\$ You can redirect any output you don't want to be returned by the function to /dev/tty. read -p seems to do this already (or maybe dups stdout). e.g. ask() { local r1 r2; read -p "$1" r1; echo "And again" > /dev/tty; read -p "$1" r2; printf '%s' "$r1 $r2" ; }; r=$(ask 'Enter something: '); echo "r=$r" \$\endgroup\$ – cas Mar 8 '18 at 1:21
  • \$\begingroup\$ @cas cool, didn't know, works like a charm, thanks! \$\endgroup\$ – janos Mar 8 '18 at 5:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy