Refactor output handling: unified logging & signal handling - #41
Merged
Conversation
Replace the outdated maintenance script with the newer, security-audited
variant (printf-based, unified log output) while keeping it a single,
standalone file suitable for the wget/cron install model.
- Inline the UI helpers (ui_init, log_ok/warn/err) instead of sourcing an
external scripts/lib/ui.sh; restores TTY colors and avoids a hard
dependency that would abort under 'set -euo pipefail'.
- Switch per-step output to unified, timestamped log_* lines; end-of-run
dashboard/summary format is unchanged.
- Harden signal handling: on_exit is re-entry guarded and runs exactly once
via EXIT; SIGINT/SIGTERM route through it so summary + tmp cleanup still
happen on Ctrl-C.
- Preserve the documented cron PATH string (no ${PATH} append).
- All flags, JSON output, steps, backup rotation and Pi-hole v6 commands
(pihole -v/-up/-g, pihole-FTL service, FTL query status codes) unchanged.
- README/README.de: note the per-step output and signal-handling changes.
Verified: git diff --check, bash -n, and CI-path dry-run all pass.
shellcheck/shfmt are not installed locally and were not run; CI (make check)
covers them.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MLBSE1qE3QvZYF8i9GjCz3
CI (make check) flagged formatting inherited from the source variant:
- space-after-redirect in cleanup_tmpdir ("2>/dev/null" -> "2> /dev/null")
- fix 6-space indent on the armhf "if dpkg" guard to 2 spaces
- remove now-unused CHECK/WARN/FAIL symbols (SC2034); run_step uses log_*
No behavior change. bash -n and the CI-path dry-run pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MLBSE1qE3QvZYF8i9GjCz3
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5310c866bd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| [[ $rc -ne 0 ]] && echo -e "${RED}Script ended with exit code $rc${NC}" | ||
| exit "$rc" | ||
| cleanup_tmpdir | ||
| [[ $rc -ne 0 ]] && printf '%sScript ended with exit code %s%s\n' "$RED" "$rc" "$NC" |
There was a problem hiding this comment.
On any clean run where rc is 0, this final [[ $rc -ne 0 ]] && ... expression returns 1 from on_exit; since the previous explicit exit "$rc" was removed and the script runs with set -e, Bash can make the overall script exit with status 1 even though every maintenance step succeeded. That will cause cron/systemd/CI wrappers to report successful maintenance runs as failures; restore an explicit exit "$rc" or otherwise make the trap return success for rc=0.
Useful? React with 👍 / 👎.
The refactor dropped the explicit `exit "$rc"` at the end of on_exit. On a clean run (rc=0) the trap's last evaluated command became `[[ 0 -ne 0 ]]`, which returns 1; under `set -e` that made the whole script exit 1 even when every step succeeded, so cron/systemd/CI wrappers would flag good runs as failures. Restore `exit "$rc"` as the final statement of on_exit. The re-entry guard already prevents double summary/cleanup, and calling exit inside the EXIT trap does not re-run it. INT/TERM still route through EXIT. Verified: buggy pattern exits 1 on a clean run, fixed pattern exits 0; full clean run of the script now returns 0. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MLBSE1qE3QvZYF8i9GjCz3
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR refactors the output and signal handling in
pihole_maintenance_pro.shto improve consistency, maintainability, and robustness. The main changes involve extracting UI helpers into self-contained functions, replacingecho -ewithprintf, and implementing proper signal handling for graceful cleanup.Key Changes
UI Helper Functions: Introduced
ui_init(),_ui_ts(), and_ui_format_line()functions that provide a unified, timestamped logging interface (log_ok(),log_warn(),log_err()). These are self-contained inline implementations (not external dependencies) suitable for a standalone script distributed via cron.Output Modernization: Replaced all
echo -ecalls withprintffor better portability and POSIX compliance. Color variables are now sourced from the UI helper system rather than defined separately.Signal Handling:
INTandTERMtrap handlers that exit with standard codes (130 for SIGINT, 143 for SIGTERM)ON_EXIT_DONEguard to prevent duplicate summary/cleanup executioncleanup_tmpdir()function for reusable cleanup logicLogging Format: Per-step output now uses a unified format
[HH:MM:SS] OK|WARN|ERR …instead of mixed✔ Erfolg/⚠ Warnungmessages, providing better consistency and machine-readability.Documentation: Updated both
README.mdandREADME.de.mdto document the output format changes in v5.3.2-refresh, clarifying that the dashboard and summary remain unchanged while per-step lines use the new timestamped format.Implementation Details
NO_COLORenvironment variable and TTY detection for conditional color outputstrip_ansi()function continues to work with the new output formatON_EXIT_DONEguard, even when multiple signals are receivedhttps://claude.ai/code/session_01MLBSE1qE3QvZYF8i9GjCz3