fix(configure.sh): disable notify_push unless Redis password is set - #145
Merged
printminion-co merged 4 commits intoAug 3, 2026
Merged
Conversation
The notify_push base URL warning was the last hand-rolled "\033[1;33m..." echo in the script. Use the log_warning helper so warnings are formatted consistently and go to stderr.
Route the remaining status echoes through the log_info helper so all progress output carries the same "[i] " prefix. The indented key = value lines of the market block and the single-line progress printf in disable_configured_apps are left untouched, since a per-line prefix would break their formatting.
notify_push without a reachable Redis throws while its console commands are constructed, and that error lands on stdout with exit code 0 (see nextcloud/server#62718). It then corrupts every occ call whose output the script captures, most notably the `occ app:list --enabled --output json` piped into jq in disable_configured_apps. Disable notify_push up front and only enable it when REDIS_HOST_PASSWORD is set, i.e. when the app can actually work.
Settle the notify_push state before any other occ invocation, so a broken notify_push bootstrap cannot corrupt the captured stdout of the config reads that follow (config:system:get in config_ui and log_market_config, app:list in disable_configured_apps).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
configure.sh:182
- The usage comment for this function doesn’t match the actual function name (comment says
configure_notify_push_app, but the function isconfigure_app_notify_push). This can mislead future edits and grepping.
# Configure notify_push app
# Usage: configure_notify_push_app
configure_app_notify_push() {
log_info "Configuring notify_push app..."
printminion-co
marked this pull request as ready for review
August 3, 2026 09:53
printminion-co
deleted the
mk/fix/disable-notify-push-without-redis-password
branch
August 3, 2026 10:35
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
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.
Problem
configure.shenablednotify_pushunconditionally. When the phpredis extension is loaded but no Redis is configured,notify_pushthrowsRedisException: Connection refusedwhile its console commands are constructed.OC\Console\Application::loadCommands()writes that error to stdout and the command still exits0:That corrupts every
occcall whose output this script captures — most fatally theocc app:list --enabled --output json | jqindisable_configured_apps, which then fails to parse and takes the whole configuration run with it.The stdout/stderr half of this is being fixed upstream in nextcloud/server#62718, but
configure.shshould not depend on that landing.Change
notify_pushis now disabled up front and only enabled whenREDIS_HOST_PASSWORDis set — i.e. only in an environment where the app can actually work. The call also moved to the top ofconfig_apps, so the app's state is settled before any otheroccinvocation whose output we read.Commits
Reviewable one by one, no behaviour change in the first two:
refactor(configure.sh): use log_warning instead of inline ANSI codes— the notify_push base URL warning was the last hand-rolled\033[1;33m…echo.refactor(configure.sh): use log_info for plain status output— remaining status echoes go through the helper. The indentedkey = valuelines of the market block and the single-line progressprintfindisable_configured_appsare deliberately left alone.fix(configure.sh): disable notify_push unless Redis password is set— the actual fix.fix(configure.sh): configure notify_push before the other apps— ordering.Notes
disable_single_app notify_pushis idempotent:occ app:disableexits0for an app that is not installed or already disabled (core/Command/App/Disable.php), so it only turns fatal on a genuine disable error.