Updated on May 2, 2023 in #linux

How Would You Improve This Shell Script?

The script handles calling another script with an optional position argument and 0 or more command line flags.

Recently I was tasked to write a wrapper script which calls another script with a few default flags plus whatever was passed into the wrapper script.

Here’s a few examples of how the wrapper script could be called. In reality this script has a better name, it’s only called “wrapper” for the sake of this post:

$ ./wrapper
$ ./wrapper Dockerfile
$ ./wrapper Dockerfile --bump-minor
$ ./wrapper Dockerfile --bump-minor -fail
$ ./wrapper --bump-minor
$ ./wrapper --bumpr-minor --fail

The call patterns can be described like this:

• No arguments
• Just a file path to a Dockerfile
• A file path and 1 flag
• A file path and more than 1 flag
• Just 1 flag
• More than 1 flag

The only rule is if a path to a Dockerfile isn’t set then it will default to Dockerfile. In other words, the file path is optional.

# Solution 1: Brute Force

Often times I’ll throw something together in the quickest way possible just to see it work. Sometimes that ends up being the final version because it turns out to be quite readable and reasonably performant.

However, if I find myself needing to write more comments than code to explain what’s happening or it just feels dirty I’ll look to refactor it as soon as possible.

The code below is an example of where it works but I don’t like it. It feels too convoluted and I think without the comments it’s hard to see what’s really going on:

#!/usr/bin/env bash

set -o errexit
set -o pipefail
set -o nounset

dockerfile_default_path="Dockerfile"
dockerfile_path="${1:-}"

if [[ -f "${dockerfile_path}" ]]; then
  # We have a Dockerfile path that's valid so the rest of our args must either
  # be empty or will be set as other args down below.
  shift
else
  # Is our first argument a flag such as: --fail or -f
  if [[ "${dockerfile_path}" =~ ^-.+$ ]]; then
    # We know a custom Dockerfile path isn't used, so let's use the default.
    dockerfile_path="${dockerfile_default_path}"
  else
    if [[ "${#}" -eq 0 ]]; then
      # No arguments were passed in so let's use the default.
      dockerfile_path="${dockerfile_default_path}"
    else
      # We're using a custom Dockerfile path so let's keep it and shift.
      shift
    fi
  fi
fi

other_args=("${@:-}")

# shellcheck disable=2068
echo "${dockerfile_path}" ${other_args[@]} --save --add-digest

I’m using echo as a placeholder for the real script that was being called, that’s not important for the sake of this post.

Here’s a bunch of usage outputs which demonstrates that the script works based on how the script can be called:

$ ./wrapper
Dockerfile --save --add-digest

$ ./wrapper Dockerfile
Dockerfile --save --add-digest

$ ./wrapper Dockerfile --bump-minor
Dockerfile --bump-minor --save --add-digest

$ ./wrapper Dockerfile --bump-minor --fail
Dockerfile --bump-minor --fail --save --add-digest

$ ./wrapper --bump-minor
Dockerfile --bump-minor --save --add-digest

$ ./wrapper --bump-minor --fail
Dockerfile --bump-minor --fail --save --add-digest

All of these outputs are the expected outputs. The default flags get added and there’s always a file path present as the first argument.

# Solution 2: Getting Loopy

A while back I made a post about parsing positional arguments and flags with a while loop.

Once I realized what I was doing in my original implementation, I figured I could replace my rat’s nest of if conditions with a while loop and case statement.

That looks like this:

#!/usr/bin/env bash

set -o errexit
set -o pipefail
set -o nounset

dockerfile_default_path="Dockerfile"
dockerfile_path=
other_args=()

while [[ "${#}" -gt 0 ]]; do
  case "${1}" in
    # Is it any type of flag such as: --fail or -f
    -*)
      other_args+=("${1}")

      shift
      ;;
    # This is the file path being set, although it's possible it's empty.
    *)
      dockerfile_path="${1}"

      shift
      ;;
  esac
done

# If the file path is empty then we'll want to fall back to using the default.
dockerfile_path="${dockerfile_path:-${dockerfile_default_path}}"

# shellcheck disable=2068
echo "${dockerfile_path}" ${other_args[@]} --save --add-digest

The above script has an added benefit of allowing the file path to exist in any order. For example you can call ./wrapper --fail Dockerfile and it will still produce the following output Dockerfile --fail --save --add-digest. The original script does not support that.

This is the real code I ended up shipping. In my opinion it’s much more readable. I didn’t commit it with this many comments. I added most of the comments for the sake of the post so it’s crystal clear on what’s going on.

# Solution 3: What Would You Do?

While the above solution isn’t bad, it’s not perfect.

If you have a custom Dockerfile named -Dockerfile or anything that starts with a - then it will fail as seen below:

$ ./wrapper -Dockerfile
Dockerfile -Dockerfile --save --add-digest

The expected output we want is -Dockerfile --save --add-digest.

The root problem is our strategy parses flags by checking to see if the argument starts with - followed by any character(s). There’s no way to differentiate a real flag like --fail or a custom file named --my-file.

One way to get around that would be to explicitly define each supported flag in the wrapper script which matches the script it calls. This way we never use -* but instead we have a separate case option for each flag.

That would work but I don’t like the idea of needing to keep the wrapper script and the script it calls in sync. There’s quite a few flags.

Realistically the above problem will never come up in the project I made it for but I do like to make things more robust if I can.

Perhaps I’m stuck looking at the trees instead of the forest as a whole. Is there a completely different strategy to be found? What would you do?

Also in case you’re wondering, I do realize the while loop solution doesn’t check to see if the file path exists with -f. That wasn’t an accidental regression. The script that gets called by the wrapper script will check to make sure the path exists. I only performed the check in the original wrapper script because it was how to differentiate a file path from a flag.

# Demo Video

Timestamps

• 0:43 – The use case for the wrapper script
• 3:37 – Going over the inputs and expected outputs
• 6:51 – Looking at the code for the first version
• 7:03 – I’m an exploratory type of developer
• 9:00 – v1: Is it a file path or flag?
• 11:12 – v1: Further testing if it’s a file or flag
• 13:05 – v1: Yep, that was confusing
• 13:37 – Making sure the second version works the same
• 14:55 – v2: High level overview
• 16:44 – v2: Handling the case with no arguments
• 17:55 – v2: Checking if it’s a file or flag in a loop
• 19:52 – Is v2 better or worse?
• 21:00 – One edge case with v2
• 22:29 – Is a much better v3 possible? Let us know

What would you do to make this script better? Let me know below.