24
Wolle
2y

Things like this make me nervous ...

Comments
  • 4
    My eyes….
  • 13
    @darrenrahnemoon You should quote all your variables:

    mkdir -p "$ARTIFACT_DIR"

    Also give yourself an explicit delete:

    rm -rf "$ARTIFACT_DIR/*"

    I'd also suggest using a `set -e`
  • 3
    This just begs for disaster.
  • 3
    Why does it even require a "force" flag at all?
    That script was made by a stupid dingus
  • 10
    Quoting doesn't solve the problem at hand...

    Check that artifact_dir is a non empty variable would be the best.

    If [[ -z "${var}" ]]; then
    printf "Variable is empty\n" >&2
    exit 1
    fi

    Don't assume defaults, don't use misleading short argument syntax.

    rm --preserve-root --recursive --force "${var}/*"

    Will fail if rm is so old that preserve-root isn't existant or default... Much safer.

    Last but not least.

    Bash option erronexit / pipefail have _a_ lot of corner cases.

    Check return codes explicitly, don't rely on the behaviour of bash options.

    if ! rm ...; then
    printf "rm failed\n" >&2
    exit 1
    fi

    Isn't superfluous, rather the right way to handle things cause you then have a clear and safe exit handling.

    Don't rely on bash options. They have a lot of nitpicky fuckity hidden, some stuff even version dependent. It's not fun to debug :)
  • 2
    export ARTIFACT_DIR=/
  • 1
    @aaronswartz it's a bash script
  • 2
    additionally run all your BASH scripts through https://www.shellcheck.net/
  • 0
    @aaronswartz it is a bash/shell script to deploy some code in a CI/CD pipeline. Yes it is basically executing terminal commands.
  • 1
    At least you can use shopts to avoid those empty/undeclared variables …
  • 1
    What is that?
Add Comment