Simple Linux upgrade script in Bash revision 2

Question

As I will be deploying this script on multiple machines with the very same system Linux Mint 18 with rather same configuration, I would like to be semi-sure I won't screw things up much.

This little script will run every day from the root's crontab and will be logged into syslog or my own log AFTER this manual script passes some quality checks. Logging is not the question now. Automation is also not the question yet. In the meantime it is being executed as normal user and elevated with sudo internally. No help message needed. And no docs needed either. I ran it through Shellcheck.net. I have also ruled out the use of subjectively nicer apt, because it is not meant to be used in scripts.

Conditions still are:

1. Code readability
2. Output readability
3. Colored headings
4. Attempt to correct things
5. Clean-up after update

My current idea is a little more advanced than the first one, having implemented exit code checking was a great idea:

#!/bin/bash


RED="\033[1;31m"
GREEN="\033[1;32m"
YELLOW="\033[1;33m"
NOCOLOR="\033[0m"


function show_success {

  echo -e "${GREEN}Success.${NOCOLOR}\n"

}


function show_error_and_exit {

  echo -e "${RED}An error occured.${NOCOLOR}\n"

  exit "$1"

}

function error_handler {

  if [[ $1 -ne 0 ]];
  then

    show_error_and_exit "$2"

  else

    show_success

  fi

}


echo -e "\n${GREEN}Step 1: configure packages${NOCOLOR}"
echo -e "${YELLOW}dpkg --configure -a${NOCOLOR}"

sudo dpkg --configure -a

error_handler $?  1


echo -e "${GREEN}Step 2: fix broken dependencies${NOCOLOR}"
echo -e "${YELLOW}apt-get install --fix-broken${NOCOLOR}"

sudo apt-get install --fix-broken

error_handler $?  2


echo -e "${GREEN}Step 3: update cache${NOCOLOR}"
echo -e "${YELLOW}apt-get update${NOCOLOR}"

sudo apt-get update

error_handler $?  3


echo -e "${GREEN}Step 4: upgrade packages${NOCOLOR}"
echo -e "${YELLOW}apt-get upgrade${NOCOLOR}"

sudo apt-get upgrade

error_handler $?  4


echo -e "${GREEN}Step 5: upgrade distribution${NOCOLOR}"
echo -e "${YELLOW}apt-get dist-upgrade${NOCOLOR}"

sudo apt-get dist-upgrade

error_handler $?  5


echo -e "${GREEN}Step 6: remove unused packages${NOCOLOR}"
echo -e "${YELLOW}apt-get --purge autoremove${NOCOLOR}"

sudo apt-get --purge autoremove

error_handler $?  6


echo -e "${GREEN}Step 7: clean up${NOCOLOR}"
echo -e "${YELLOW}apt-get autoclean${NOCOLOR}"

sudo apt-get autoclean

error_handler $?  7

Answer 1

Don't use non-portable terminal escape codes

The tput program exists for exactly this purpose:

red=$(tput setaf 1)
green=$(tput setaf 2)
yellow=$(tput setaf 3)
nocolor=$(tput sgr0)

