1

I use Ubuntu 16.04 with Bash and I've created this extensionless, shebangless file /etc/cron.daily/cron_daily:

for dir in "$drt"/*/; do
if pushd "$dir"; then
wp plugin update --all --allow-root
wp core update --allow-root
wp language core update --allow-root
wp theme update --all --allow-root
popd
fi
done
"$rse"

The reason I do it is to be less dependent on crontab.

I'd like to ask if the naming of the file is safe, and if the overall syntax and variable expansions are okay.


The drt and rse variables were already exported after their file was sourced and are usable.

2

Looking at the other scripts in the same location on an Ubuntu machine I have access to, it is clear that these scripts should be proper shell scripts. They should be executable and have a #!-line pointing to the correct interpreter.

Since you expect that the variable drt is set to something, you should check that it is actually set and that it's set to something reasonable. For example, if $drt is supposed to be the pathname of an existing directory:

if [ ! -d "$drt" ]; then
    echo 'drt is unset or does not contain path to directory' >&2
    exit 1
fi

Likewise for rse:

if [ -z "$rse" ]; then
    echo 'rse is unset' >&2
    exit 1
fi

This would be done at the start of the script.

Dir checking

pushd and popd are primarily meant for interactive use (this may be argued about). It is additionally difficult to read and maintain a script that changes directories back and forth. Maybe not in this script, but in general.

Instead of changing working directory, doing something, and then changing back, you may use

( cd "some directory" && "some other command" )

The cd above is only affecting the ( ... ) subshell.

In this script, it may be enough with

if cd "$dir"; then
    command1
    command2
    # etc.
fi

assuming that $drt is an absolute path and that the simple command $rse is able to run correctly regardless of where it's started from (this leaves the the script in a modified working directory after the if-statement). See the other scripts in /etc/cron.daily/ to get view of how they do (the above suggestion is how the /etc/cron.daily/dpkg script does it, but it has no further commands after its if-statement).

The script would benefit from properly indenting the body of the for-loop and if-statement.

With the original example code, it can be done like this:

#!/bin/bash
for dir in "$drt"/*/; do
    if pushd "$dir"; then
        wp plugin update --all --allow-root
        wp core update --allow-root
        wp language core update --allow-root
        wp theme update --all --allow-root
        popd
    fi
done
"$rse"

Indentation can be done either with spaces or tabs (it's a matter of taste).

Additionally, I mistyped your variable names several times while writing this. Having descriptive variable names is beneficial for both yourself (in a few weeks time) and for anyone else who is trying to figure out what your script is supposed to be doing. There is no benefit of using short variable names in scripts as it leads to obscure code. I'm furthermore uneasy about the fact that these variables are set elsewhere as it implies a dependency on something that is not known and not documented in the script.

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.