5
\$\begingroup\$

I'm quite a novice in shell scripting so far, but i'm learning and getting quite comfortable in writing shell scripts, but i have a lot to learn.

The snippet at the bottom of the page creates backups of a certain users databases.

The script checks first if there is more than 7 compressed .gz files in a folder.

Fetches oldest gz archive and removes it.

Checks for latest .sql backup and compresses it with gzip

Creates a new .sql dump

The script is run once a day

How would you write this differently?

#!/bin/bash

BACKUPPATH=/var/mysql_backup/
mkdir -p $BACKUPPATH
if [ $(ls -l $BACKUPPATH*.gz 2>/dev/null | wc -l) -gt 7 ]; then
        OLD=$(find $BACKUPPATH -type f -name '*.gz' -printf '%T+ %p\n' | sort | head -n 1 | awk '{print $2}')
        rm $OLD # Removing oldest compressed archive
fi

if [ $(ls -l $BACKUPPATH*.sql 2>/dev/null | wc -l) -eq 1 ]; then
        LATESTLOG=$(find $BACKUPPATH -type f -name '*.sql' -printf '%p\n' | head -n 1)
        gzip $LATESTLOG
fi

/usr/bin/mysqldump -u db_user -p'password' --single-transaction  --all-databases > $BACKUPPATH$(date "+%Y.%m.%d-%H.%M.%S")_dump.sql
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review. While we can help you clean up your code, asking for how to fix what your code does ("the logger doesn't work) is off-topic for Code Review. I recommend that you either fix that part of your code first, or that you remove your logger in your code and the part of your question where you mention the small error with your script. Code Review is all about making your code do the same thing in a better way. \$\endgroup\$ – Simon Forsberg Feb 6 '16 at 18:55
  • \$\begingroup\$ Got that Simon, wasn't really a part of the question to fix it , but i'll remove it. \$\endgroup\$ – JazzCat Feb 6 '16 at 18:56
6
\$\begingroup\$

I'd do this:

#!/bin/bash

backuppath=/var/mysql_backup
mkdir -p "$backuppath"

shopt -s nullglob

# Remove old compressed archive
backups=( "$backuppath"/*.gz )
if [[ ${#backups[@]} -gt 0 ]]; then
    stat -c "%Y %n" "${backups[@]}" |
      sort -rn |
      sed -e '1,7d' -e 's/^[0-9]\+ //' |
      xargs -r rm
fi

# Compress all the logs
logs=( "$backuppath"/*.sql )
if [[ ${#logs[@]} -gt 0 ]]; then
    gzip "${logs[@]}"
fi

/usr/bin/mysqldump -u db_user -p'password' --single-transaction --all-databases \
    > "$backuppath/"$(date "+%Y.%m.%d-%H.%M.%S")_dump.sql

Notes:

  • don't use ALLCAPSVARS -- leave those for the shell. I've seen people accidentally do PATH=... and then wonder why their script is broken.
  • don't parse ls -- I'm using arrays to hold the results of the glob expansion, and checking how many elements are in the array.
  • always quote variables, except when you know exactly why not. See https://unix.stackexchange.com/q/171346/4667
  • I'm assuming you want to compress any log.sql file, not just the first found
  • I'm assuming you want to keep 7 compressed logs and remove any older ones.
  • I'm assuming you have the GNU coreutils (xargs -r, those stat options)
\$\endgroup\$
  • \$\begingroup\$ just one syntax error at backups=( "$backuppath"/*.gz" ), remove the last quote \$\endgroup\$ – JazzCat Feb 7 '16 at 12:34
3
\$\begingroup\$

Assumptions, Asssumptions, Assumptions

Under the assumption there is only exactly one ".sql"-File in the $BACKUPPATH you can rewrite your LATESTLOG into:

LATESTLOG = $(ls -A $BACKUPPATH | grep -o .*\.sql)

sidenote: I'd personally not assume there is only exactly one such file present and instead just gzip all the ".sql"-Files currently in the directory. Consider:

ls -A $BACKUPPATH | grep -o .*\.sql | xargs gzip {}

Then it becomes even more obvious what's wrong with your assumption in the preceding block...
Just because there's more than 7 ".gz" files, that doesn't mean there's exactly 8 (as you seem to assume). Moreover, it isn't even guaranteed that these ".gz"-files will follow the naming convention to include the date.

Because somewhen someone will come and do a manual backup. Just in case. And then this thing will break. But until then it's a well-crafted piece of code. Well done :)

\$\endgroup\$
  • \$\begingroup\$ Don't know about well-crafted but thanks, and thanks for the feedback! Much appreciated. \$\endgroup\$ – JazzCat Feb 7 '16 at 11:02

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.