From 82528a14028ea7c17d63d5f7aa931564c459111e Mon Sep 17 00:00:00 2001 From: John Coffey Date: Sat, 22 Aug 2026 22:15:21 -0700 Subject: [PATCH] Fix the two ShellCheck warnings and hold CI at that level The workflow went in at severity: error on the assumption that a never-linted repository would have a backlog worth grandfathering. It did not -- error found nothing, and warning found exactly two things, so the cautious setting was protecting against a problem that was not there. zimbra_backup.sh: SC2024, sudo does not affect redirects. The `> "$BACKUP_FILE"` runs as root rather than as the sudo'd zimbra user, so backups landed root-owned inside a directory the script deliberately chowns to zimbra:zimbra. Kept the redirect -- root can always write there, and piping into `tee` would put tee's status in $? and hide a zmmailbox failure -- and handed ownership over explicitly afterwards. The suppression is narrow and states why. disk_cleanup.sh: SC2034, total_freed was assigned and never read. CI now holds at warning with a clean tree, so anything that trips it is new rather than inherited. --- .github/workflows/shellcheck.yml | 11 ++++++----- disk_cleanup.sh | 2 -- zimbra_backup.sh | 7 +++++++ 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/.github/workflows/shellcheck.yml b/.github/workflows/shellcheck.yml index bd422e7..855c32b 100644 --- a/.github/workflows/shellcheck.yml +++ b/.github/workflows/shellcheck.yml @@ -23,11 +23,12 @@ jobs: - name: Run ShellCheck uses: ludeeus/action-shellcheck@master with: - # Findings are reported but do not fail the build yet: the - # existing scripts have not been through a lint pass, so - # failing on day one would block every PR on pre-existing - # issues. Tighten to "error" -> "warning" -> remove once the - # backlog is cleared; see the audit in the README. + # warning, not error: the first run at this level surfaced + # exactly two findings and both were fixed, so there is no + # pre-existing backlog to grandfather in. Anything new that + # trips it is genuinely new. Dropping to the default (info) + # would also pull in style suggestions across every script -- + # worth doing, but as its own pass. severity: warning check_together: 'yes' format: gcc diff --git a/disk_cleanup.sh b/disk_cleanup.sh index 6d339a4..f3364d4 100644 --- a/disk_cleanup.sh +++ b/disk_cleanup.sh @@ -208,8 +208,6 @@ clean_package_caches() { # Clean temporary files clean_temporary_files() { - local total_freed=0 - for dir in "${DIRS_TO_CLEAN[@]}"; do if [ ! -d "$dir" ]; then log "Warning: Directory $dir does not exist, skipping..." diff --git a/zimbra_backup.sh b/zimbra_backup.sh index ad88ea2..da98b97 100644 --- a/zimbra_backup.sh +++ b/zimbra_backup.sh @@ -57,10 +57,17 @@ echo "📦 Starting backup..." # $EMAIL arrives as a positional argument instead. Interpolating it (as # this line previously did) let any shell metacharacter in the address # run commands as the zimbra user, which owns the whole mail store. +# The redirect is performed by this shell, which is root -- not by the +# sudo'd zimbra process. That is intentional (root can always write +# here, and a pipe into `tee` would put tee's exit status in $? and mask +# a zmmailbox failure), but it means the file lands root-owned inside a +# directory chowned to zimbra, so ownership is handed over below. +# shellcheck disable=SC2024 sudo -u zimbra bash -c '/opt/zimbra/bin/zmmailbox -z -m "$1" getRestURL "//?fmt=tgz"' _ "$EMAIL" > "$BACKUP_FILE" # Verify success if [ $? -eq 0 ]; then + chown zimbra:zimbra "$BACKUP_FILE" 2>/dev/null || true echo "✅ Backup completed: $BACKUP_FILE" else echo "❌ Backup failed. Check if the user exists or zmmailbox is working."