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

• Toby Speight: "Simple" and "Upgrade" - two words that rarely go together without a negative somewhere nearby...
• Edward: I know you've said you're not interested in logging, but your view on that will change the first time something fails and you have to try to figure out why.

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

• ferada: That looks very nice, but could you perhaps link some documentation and explain what you did to arrive at the refactored version?
• tripleee: You mean like en.wikipedia.org/wiki/Don%27t_repeat_yourself?
• ferada: Sure, but also like en.wikipedia.org/wiki/Here_document and tldp.org/LDP/Bash-Beginners-Guide/html/sect_08_02.html. If you prefer printf, explain why. If the script is structured inside out, how so. The exit doesn't pass through the status code, does it - not saying that's bad, just different from the original?
• ben rudgers: The answer does not really provide a review of the question.
• tripleee: @VlastimilBurian The parts you changed I had actually changed on purpose in order to improve the script, but if losing the failed command's exit code is specifically what you want, more power to you. I'll add a couple of links to more documentation but I don't plan to work on other things. The logic in my answer should hopefully reveal how I mean "inside out" if you compare it to the original.