Feat/adding optional journal logging#90
Conversation
GingerGraham
commented
Mar 11, 2026
- Adding the ability to log a single line to the journal without needing to include logic to enable and then disable journal logging fully
- Updated tests to account for this option
There was a problem hiding this comment.
Pull request overview
This PR adds a new public API function to force a single log entry to be written to the system journal (even when journal logging is disabled), along with dedicated tests and documentation updates to reflect the expanded journal/test coverage.
Changes:
- Add
log_to_journal LEVEL MESSAGEand extend internal journal dispatch to support forced, per-call journal writes. - Introduce a new
tests/test_journal_logging.shsuite and migrate existing journal init/runtime-toggle tests into it. - Update docs to include the new API entry and refresh testing documentation to match current suite discovery/coverage descriptions.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
logging.sh |
Adds log_to_journal, introduces a logger-discovery flag, and extends _log_message → _write_to_journal to support forced journal writes. |
tests/test_journal_logging.sh |
New focused suite covering forced journal writes, level validation/aliases, suppression behavior, and logger-unavailable warnings. |
tests/test_initialization.sh |
Removes journal-related tests that were migrated into the new journal suite. |
docs/testing.md |
Updates the testing overview and suite list to reflect current auto-discovery and broader coverage. |
docs/api-reference.md |
Documents the new log_to_journal public API and its behavior/return codes. |
You can also share your feedback on Copilot code review. Take the survey.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Graham Watts <34165628+GingerGraham@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Graham Watts <34165628+GingerGraham@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Graham Watts <34165628+GingerGraham@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Graham Watts <34165628+GingerGraham@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Graham Watts <34165628+GingerGraham@users.noreply.github.com>
|
@GingerGraham I've opened a new pull request, #91, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
You can also share your feedback on Copilot code review. Take the survey.
…_to_journal_level_aliases
Replace single-quoted 'alias_test_${alias}' with escaped double quotes
\"alias_test_${alias}\" to make the variable expansion intent explicit and
unambiguous inside the bash -c double-quoted string.
Addresses review comment on PR #90.
Co-authored-by: GingerGraham <34165628+GingerGraham@users.noreply.github.com>
…l_level_aliases (#91) * Initial plan * fix(tests): use double quotes for alias message expansion in test_log_to_journal_level_aliases Replace single-quoted 'alias_test_${alias}' with escaped double quotes \"alias_test_${alias}\" to make the variable expansion intent explicit and unambiguous inside the bash -c double-quoted string. Addresses review comment on PR #90. Co-authored-by: GingerGraham <34165628+GingerGraham@users.noreply.github.com> * fix(logging): change warning message to uppercase in log_to_journal function --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: GingerGraham <34165628+GingerGraham@users.noreply.github.com>
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@GingerGraham I've opened a new pull request, #92, to work on those changes. Once the pull request is ready, I'll request review from you. |
) * Initial plan * fix(logging): check log level before logger availability in log_to_journal Messages below CURRENT_LOG_LEVEL are now silently suppressed (return 0) before any logger discovery or warning, matching the behaviour of all other log functions. Also updates docs and adds a dedicated test that asserts no warning is emitted for below-level calls even when the logger binary is unavailable. Fixes unresolved comments from pullrequestreview-3931493070 Co-authored-by: GingerGraham <34165628+GingerGraham@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: GingerGraham <34165628+GingerGraham@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
You can also share your feedback on Copilot code review. Take the survey.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Graham Watts <34165628+GingerGraham@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Graham Watts <34165628+GingerGraham@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
You can also share your feedback on Copilot code review. Take the survey.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Graham Watts <34165628+GingerGraham@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
You can also share your feedback on Copilot code review. Take the survey.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Graham Watts <34165628+GingerGraham@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Graham Watts <34165628+GingerGraham@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
You can also share your feedback on Copilot code review. Take the survey.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Graham Watts <34165628+GingerGraham@users.noreply.github.com>
|
🎉 This PR is included in version 2.2.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
You can also share your feedback on Copilot code review. Take the survey.
| if [[ -z "$LOGGER_PATH" || ! -x "$LOGGER_PATH" ]]; then | ||
| # Mirror _write_to_journal: only warn once to avoid noisy stderr spam when | ||
| # logger is missing or untrusted. Subsequent calls still fail but stay quiet. | ||
| if [[ -z "$LOGGER_JOURNAL_ERROR_REPORTED" ]]; then |
There was a problem hiding this comment.
log_to_journal checks [[ -z "$LOGGER_JOURNAL_ERROR_REPORTED" ]] directly. In nounset mode (set -u), this will raise an unbound variable error the first time it runs. Use the existing pattern used elsewhere in this file (${LOGGER_JOURNAL_ERROR_REPORTED:-}) to keep the function nounset-safe.
| if [[ -z "$LOGGER_JOURNAL_ERROR_REPORTED" ]]; then | |
| if [[ -z "${LOGGER_JOURNAL_ERROR_REPORTED:-}" ]]; then |
| # Abort before any writes if logger is still unavailable after attempted discovery. | ||
| if [[ -z "$LOGGER_PATH" || ! -x "$LOGGER_PATH" ]]; then | ||
| # Mirror _write_to_journal: only warn once to avoid noisy stderr spam when | ||
| # logger is missing or untrusted. Subsequent calls still fail but stay quiet. | ||
| if [[ -z "$LOGGER_JOURNAL_ERROR_REPORTED" ]]; then | ||
| echo "WARNING: log_to_journal called but logger command is not available" >&2 | ||
| LOGGER_JOURNAL_ERROR_REPORTED="yes" | ||
| fi | ||
| return 1 | ||
| fi |
There was a problem hiding this comment.
The function comment and API docs describe log_to_journal as using normal routing (console/log file) and also forcing a journal write. However, the early return when LOGGER_PATH is unavailable aborts before calling _log_message, so nothing is written to console/log file in that case. Either update the docs to reflect that the message is not logged anywhere on journal failure, or restructure so console/file logging still occurs even when the journal write cannot be performed (while still returning non-zero).
| return 1 | ||
| fi | ||
|
|
||
| _log_message "$canonical_level" "$level_value" "$message" "false" "false" "true" |
There was a problem hiding this comment.
log_to_journal currently returns success as long as LOGGER_PATH exists and is executable, but it does not propagate a failure from _write_to_journal (e.g., if logger exits non-zero). This can lead to callers thinking a message was written to the journal when it wasn't. Consider invoking _write_to_journal in a way that allows log_to_journal to return its failure status, or refactor _log_message/_write_to_journal so the journal write result can be surfaced.
| _log_message "$canonical_level" "$level_value" "$message" "false" "false" "true" | |
| if ! _log_message "$canonical_level" "$level_value" "$message" "false" "false" "true"; then | |
| return 1 | |
| fi | |
| return 0 |
| init_logger --no-color --log '$log_file' --quiet | ||
| USE_JOURNAL='true' | ||
|
|
||
| log_sensitive 'sensitive_secret_value' | ||
| log_info 'public_info_message' | ||
| " 2>/dev/null | ||
|
|
||
| # Stub should NOT have received the sensitive message | ||
| if [[ -f "$STUB_CAPTURE" ]]; then | ||
| assert_file_not_contains "$STUB_CAPTURE" "sensitive_secret_value" \ | ||
| "Sensitive message must never reach the journal" || return | ||
| fi | ||
|
|
There was a problem hiding this comment.
This test verifies that log_sensitive does not reach the journal, but it never asserts that journal dispatch actually happened for the non-sensitive control message (log_info 'public_info_message'). If the journal write path is broken entirely (or the stub wasn’t invoked), the test can still pass. Add an assertion that the stub capture exists and contains the non-sensitive message to ensure the test is meaningful.
| test_log_to_journal_level_aliases() { | ||
| start_test "log_to_journal accepts level aliases (WARNING, ERR, CRIT, EMERG, FATAL)" | ||
|
|
||
| _create_stub_logger "$TEST_DIR" | ||
|
|
||
| local aliases=("WARNING" "ERR" "CRIT" "EMERG" "FATAL") | ||
| local alias | ||
|
|
||
| for alias in "${aliases[@]}"; do | ||
| bash -c " | ||
| source '$PROJECT_ROOT/logging.sh' | ||
|
|
||
| LOGGER_PATH='$STUB_LOGGER' | ||
| _find_and_validate_logger() { return 0; } | ||
| check_logger_available() { return 0; } | ||
|
|
||
| init_logger --no-color --quiet --level DEBUG | ||
| USE_JOURNAL='false' | ||
|
|
||
| log_to_journal '$alias' \"alias_test_${alias}\" | ||
| " 2>/dev/null | ||
|
|
||
| local exit_code=$? | ||
| if [[ $exit_code -ne 0 ]]; then | ||
| fail_test "log_to_journal '$alias' returned $exit_code; expected 0" | ||
| return | ||
| fi | ||
| done | ||
|
|
||
| pass_test | ||
| } | ||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Test 11: Numeric syslog levels (0-7) are accepted | ||
| # --------------------------------------------------------------------------- | ||
| test_log_to_journal_numeric_levels() { | ||
| start_test "log_to_journal accepts numeric syslog levels 0-7" | ||
|
|
||
| _create_stub_logger "$TEST_DIR" | ||
|
|
||
| local level | ||
| for level in 0 1 2 3 4 5 6 7; do | ||
| local exit_code | ||
| exit_code=$(bash -c " | ||
| source '$PROJECT_ROOT/logging.sh' | ||
|
|
||
| LOGGER_PATH='$STUB_LOGGER' | ||
| _find_and_validate_logger() { return 0; } | ||
| check_logger_available() { return 0; } | ||
|
|
||
| init_logger --no-color --quiet --level DEBUG | ||
| USE_JOURNAL='false' | ||
|
|
||
| log_to_journal '$level' 'numeric_level_test' | ||
| echo \$? | ||
| " 2>/dev/null) | ||
|
|
||
| assert_equals "0" "$exit_code" \ | ||
| "log_to_journal '$level' should return 0" || return | ||
| done |
There was a problem hiding this comment.
These alias/numeric-level tests only check log_to_journal's exit code. They don’t verify that the stub logger was actually invoked and that the message was written to the capture file, so they could pass even if no journal write occurs. Add assertions against $STUB_CAPTURE (e.g., expected marker present, and count matches the number of iterations) to validate the primary behavior under test.
| Total Tests: 466 | ||
| Passed: 461 | ||
| Failed: 0 | ||
| Skipped: 5 |
There was a problem hiding this comment.
The surrounding text now emphasizes using tests/run_tests.sh for authoritative totals, but this example output hard-codes specific test counts. These numbers will go stale as suites/tests change. Consider replacing the totals with placeholders (e.g., <n>) or removing the numeric lines entirely to avoid documentation drift.
| Total Tests: 466 | |
| Passed: 461 | |
| Failed: 0 | |
| Skipped: 5 | |
| Total Tests: <total> | |
| Passed: <passed> | |
| Failed: <failed> | |
| Skipped: <skipped> |