(I've downcased your variable names, to avoid surprising conflicts with environment variables)

Inline the single use functions

show_success and show_error_and_exit are laudable, and do help readability, but they are only used once each, and may be clearer if written directly into error_handler. Either way, their output really ought to go to the error stream: >&2.

Reduce duplication

Look at this pattern:

echo -e "\n${GREEN}Step 1: configure packages${NOCOLOR}"
echo -e "${YELLOW}dpkg --configure -a${NOCOLOR}"

sudo dpkg --configure -a

error_handler $?  1

This structure is repeated throughout the program, but it can be refactored. Let's write the variable parts using variables:

echo "${green}Step ${step}: ${description}${nocolor}"
echo "${yellow}${command}${nocolor}"

$command

error_handler $?  $step

There's three things we need: a step number, description, and the command to run. The command needs to be an array; that's easy to arrange by passing it as all the arguments after description. We can also inline the error_handler:

do_action()
{
    step=$1; shift
    description=$1; shift
    printf "${green}Step %s: %s\n" "$step" "$description"
    printf "${yellow}"
    printf '%q ' "$@"
    printf "${nocolor}\n"

    if "$@"
    then
        printf "${green}Step %s: SUCCESS${nocolor}\n" "$step"
    else
        err=$?
        printf "${red}Step %s: FAILED${nocolor}\n" "$step" >&2
        exit $err
    fi
}

This function is then used as:

i=0
do_action $((++i)) "configure packages" dpkg --configure -a
do_action $((++i)) "fix broken dependencies" apt-get install --fix-broken
do_action $((++i)) "update cache" apt-get update
do_action $((++i)) "upgrade packages" apt-get upgrade
do_action $((++i)) "upgrade distribution" apt-get dist-upgrade
do_action $((++i)) "remove unused packages" apt-get --purge autoremove
do_action $((++i)) "clean up" apt-get autoclean

You might choose to make the step number a global variable and increment it inside the function; I haven't done that, because global variables tend to proliferate and make the program hard to reason about.

Don't sudo in a script

Many calls to sudo is a sign that the entire script needs superuser capabilities, so it's better if the user runs the script as root. You could automate this (though I would personally just error out):

test $EUID = 0 || exec sudo $0 "$@"

---

Full program

#!/bin/bash
set -e -u

red=$(tput setaf 1)
green=$(tput setaf 2)
yellow=$(tput setaf 3)
nocolor=$(tput sgr0)

do_action()
{
    step=$1; shift
    description=$1; shift
    printf "${green}Step %s: %s\n" "$step" "$description"
    printf "${yellow}"
    printf '%q ' "$@"
    printf "${nocolor}\n"

    if "$@"
    then
        printf "${green}Step %s: SUCCESS${nocolor}\n" "$step"
    else
        err=$?
        printf "${red}Step %s: FAILED${nocolor}\n" "$step" >&2
        exit $err
    fi
}

i=0
do_action $((++i)) "configure packages" dpkg --configure -a
do_action $((++i)) "fix broken dependencies" apt-get install --fix-broken
do_action $((++i)) "update cache" apt-get update
do_action $((++i)) "upgrade packages" apt-get upgrade
do_action $((++i)) "upgrade distribution" apt-get dist-upgrade
do_action $((++i)) "remove unused packages" apt-get --purge autoremove
do_action $((++i)) "clean up" apt-get autoclean

Answer 2

Your script is structured sort of inside out.

Also, as a minor stylistic remark, I'd prefer printf over echo -e.

#!/bin/bash

set -e

run () {
    local task=$1
    local desc=$2
    shift 2

    #local rc

    # More portable would be to use tput instead of literal escape codes
    # Avoid uppercase for non-system variables
    local red="\033[1;31m"
    local green="\033[1;32m"
    local yellow="\033[1;33m"
    local nocolor="\033[0m"

    printf "%sStep %s: %s.%s\n" "$green" "$task" "$desc" "$nocolor"
    printf "%ss%s\n" "$yellow" "$*" "$nocolor"

    if sudo "$@"; then
        printf "%sSuccess.%s\n" "$green" "$nocolor"
    else
        # The fix to capture the failed command's exit code
        # was removed by the OP in an edit of the code
        # but I'm recording it here for posterity.
        #rc=$?
        #printf "%sFailure: %s%s\n" "$red" "$task" "$nocolor"
        #return $rc
        printf "%sFailure: %s%s\n" "$red" "$task" "$nocolor"
        return $task
    fi
}

while IFS=: read exit cmd doco; do
    run $exit "$doco" $cmd || exit
done <<____HERE
    1:dpkg --configure -a          :configure packages
    2:apt-get install --fix-broken :fix broken dependencies
    3:apt-get update               :update cache
    4:apt-get upgrade              :upgrade packages
    5:apt-get dist-upgrade         :upgrade distribution
    6:apt-get --purge autoremove   :remove unused packages
    7:apt-get autoclean            :clean up
____HERE

We use read with a custom IFS to read the sequence numbers, commands, and command descriptions from a here document, which is basically a wrapper around the "real script". There would probably be elegant ways to avoid wrapping it in a here document if you would forego the (IMHO slightly obnoxious) color output, perhaps by refactoring that to a separate script.

Documentation for using lowercase private variable names is available e.g. in https://unix.stackexchange.com/questions/42847/are-there-naming-conventions-for-variables-in-shell-scripts