swag: modernize module with improved error handling and UX#896
Draft
igorpecovnik wants to merge 25 commits into
Draft
swag: modernize module with improved error handling and UX#896igorpecovnik wants to merge 25 commits into
igorpecovnik wants to merge 25 commits into
Conversation
github-actions
Bot
added
05
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRefactors the SWAG module’s install and password flows to use dialog helpers, validate/sanitize entered domains (strip protocol and trailing path), persist sanitized SWAG_URL, restart the container and wait for dynamicssl.conf, and enforce non-empty credentials with explicit success/failure dialogs. Adds docker_configure_swag_proxy() to automate creating/enabling SWAG proxy confs and updates docker_operation_progress to preflight-check/install Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/modules/software/module_swag.sh`:
- Around line 162-165: The issue is a mismatched variable: the dialog result is
stored in sway_password but the subsequent check uses swag_password, causing the
"Enter Own" path to always behave as cancelled; fix by consistently using one
variable name (preferably swag_password) — update the dialog assignment to
swag_password=$(dialog_passwordbox ...) or, alternatively, change the
conditional to test [[ -z "$sway_password" ]], and ensure any later references
in the same function/flow use the same symbol (swag_password or sway_password)
consistently.
- Around line 46-55: The cleaned domain is incorrectly assigned to sway_url
instead of replacing swag_url, so downstream uses (hostnamectl and URL=) still
get the unsanitized value; change the assignment in the sanitization block so
the sanitized result is stored back into swag_url (or consistently use sway_url
everywhere) and ensure subsequent references (hostnamectl set-hostname and any
URL= usage) use the sanitized variable (swag_url) so hostname and config receive
the stripped domain.
- Around line 74-86: The script logs that SWAG will fall back to the default
bridge network when creation of the 'lsio' network fails, but the docker run
call in the docker_operation_progress invocation still unconditionally includes
--net=lsio; update module_swag.sh so the --net=lsio option is only added when
the earlier check/docker network create succeeded (e.g., track the result in a
boolean like LSIO_NET_EXISTS or reuse the docker network ls | grep -q "^lsio$"
check) and omit or change to --net=bridge when it did not succeed, ensuring the
docker_operation_progress run (the invocation that sets -d --name="$dockername"
--cap-add=NET_ADMIN ...) uses the correct network option.
- Around line 174-179: The script currently runs docker restart "$dockername"
and immediately shows dialog_infobox with credentials even if the restart
failed; update the logic around the docker restart call in module_swag.sh so you
capture its exit status and only call dialog_infobox "Password Configuration
Complete" (with ${swag_user}/${swag_password} and ${LOCALIPADD}) when the
restart succeeds, and otherwise show a failure message (and do not display
credentials) that includes the docker restart error/exit; reference the docker
restart "$dockername" invocation and the dialog_infobox call to locate and fix
the flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 56b10d4c-6b7f-483f-91b4-5a97cf910ce0
📒 Files selected for processing (1)
tools/modules/software/module_swag.sh
Comment on lines
+174
to
+179
| # Restart container to apply changes | ||
| docker restart "$dockername" >/dev/null 2>&1 | ||
|
|
||
| # Show success message with credentials | ||
| dialog_infobox "Password Configuration Complete" \ | ||
| "Username: ${swag_user}\nPassword: ${swag_password}\n\nWeb Interface: https://${LOCALIPADD}\n\nPlease save these credentials!" 10 70 |
Contributor
There was a problem hiding this comment.
Check docker restart before showing success.
If Line 175 fails, the code still displays a success message with credentials. That can leave the user thinking the password is active when the container never restarted.
Suggested fix
- # Restart container to apply changes
- docker restart "$dockername" >/dev/null 2>&1
+ # Restart container to apply changes
+ if ! docker restart "$dockername" >/dev/null 2>&1; then
+ dialog_msgbox "Password Configuration Failed" \
+ "Password was updated, but SWAG failed to restart.\n\nRestart it manually with:\n docker restart $dockername" 10 60
+ return 1
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Restart container to apply changes | |
| docker restart "$dockername" >/dev/null 2>&1 | |
| # Show success message with credentials | |
| dialog_infobox "Password Configuration Complete" \ | |
| "Username: ${swag_user}\nPassword: ${swag_password}\n\nWeb Interface: https://${LOCALIPADD}\n\nPlease save these credentials!" 10 70 | |
| # Restart container to apply changes | |
| if ! docker restart "$dockername" >/dev/null 2>&1; then | |
| dialog_msgbox "Password Configuration Failed" \ | |
| "Password was updated, but SWAG failed to restart.\n\nRestart it manually with:\n docker restart $dockername" 10 60 | |
| return 1 | |
| fi | |
| # Show success message with credentials | |
| dialog_infobox "Password Configuration Complete" \ | |
| "Username: ${swag_user}\nPassword: ${swag_password}\n\nWeb Interface: https://${LOCALIPADD}\n\nPlease save these credentials!" 10 70 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/modules/software/module_swag.sh` around lines 174 - 179, The script
currently runs docker restart "$dockername" and immediately shows dialog_infobox
with credentials even if the restart failed; update the logic around the docker
restart call in module_swag.sh so you capture its exit status and only call
dialog_infobox "Password Configuration Complete" (with
${swag_user}/${swag_password} and ${LOCALIPADD}) when the restart succeeds, and
otherwise show a failure message (and do not display credentials) that includes
the docker restart error/exit; reference the docker restart "$dockername"
invocation and the dialog_infobox call to locate and fix the flow.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tools/modules/software/module_wireguard.sh (1)
295-295: Use module variables instead of hardcoded status args.This status check should reuse
"$dockername"and"$dockerimage"to avoid future drift.Suggested refactor
- docker_is_installed "wireguard" "linuxserver/wireguard" + docker_is_installed "$dockername" "$dockerimage"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/modules/software/module_wireguard.sh` at line 295, Replace the hardcoded status check call docker_is_installed "wireguard" "linuxserver/wireguard" with the module variables by passing "$dockername" and "$dockerimage" instead; locate the docker_is_installed invocation in module_wireguard.sh (function or init sequence where the check is run) and change its arguments to use "$dockername" and "$dockerimage" so the check stays in sync with the module variables.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/modules/functions/module_docker_utils.sh`:
- Around line 145-148: Move the unbuffer availability check and pkg_install
expect call out of the global setup and into the branch that handles the pull
operation (the code that performs "pull" at / around the pull handler where
unbuffer is used), so that expect is only installed when performing pull;
additionally, change the behavior when unbuffer is not present to not hard-fail:
if command -v unbuffer returns false, skip pkg_install (or attempt it but do not
abort on failure) and use a safe fallback that calls plain curl (or the
non-unbuffered code path) for showing download progress; update the `unbuffer`
check and any calls that assume it in the pull handler to prefer unbuffer when
available and otherwise use the curl fallback, ensuring pkg_install expect is
not invoked for rm/rmi/run operations.
In `@tools/modules/software/module_swag.sh`:
- Around line 39-47: The code validates swag_url before sanitization but not
after, so inputs like "https://" become empty after the sed cleanup and later
break hostname/container config; after the existing sanitization line that sets
swag_url you should revalidate swag_url (the same check used earlier), and if
it's empty call dialog_msgbox "Installation Cancelled" with the same message and
return 1 so the function (where swag_url is used) aborts safely.
- Around line 98-113: The restart-and-wait block for SWAG can silently continue
on failure; explicitly check the docker restart result and the post-restart
timeout for /config/nginx/dynamicssl.conf and abort on failure: after docker
restart "$dockername" capture its exit status and on non-zero log an error via
dialog or process logger and exit with a non-zero code; in the while loop, if
the file never appears after the 30s wait, log a clear failure (including
$dockername and ${swag_url}) and exit non-zero so the install stops instead of
continuing in a partial state.
---
Nitpick comments:
In `@tools/modules/software/module_wireguard.sh`:
- Line 295: Replace the hardcoded status check call docker_is_installed
"wireguard" "linuxserver/wireguard" with the module variables by passing
"$dockername" and "$dockerimage" instead; locate the docker_is_installed
invocation in module_wireguard.sh (function or init sequence where the check is
run) and change its arguments to use "$dockername" and "$dockerimage" so the
check stays in sync with the module variables.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 400bbc25-9410-4261-af22-fdd9ed9d3792
📒 Files selected for processing (21)
tools/modules/functions/module_docker_utils.shtools/modules/software/module_bazarr.shtools/modules/software/module_code-server.shtools/modules/software/module_deluge.shtools/modules/software/module_duplicati.shtools/modules/software/module_embyserver.shtools/modules/software/module_jellyfin.shtools/modules/software/module_lidarr.shtools/modules/software/module_mariadb.shtools/modules/software/module_medusa.shtools/modules/software/module_nextcloud.shtools/modules/software/module_phpmyadmin.shtools/modules/software/module_prowlarr.shtools/modules/software/module_qbittorrent.shtools/modules/software/module_radarr.shtools/modules/software/module_sabnzbd.shtools/modules/software/module_sonarr.shtools/modules/software/module_swag.shtools/modules/software/module_syncthing.shtools/modules/software/module_transmission.shtools/modules/software/module_wireguard.sh
✅ Files skipped from review due to trivial changes (6)
- tools/modules/software/module_medusa.sh
- tools/modules/software/module_bazarr.sh
- tools/modules/software/module_transmission.sh
- tools/modules/software/module_qbittorrent.sh
- tools/modules/software/module_lidarr.sh
- tools/modules/software/module_duplicati.sh
Comment on lines
+145
to
+148
| # Ensure unbuffer is available (for real-time pull progress) | ||
| if ! command -v unbuffer >/dev/null 2>&1; then | ||
| pkg_install expect | ||
| fi |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 'command -v unbuffer|pkg_install expect|unbuffer curl|case "\$operation" in' tools/modules/functions/module_docker_utils.shRepository: armbian/configng
Length of output: 896
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for all unbuffer occurrences
rg -n 'unbuffer' tools/modules/functions/module_docker_utils.shRepository: armbian/configng
Length of output: 258
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the full docker_operation_progress function to understand scope
rg -n -A150 'docker_operation_progress\(\)' tools/modules/functions/module_docker_utils.sh | head -200Repository: armbian/configng
Length of output: 5930
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if expect is used elsewhere in the file
rg -n 'expect' tools/modules/functions/module_docker_utils.shRepository: armbian/configng
Length of output: 85
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get context around line 298 to understand jq --unbuffered usage
sed -n '290,305p' tools/modules/functions/module_docker_utils.shRepository: armbian/configng
Length of output: 790
Scope expect installation to pull and add a safe fallback.
The unbuffer availability check and expect package installation run unconditionally for all operations (pull, rm, rmi, run), but unbuffer is only used in the pull operation (line 255). For non-pull operations, this causes unnecessary system mutation before operation validation, and any installation failure would incorrectly block rm/rmi/run operations. Gate this to pull only, and if unbuffer is unavailable, fall back to plain curl instead of hard-failing.
Proposed fix
docker_operation_progress() {
local operation="$1"
local target="$2"
+ local use_unbuffer=0
@@
- # Ensure unbuffer is available (for real-time pull progress)
- if ! command -v unbuffer >/dev/null 2>&1; then
- pkg_install expect
- fi
+ # Only pull needs unbuffer; fallback to plain curl if unavailable
+ if [[ "$operation" == "pull" ]]; then
+ if command -v unbuffer >/dev/null 2>&1; then
+ use_unbuffer=1
+ elif pkg_install expect >/dev/null 2>&1 && command -v unbuffer >/dev/null 2>&1; then
+ use_unbuffer=1
+ fi
+ fi
@@
- unbuffer curl --silent --show-error \
+ if [[ $use_unbuffer -eq 1 ]]; then
+ unbuffer curl --silent --show-error \
+ --unix-socket "$socket_path" \
+ -X POST "http://localhost/images/create?fromImage=${pull_image}&tag=${pull_tag}" \
+ -w "%{http_code}" \
+ -o "$raw_response_file" \
+ 2> "$error_file" \
+ > "$http_code_file"
+ else
+ curl --silent --show-error \
+ --unix-socket "$socket_path" \
+ -X POST "http://localhost/images/create?fromImage=${pull_image}&tag=${pull_tag}" \
+ -w "%{http_code}" \
+ -o "$raw_response_file" \
+ 2> "$error_file" \
+ > "$http_code_file"
+ fi
- --unix-socket "$socket_path" \
- -X POST "http://localhost/images/create?fromImage=${pull_image}&tag=${pull_tag}" \
- -w "%{http_code}" \
- -o "$raw_response_file" \
- 2> "$error_file" \
- > "$http_code_file"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/modules/functions/module_docker_utils.sh` around lines 145 - 148, Move
the unbuffer availability check and pkg_install expect call out of the global
setup and into the branch that handles the pull operation (the code that
performs "pull" at / around the pull handler where unbuffer is used), so that
expect is only installed when performing pull; additionally, change the behavior
when unbuffer is not present to not hard-fail: if command -v unbuffer returns
false, skip pkg_install (or attempt it but do not abort on failure) and use a
safe fallback that calls plain curl (or the non-unbuffered code path) for
showing download progress; update the `unbuffer` check and any calls that assume
it in the pull handler to prefer unbuffer when available and otherwise use the
curl fallback, ensuring pkg_install expect is not invoked for rm/rmi/run
operations.
Comment on lines
+39
to
+47
| # Check if user cancelled or entered empty value | ||
| if [[ -z "$swag_url" ]]; then | ||
| dialog_msgbox "Installation Cancelled" \ | ||
| "A fully qualified domain name is required for SWAG to function properly." 10 70 | ||
| return 1 | ||
| fi | ||
|
|
||
| # Clean URL (remove protocol if present) | ||
| swag_url=$(echo "$swag_url" | sed -E 's|^\s*https?://||' | sed 's|/.*$||') |
Contributor
There was a problem hiding this comment.
Revalidate domain after sanitization.
A value like https:// passes the first empty check but becomes empty after cleanup, then gets used in hostname/container config.
Suggested fix
- # Clean URL (remove protocol if present)
- swag_url=$(echo "$swag_url" | sed -E 's|^\s*https?://||' | sed 's|/.*$||')
+ # Clean URL (remove protocol/path and trim whitespace)
+ swag_url=$(printf '%s' "$swag_url" | sed -E 's|^[[:space:]]*https?://||; s|/.*$||; s|^[[:space:]]+||; s|[[:space:]]+$||')
+
+ # Validate sanitized value
+ if [[ -z "$swag_url" ]]; then
+ dialog_msgbox "Installation Cancelled" \
+ "Invalid domain input. Please provide a valid fully qualified domain name." 10 70
+ return 1
+ fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/modules/software/module_swag.sh` around lines 39 - 47, The code
validates swag_url before sanitization but not after, so inputs like "https://"
become empty after the sed cleanup and later break hostname/container config;
after the existing sanitization line that sets swag_url you should revalidate
swag_url (the same check used earlier), and if it's empty call dialog_msgbox
"Installation Cancelled" with the same message and return 1 so the function
(where swag_url is used) aborts safely.
Comment on lines
+98
to
+113
| # Restart container to initialize SSL certificates | ||
| dialog_infobox "Initializing SSL Certificates" \ | ||
| "SWAG installed. Restarting to initialize SSL certificates for:\n ${swag_url}\n\nPlease wait..." 12 70 | ||
| sleep 2 | ||
| docker restart "$dockername" >/dev/null 2>&1 | ||
|
|
||
| # Wait for SSL certificate initialization (up to 30 seconds) | ||
| local wait_count=0 | ||
| while [[ $wait_count -lt 30 ]]; do | ||
| if docker exec "$dockername" test -f /config/nginx/dynamicssl.conf 2>/dev/null; then | ||
| break | ||
| fi | ||
| sleep 1 | ||
| ((wait_count++)) | ||
| done | ||
|
|
Contributor
There was a problem hiding this comment.
Handle restart/SSL-init failures explicitly in install flow.
The code continues even if restart fails or dynamicssl.conf never appears, which can leave install in a partial state.
Suggested fix
- docker restart "$dockername" >/dev/null 2>&1
+ if ! docker restart "$dockername" >/dev/null 2>&1; then
+ dialog_msgbox "Installation Failed" \
+ "SWAG was installed, but failed to restart for SSL initialization.\n\nCheck with:\n docker logs $dockername" 12 70
+ return 1
+ fi
@@
local wait_count=0
while [[ $wait_count -lt 30 ]]; do
if docker exec "$dockername" test -f /config/nginx/dynamicssl.conf 2>/dev/null; then
break
fi
sleep 1
((wait_count++))
done
+ if [[ $wait_count -ge 30 ]]; then
+ dialog_msgbox "SSL Initialization Timeout" \
+ "SWAG did not finish SSL initialization within 30 seconds.\n\nCheck logs:\n docker logs $dockername" 12 70
+ return 1
+ fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/modules/software/module_swag.sh` around lines 98 - 113, The
restart-and-wait block for SWAG can silently continue on failure; explicitly
check the docker restart result and the post-restart timeout for
/config/nginx/dynamicssl.conf and abort on failure: after docker restart
"$dockername" capture its exit status and on non-zero log an error via dialog or
process logger and exit with a non-zero code; in the while loop, if the file
never appears after the 30s wait, log a clear failure (including $dockername and
${swag_url}) and exit non-zero so the install stops instead of continuing in a
partial state.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
tools/modules/software/module_swag.sh (4)
187-194:⚠️ Potential issue | 🟡 MinorOnly show password success after the restart succeeds.
If the restart fails here, the dialog still tells the user the new credentials are active.
Suggested fix
if docker exec -it "$dockername" htpasswd -b -c /config/nginx/.htpasswd "${swag_user}" "${swag_password}" >/dev/null 2>&1; then # Restart container to apply changes - docker restart "$dockername" >/dev/null 2>&1 + if ! docker restart "$dockername" >/dev/null 2>&1; then + dialog_msgbox "Password Configuration Failed" \ + "Password was updated, but SWAG failed to restart.\n\nRestart it manually with:\n docker restart $dockername" 10 70 + return 1 + fi # Show success message with credentials (using domain URL, not IP) dialog_infobox "Password Configuration Complete" \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/modules/software/module_swag.sh` around lines 187 - 194, The success dialog is shown unconditionally after attempting docker restart; change the logic so the "Password Configuration Complete" dialog (the dialog_infobox call that prints Username/Password and Web Interface) is only executed if docker restart "$dockername" succeeds — i.e., capture the exit status of docker restart and move the dialog_infobox into the restart-success branch (or add an explicit if ...; then ...; else ...; fi) and handle the restart failure path (log or show an error) instead of displaying success when restart fails.
99-116:⚠️ Potential issue | 🟠 MajorStop the install when SSL bootstrap fails.
Both the restart and the 30-second wait can fail silently, but the flow still persists
SWAG_URLand drops straight into password setup.Suggested fix
dialog_infobox "Initializing SSL Certificates" \ "SWAG installed. Restarting to initialize SSL certificates for:\n ${swag_url}\n\nPlease wait..." 12 70 sleep 2 - docker restart "$dockername" >/dev/null 2>&1 + if ! docker restart "$dockername" >/dev/null 2>&1; then + dialog_msgbox "Installation Failed" \ + "SWAG was installed, but failed to restart for SSL initialization.\n\nCheck logs with:\n docker logs $dockername" 12 70 + return 1 + fi # Wait for SSL certificate initialization (up to 30 seconds) local wait_count=0 while [[ $wait_count -lt 30 ]]; do if docker exec "$dockername" test -f /config/nginx/dynamicssl.conf 2>/dev/null; then break fi sleep 1 ((wait_count++)) done + + if [[ $wait_count -ge 30 ]]; then + dialog_msgbox "SSL Initialization Timeout" \ + "SWAG did not finish SSL initialization within 30 seconds.\n\nCheck logs with:\n docker logs $dockername" 12 70 + return 1 + fi # Set password ${module_options["module_swag,feature"]} ${commands[4]}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/modules/software/module_swag.sh` around lines 99 - 116, The restart-and-wait steps for SWAG must abort the install on failure: check the exit status of docker restart "$dockername" and if it fails call the existing error/abort flow (show a dialog/error and exit), and after the while loop verify that the test for /config/nginx/dynamicssl.conf actually succeeded (e.g., boolean from the loop or check test -f one more time); if the file is not present after the timeout, stop the script instead of continuing to the password step invoked by ${module_options["module_swag,feature"]} ${commands[4]}. Ensure you reference the variables dockername, wait_count, and the final password setup invocation when adding the error/exit handling so the flow cannot proceed on failed SSL bootstrap.
39-47:⚠️ Potential issue | 🟠 MajorRevalidate the domain after sanitization.
https://or whitespace-only input passes the first check, becomes empty/invalid after cleanup, and then flows intohostnamectl,URL=, and the persistedSWAG_URL.Suggested fix
- # Clean URL (remove protocol if present) - swag_url=$(echo "$swag_url" | sed -E 's|^\s*https?://||' | sed 's|/.*$||') + # Clean URL (remove protocol/path and trim whitespace) + swag_url=$(printf '%s' "$swag_url" | sed -E 's|^[[:space:]]*https?://||; s|/.*$||; s|^[[:space:]]+||; s|[[:space:]]+$||') + + if [[ -z "$swag_url" ]]; then + dialog_msgbox "Installation Cancelled" \ + "Invalid domain input. Please provide a valid fully qualified domain name." 10 70 + return 1 + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/modules/software/module_swag.sh` around lines 39 - 47, After stripping protocol/path into the swag_url variable the code must revalidate it — if swag_url becomes empty or only whitespace after the cleanup, abort and show the same dialog/return status instead of continuing to call hostnamectl or writing URL=/persisted SWAG_URL; update the flow in module_swag.sh to (1) perform the sanitization of swag_url, (2) immediately check [[ -z "$swag_url" ]] and call dialog_msgbox "Installation Cancelled" and return 1 if empty, and only then proceed to hostnamectl and persisting URL= / SWAG_URL.
74-89:⚠️ Potential issue | 🟠 MajorDon't hard-require the
lsionetwork on fresh installs.
docker run --net=lsiostill assumes the custom network already exists, so a first-time install can fail before the container ever starts.Suggested fix
+ local docker_network_args=() + if docker network ls --format "{{.Name}}" | grep -q '^lsio$' || \ + docker network create lsio >/dev/null 2>&1; then + docker_network_args=(--net=lsio) + else + dialog_msgbox "Warning" \ + "Failed to create 'lsio' Docker network.\n\nSWAG will use the default bridge network." 8 60 + fi + # Run container if ! docker_operation_progress run "$dockername" \ -d \ --name="$dockername" \ --cap-add=NET_ADMIN \ - --net=lsio \ + "${docker_network_args[@]}" \ -e PUID="${DOCKER_USERUID}" \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/modules/software/module_swag.sh` around lines 74 - 89, The run command in module_swag.sh currently forces --net=lsio which breaks fresh installs; modify the logic around the docker_operation_progress run invocation so it does not hard-require the lsio network: before calling docker_operation_progress run "$dockername" (the block that includes --net=lsio), check whether the Docker network "lsio" exists (e.g., via docker network inspect) and only include the --net=lsio option if that check succeeds, otherwise either create the network or omit the --net flag so the container can start on the default network; update the invocation that builds the docker run options accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tools/modules/software/module_swag.sh`:
- Around line 187-194: The success dialog is shown unconditionally after
attempting docker restart; change the logic so the "Password Configuration
Complete" dialog (the dialog_infobox call that prints Username/Password and Web
Interface) is only executed if docker restart "$dockername" succeeds — i.e.,
capture the exit status of docker restart and move the dialog_infobox into the
restart-success branch (or add an explicit if ...; then ...; else ...; fi) and
handle the restart failure path (log or show an error) instead of displaying
success when restart fails.
- Around line 99-116: The restart-and-wait steps for SWAG must abort the install
on failure: check the exit status of docker restart "$dockername" and if it
fails call the existing error/abort flow (show a dialog/error and exit), and
after the while loop verify that the test for /config/nginx/dynamicssl.conf
actually succeeded (e.g., boolean from the loop or check test -f one more time);
if the file is not present after the timeout, stop the script instead of
continuing to the password step invoked by
${module_options["module_swag,feature"]} ${commands[4]}. Ensure you reference
the variables dockername, wait_count, and the final password setup invocation
when adding the error/exit handling so the flow cannot proceed on failed SSL
bootstrap.
- Around line 39-47: After stripping protocol/path into the swag_url variable
the code must revalidate it — if swag_url becomes empty or only whitespace after
the cleanup, abort and show the same dialog/return status instead of continuing
to call hostnamectl or writing URL=/persisted SWAG_URL; update the flow in
module_swag.sh to (1) perform the sanitization of swag_url, (2) immediately
check [[ -z "$swag_url" ]] and call dialog_msgbox "Installation Cancelled" and
return 1 if empty, and only then proceed to hostnamectl and persisting URL= /
SWAG_URL.
- Around line 74-89: The run command in module_swag.sh currently forces
--net=lsio which breaks fresh installs; modify the logic around the
docker_operation_progress run invocation so it does not hard-require the lsio
network: before calling docker_operation_progress run "$dockername" (the block
that includes --net=lsio), check whether the Docker network "lsio" exists (e.g.,
via docker network inspect) and only include the --net=lsio option if that check
succeeds, otherwise either create the network or omit the --net flag so the
container can start on the default network; update the invocation that builds
the docker run options accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 025fa820-99ff-4942-8a43-bcba4da83730
📒 Files selected for processing (4)
tools/modules/functions/module_env_init.shtools/modules/runtime/config.runtime.shtools/modules/software/module_swag.shtools/modules/software/module_transmission.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/modules/software/module_transmission.sh
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/modules/software/module_transmission.sh (1)
44-64:⚠️ Potential issue | 🟠 MajorHandle SWAG proxy setup as best-effort, not the install's exit status.
docker_configure_swag_proxyreturns2when SWAG is absent, so this bare call can make a normal Transmission install fail on systems without SWAG. It also becomes the branch's final status, which can mask an earlierdocker_operation_progress runfailure if the proxy setup succeeds. Capture the run result first, then treat SWAG setup as optional.Suggested control flow
- docker_operation_progress run "$dockername" \ + if ! docker_operation_progress run "$dockername" \ -d \ --name="$dockername" \ @@ - "$dockerimage" - # Auto-configure SWAG reverse proxy if available - docker_configure_swag_proxy "transmission" + "$dockerimage"; then + return 1 + fi + + # Auto-configure SWAG reverse proxy if available + if ! docker_configure_swag_proxy "transmission"; then + [[ $? -eq 2 ]] || dialog_msgbox "Warning" "SWAG proxy setup failed." + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/modules/software/module_transmission.sh` around lines 44 - 64, The install currently calls docker_configure_swag_proxy directly which can return 2 when SWAG is absent and overwrite the real install status; change the flow in the Transmission install branch so you first capture the exit code of docker_operation_progress run (the command invoking docker_operation_progress run "$dockername" ... "$dockerimage") into a variable, then call docker_configure_swag_proxy "transmission" but treat its return code as best-effort (ignore a return of 2 and do not let its non-zero value replace the previously captured run result), and finally exit/return using the docker_operation_progress run exit code so that a failed container start is preserved while missing SWAG does not cause failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/modules/functions/module_docker_utils.sh`:
- Around line 503-507: The script currently returns success immediately after
touching the SWAG proxy conf; change the logic in the block that runs docker
exec swag touch
"/config/nginx/proxy-confs/${servicename}.subfolder.conf.enabled" so that it
only returns 0 if the subsequent docker exec swag nginx -s reload succeeds; run
the reload, capture its exit status (from the docker exec nginx command), and
propagate a non-zero return if the reload fails (include context in logs if
present) instead of ignoring the reload's failure.
---
Outside diff comments:
In `@tools/modules/software/module_transmission.sh`:
- Around line 44-64: The install currently calls docker_configure_swag_proxy
directly which can return 2 when SWAG is absent and overwrite the real install
status; change the flow in the Transmission install branch so you first capture
the exit code of docker_operation_progress run (the command invoking
docker_operation_progress run "$dockername" ... "$dockerimage") into a variable,
then call docker_configure_swag_proxy "transmission" but treat its return code
as best-effort (ignore a return of 2 and do not let its non-zero value replace
the previously captured run result), and finally exit/return using the
docker_operation_progress run exit code so that a failed container start is
preserved while missing SWAG does not cause failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a46097bb-6f0f-48b5-97fc-58cd5e6d8c05
📒 Files selected for processing (3)
tools/modules/functions/module_docker_utils.shtools/modules/runtime/config.runtime.shtools/modules/software/module_transmission.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/modules/runtime/config.runtime.sh
Comment on lines
+503
to
+507
| # Enable the proxy configuration | ||
| if docker exec swag touch "/config/nginx/proxy-confs/${servicename}.subfolder.conf.enabled" 2>/dev/null; then | ||
| # Reload nginx to apply | ||
| docker exec swag nginx -s reload >/dev/null 2>&1 | ||
| return 0 |
Contributor
There was a problem hiding this comment.
Don't return success until the nginx reload succeeds.
docker exec swag nginx -s reload is ignored here, so the helper can return 0 even when SWAG rejects the new config. That makes the new auto-configuration look successful while leaving the proxy inactive.
Proposed fix
# Enable the proxy configuration
- if docker exec swag touch "/config/nginx/proxy-confs/${servicename}.subfolder.conf.enabled" 2>/dev/null; then
- # Reload nginx to apply
- docker exec swag nginx -s reload >/dev/null 2>&1
- return 0
- fi
+ if ! docker exec swag touch "/config/nginx/proxy-confs/${servicename}.subfolder.conf.enabled" 2>/dev/null; then
+ return 1
+ fi
+ if ! docker exec swag nginx -s reload >/dev/null 2>&1; then
+ return 1
+ fi
+ return 0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Enable the proxy configuration | |
| if docker exec swag touch "/config/nginx/proxy-confs/${servicename}.subfolder.conf.enabled" 2>/dev/null; then | |
| # Reload nginx to apply | |
| docker exec swag nginx -s reload >/dev/null 2>&1 | |
| return 0 | |
| # Enable the proxy configuration | |
| if ! docker exec swag touch "/config/nginx/proxy-confs/${servicename}.subfolder.conf.enabled" 2>/dev/null; then | |
| return 1 | |
| fi | |
| if ! docker exec swag nginx -s reload >/dev/null 2>&1; then | |
| return 1 | |
| fi | |
| return 0 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/modules/functions/module_docker_utils.sh` around lines 503 - 507, The
script currently returns success immediately after touching the SWAG proxy conf;
change the logic in the block that runs docker exec swag touch
"/config/nginx/proxy-confs/${servicename}.subfolder.conf.enabled" so that it
only returns 0 if the subsequent docker exec swag nginx -s reload succeeds; run
the reload, capture its exit status (from the docker exec nginx command), and
propagate a non-zero return if the reload fails (include context in logs if
present) instead of ignoring the reload's failure.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
tools/modules/functions/module_docker_utils.sh (2)
510-514:⚠️ Potential issue | 🟠 MajorPropagate nginx reload failures.
The helper still returns success immediately after touching the
.enabledfile, even ifnginx -s reloadfails. That leaves the proxy inactive while reporting success. This was already raised on an earlier revision and still applies.Suggested fix
- if docker exec swag touch "/config/nginx/proxy-confs/${servicename}.subfolder.conf.enabled" 2>/dev/null; then - # Reload nginx to apply - docker exec swag nginx -s reload >/dev/null 2>&1 - return 0 - fi + if ! docker exec swag touch "/config/nginx/proxy-confs/${servicename}.subfolder.conf.enabled" 2>/dev/null; then + return 1 + fi + if ! docker exec swag nginx -s reload >/dev/null 2>&1; then + return 1 + fi + return 0 return 1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/modules/functions/module_docker_utils.sh` around lines 510 - 514, The current block touches "/config/nginx/proxy-confs/${servicename}.subfolder.conf.enabled" then always returns success even if the subsequent "docker exec swag nginx -s reload" fails; change it so the function runs "docker exec swag nginx -s reload", captures its exit status, and only returns 0 on success and a non-zero exit code on failure (propagate the reload failure). Locate the block using the touch command for "/config/nginx/proxy-confs/${servicename}.subfolder.conf.enabled" and the docker exec swag nginx -s reload invocation and modify the flow to check the reload command's exit code and return that code (or return 1) when it fails.
145-148:⚠️ Potential issue | 🟠 MajorScope
expectinstallation topulland keep a fallback path.
rm,rmi, andrunnever useunbuffer, but they still triggerpkg_install expectbefore validation. That means a package-install failure can block unrelated Docker operations. This was already flagged on an earlier revision and still applies.Run this to confirm the current flow:
#!/bin/bash set -euo pipefail echo "== preflight block ==" sed -n '131,170p' tools/modules/functions/module_docker_utils.sh echo echo "== unbuffer / expect usages ==" rg -n -C2 '\bunbuffer\b|\bpkg_install expect\b' tools/modules/functions/module_docker_utils.shExpected result:
pkg_install expectappears before the operation dispatch, whileunbufferis only used in thepull)branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/modules/functions/module_docker_utils.sh` around lines 145 - 148, The current preflight always installs expect (pkg_install expect) for all docker subcommands because the unbuffer check runs before dispatch; move the unbuffer/expect installation logic into the pull) case so only docker pull attempts to install expect, and remove the global preflight install; in the pull) branch, prefer using unbuffer when available (check for command -v unbuffer) but implement a clear fallback to run docker pull without unbuffer if it's absent or pkg_install fails, and keep references to the existing symbols unbuffer, pkg_install, and the pull) case so you can find and update the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/modules/functions/module_docker_utils.sh`:
- Around line 490-493: The helper currently treats a missing "swag" container as
an error by returning 2 when the check docker container ls -a --format
"{{.Names}}" | grep -q "^swag$" fails; change this behavior to be a no-op for
optional auto-configuration by removing the error return (or returning 0) so
callers don’t treat absence of the "swag" container as a failure—update the
block that checks for the "swag" container in module_docker_utils.sh
accordingly.
- Around line 499-508: The code currently runs docker exec swag cp
"$proxy_sample" "$proxy_actual" and docker exec swag sed -i ... without checking
their exit status, so failures can be hidden; update the block around the cp and
sed invocations (references: variables proxy_sample, proxy_actual, port and the
docker exec swag cp / docker exec swag sed -i commands) to check each command's
return code and fail fast: if the cp fails, log an error and exit/return
non-zero immediately instead of continuing, and similarly after running sed -i
(only when port is set) verify it succeeded and abort with an error if it did
not; ensure the error messages provide context (which file and command failed)
so callers can detect and handle the failure.
In `@tools/modules/software/module_homepage.sh`:
- Around line 53-54: The docker_configure_swag_proxy call can override or
misreport the module install result; wrap the docker_configure_swag_proxy
"homepage" "3000" invocation in the same install-status guard used after
docker_operation_progress run so the proxy configuration only runs when the
container start/install succeeded. Check the success status returned by
docker_operation_progress (or the module's install status variable) before
calling docker_configure_swag_proxy and skip the proxy step when the install
failed or when swag is absent.
In `@tools/modules/software/module_portainer.sh`:
- Around line 51-52: The optional SWAG step (docker_configure_swag_proxy
"portainer" "9000") can overwrite the install result; capture the exit code of
the Portainer start (the docker_operation_progress run that starts Portainer),
then only call docker_configure_swag_proxy if that exit code is zero, and
finally exit/return with the original Portainer start exit code so the install
status isn't masked by the optional proxy step; reference
docker_operation_progress and docker_configure_swag_proxy when locating where to
add the exit-code check and conditional call.
---
Duplicate comments:
In `@tools/modules/functions/module_docker_utils.sh`:
- Around line 510-514: The current block touches
"/config/nginx/proxy-confs/${servicename}.subfolder.conf.enabled" then always
returns success even if the subsequent "docker exec swag nginx -s reload" fails;
change it so the function runs "docker exec swag nginx -s reload", captures its
exit status, and only returns 0 on success and a non-zero exit code on failure
(propagate the reload failure). Locate the block using the touch command for
"/config/nginx/proxy-confs/${servicename}.subfolder.conf.enabled" and the docker
exec swag nginx -s reload invocation and modify the flow to check the reload
command's exit code and return that code (or return 1) when it fails.
- Around line 145-148: The current preflight always installs expect (pkg_install
expect) for all docker subcommands because the unbuffer check runs before
dispatch; move the unbuffer/expect installation logic into the pull) case so
only docker pull attempts to install expect, and remove the global preflight
install; in the pull) branch, prefer using unbuffer when available (check for
command -v unbuffer) but implement a clear fallback to run docker pull without
unbuffer if it's absent or pkg_install fails, and keep references to the
existing symbols unbuffer, pkg_install, and the pull) case so you can find and
update the logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 87adb2e9-8b80-47ef-b589-5f1fd032b6a6
📒 Files selected for processing (4)
tools/modules/functions/module_docker_utils.shtools/modules/software/module_homepage.shtools/modules/software/module_portainer.shtools/modules/software/module_transmission.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/modules/software/module_transmission.sh
Comment on lines
+490
to
+493
| # Check if SWAG container exists | ||
| if ! docker container ls -a --format "{{.Names}}" | grep -q "^swag$"; then | ||
| return 2 | ||
| fi |
Contributor
There was a problem hiding this comment.
Treat missing swag as a no-op in this helper.
This helper is used for optional auto-configuration, but it returns 2 when the swag container is absent. Callers that don't special-case that status turn a successful install into a failure path, which is what the new module install flows now do.
Suggested fix
# Check if SWAG container exists
if ! docker container ls -a --format "{{.Names}}" | grep -q "^swag$"; then
- return 2
+ return 0
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Check if SWAG container exists | |
| if ! docker container ls -a --format "{{.Names}}" | grep -q "^swag$"; then | |
| return 2 | |
| fi | |
| # Check if SWAG container exists | |
| if ! docker container ls -a --format "{{.Names}}" | grep -q "^swag$"; then | |
| return 0 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/modules/functions/module_docker_utils.sh` around lines 490 - 493, The
helper currently treats a missing "swag" container as an error by returning 2
when the check docker container ls -a --format "{{.Names}}" | grep -q "^swag$"
fails; change this behavior to be a no-op for optional auto-configuration by
removing the error return (or returning 0) so callers don’t treat absence of the
"swag" container as a failure—update the block that checks for the "swag"
container in module_docker_utils.sh accordingly.
Comment on lines
+499
to
+508
| if docker exec swag test -f "$proxy_sample" 2>/dev/null; then | ||
| # Copy sample to actual config if it doesn't exist | ||
| if ! docker exec swag test -f "$proxy_actual" 2>/dev/null; then | ||
| docker exec swag cp "$proxy_sample" "$proxy_actual" 2>/dev/null | ||
| fi | ||
|
|
||
| # If port is specified, update it in the config | ||
| if [[ -n "$port" ]]; then | ||
| docker exec swag sed -i "s/set \\\$upstream_port [0-9]*/set \\\$upstream_port ${port}/g" "$proxy_actual" 2>/dev/null | ||
| fi |
Contributor
There was a problem hiding this comment.
Fail fast when creating or rewriting the proxy config fails.
Both cp and sed -i are unchecked here. The helper can therefore return success with no generated config or with the old upstream_port still in place.
Suggested fix
- if docker exec swag test -f "$proxy_sample" 2>/dev/null; then
- # Copy sample to actual config if it doesn't exist
- if ! docker exec swag test -f "$proxy_actual" 2>/dev/null; then
- docker exec swag cp "$proxy_sample" "$proxy_actual" 2>/dev/null
- fi
-
- # If port is specified, update it in the config
- if [[ -n "$port" ]]; then
- docker exec swag sed -i "s/set \\\$upstream_port [0-9]*/set \\\$upstream_port ${port}/g" "$proxy_actual" 2>/dev/null
- fi
+ if docker exec swag test -f "$proxy_actual" 2>/dev/null || docker exec swag test -f "$proxy_sample" 2>/dev/null; then
+ if ! docker exec swag test -f "$proxy_actual" 2>/dev/null; then
+ docker exec swag cp "$proxy_sample" "$proxy_actual" 2>/dev/null || return 1
+ fi
+
+ if [[ -n "$port" ]]; then
+ docker exec swag sed -i "s/set \\\$upstream_port [0-9]*/set \\\$upstream_port ${port}/g" "$proxy_actual" 2>/dev/null || return 1
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if docker exec swag test -f "$proxy_sample" 2>/dev/null; then | |
| # Copy sample to actual config if it doesn't exist | |
| if ! docker exec swag test -f "$proxy_actual" 2>/dev/null; then | |
| docker exec swag cp "$proxy_sample" "$proxy_actual" 2>/dev/null | |
| fi | |
| # If port is specified, update it in the config | |
| if [[ -n "$port" ]]; then | |
| docker exec swag sed -i "s/set \\\$upstream_port [0-9]*/set \\\$upstream_port ${port}/g" "$proxy_actual" 2>/dev/null | |
| fi | |
| if docker exec swag test -f "$proxy_actual" 2>/dev/null || docker exec swag test -f "$proxy_sample" 2>/dev/null; then | |
| # Copy sample to actual config if it doesn't exist | |
| if ! docker exec swag test -f "$proxy_actual" 2>/dev/null; then | |
| docker exec swag cp "$proxy_sample" "$proxy_actual" 2>/dev/null || return 1 | |
| fi | |
| # If port is specified, update it in the config | |
| if [[ -n "$port" ]]; then | |
| docker exec swag sed -i "s/set \\\$upstream_port [0-9]*/set \\\$upstream_port ${port}/g" "$proxy_actual" 2>/dev/null || return 1 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/modules/functions/module_docker_utils.sh` around lines 499 - 508, The
code currently runs docker exec swag cp "$proxy_sample" "$proxy_actual" and
docker exec swag sed -i ... without checking their exit status, so failures can
be hidden; update the block around the cp and sed invocations (references:
variables proxy_sample, proxy_actual, port and the docker exec swag cp / docker
exec swag sed -i commands) to check each command's return code and fail fast: if
the cp fails, log an error and exit/return non-zero immediately instead of
continuing, and similarly after running sed -i (only when port is set) verify it
succeeded and abort with an error if it did not; ensure the error messages
provide context (which file and command failed) so callers can detect and handle
the failure.
Comment on lines
+53
to
+54
| # Auto-configure SWAG reverse proxy if available | ||
| docker_configure_swag_proxy "homepage" "3000" |
Contributor
There was a problem hiding this comment.
Apply the same install-status guard here.
This optional proxy step can override the result of docker_operation_progress run. On a failed container start, the module can still return success; on a host without swag, a successful Homepage install returns failure.
Suggested fix
- docker_configure_swag_proxy "homepage" "3000"
+ docker_configure_swag_proxy "homepage" "3000" || [[ $? -eq 2 ]]- "$dockerimage"
+ "$dockerimage" || return 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/modules/software/module_homepage.sh` around lines 53 - 54, The
docker_configure_swag_proxy call can override or misreport the module install
result; wrap the docker_configure_swag_proxy "homepage" "3000" invocation in the
same install-status guard used after docker_operation_progress run so the proxy
configuration only runs when the container start/install succeeded. Check the
success status returned by docker_operation_progress (or the module's install
status variable) before calling docker_configure_swag_proxy and skip the proxy
step when the install failed or when swag is absent.
Comment on lines
+51
to
+52
| # Auto-configure SWAG reverse proxy if available | ||
| docker_configure_swag_proxy "portainer" "9000" |
Contributor
There was a problem hiding this comment.
Don't let the optional SWAG step overwrite the install result.
This runs even when docker_operation_progress run failed, and its exit code becomes the install branch status. That can both mask a failed Portainer start and fail a successful install on hosts where swag is not present.
Suggested fix
docker_operation_progress run "$dockername" \
-d \
--name="$dockername" \
--restart=always \
-p 9000:9000 \
-p 8000:8000 \
-p 9443:9443 \
-v /run/docker.sock:/var/run/docker.sock \
-v "${base_dir}/data:/data" \
- "$dockerimage"
+ "$dockerimage" || return 1
# Auto-configure SWAG reverse proxy if available
- docker_configure_swag_proxy "portainer" "9000"
+ docker_configure_swag_proxy "portainer" "9000" || [[ $? -eq 2 ]]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/modules/software/module_portainer.sh` around lines 51 - 52, The
optional SWAG step (docker_configure_swag_proxy "portainer" "9000") can
overwrite the install result; capture the exit code of the Portainer start (the
docker_operation_progress run that starts Portainer), then only call
docker_configure_swag_proxy if that exit code is zero, and finally exit/return
with the original Portainer start exit code so the install status isn't masked
by the optional proxy step; reference docker_operation_progress and
docker_configure_swag_proxy when locating where to add the exit-code check and
conditional call.
Update module_swag.sh to follow current armbian-config patterns: Changes: - Replace direct dialog calls with wrapper functions (dialog_inputbox, dialog_passwordbox, dialog_yesno, dialog_msgbox, dialog_infobox) - Add comprehensive error handling with user-friendly messages - Make hostname change optional with confirmation dialog - Add URL sanitization to remove protocol prefixes - Add lsio network existence check before container creation - Add container status check in password command - Improve variable naming consistency (swag_* prefix) - Add help text with web interface URL and documentation link - Add success/info dialogs throughout the workflow - Fix typos in variable names Benefits: - Better user experience with clear error messages - More robust handling of edge cases - Consistent with other software modules - Prevents silent failures - Provides actionable feedback when operations fail Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace lscr.io/linuxserver/* with linuxserver/* to use Docker Hub instead of the LinuxServer.io registry, which resolves pull failures when lscr.io is not accessible. Root cause: lscr.io registry is not reachable from many networks, causing "pull failed" errors during installation. Docker Hub (docker.io/linuxserver/*) is more widely accessible and reliable. Changed 20 modules: - bazarr, code-server, deluge, duplicati, embyserver, jellyfin - lidarr, mariadb, medusa, nextcloud, phpmyadmin, prowlarr - qbittorrent, radarr, sabnzbd, sonarr, swag, syncthing - transmission, wireguard All modules now pull from linuxserver/* on Docker Hub which is the canonical registry for LinuxServer.io images. Fixes installation failures for users without lscr.io access. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Fix UX issues with dialog windows being too small: - Increase all dialog box dimensions to prevent text cutoff - Input boxes: 15x90 and 12x80 (was 10x80 and 10x70) - Message boxes: 10-12x60-70 (was 8-10x50-60) - Yes/No dialogs: 15x80 (was 12x70) - Info boxes: 15x80 (was 10x70) - Remove redundant success dialog between container creation and password setup to reduce click count - Improve dialog titles for better clarity - Enhance message wording throughout Benefits: - Windows properly sized for content - One less click during installation - Better user experience with readable dialogs Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Fix 'unbuffer: command not found' error during Docker image pulls by automatically installing the expect package when unbuffer is not available. The unbuffer command (from expect package) is required for real-time progress display during Docker image pulls via Docker API. This was causing failures for users who didn't have expect installed. Changes: - Add unbuffer availability check in docker_operation_progress() - Auto-install expect package via pkg_install if missing - Ensures smooth Docker pull experience for all users Fixes errors during image pulls in SWAG and other Docker modules. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Fix two issues reported during testing: 1. SSL certificates not working until manual container restart: - Add automatic restart after container creation - Wait up to 30 seconds for SSL certificate initialization - Show info dialog during certificate initialization - Store domain URL in config file for later use 2. Success messages showing IP instead of domain URL: - Store swag_url to /armbian/swag/config/.swag_url during install - Read stored URL in password command for display - Read stored URL in help command for display - Fallback to LOCALIPADD if stored URL not found Changes: - Container restart after installation for SSL certificate init - URL storage/retrieval system for domain display - All user-facing messages now show domain URL instead of IP - 30-second wait for certificate initialization with progress dialog Benefits: - SSL certificates work immediately after installation - Users see their actual domain in messages, not IP address - Better UX with clear feedback during certificate init Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Remove lsio network creation/check code as this is already handled by the Docker module during installation. The Docker module ensures the lsio network exists, so individual software modules don't need to duplicate this check. Reduces code duplication and simplifies SWAG module. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Change from .swag_url (hidden file) to SWAG_URL (regular file) for storing the domain URL configuration. Changes: - Store to SWAG_URL instead of .swag_url - Use SWAG_URL globally across password, help, and status commands - Fallback to LOCALIPADD only if SWAG_URL file doesn't exist - Update comments to clarify global usage pattern Benefits: - Consistent naming with environment variable conventions (uppercase) - Regular file instead of hidden file (easier to find and manage) - Clear documentation of global URL configuration - Replaces IP address detection with stored domain URL This makes SWAG_URL the canonical source for the domain URL throughout the module, replacing IP-based fallbacks. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add global SWAG_URL variable that replaces LOCALIPADD in runtime menu URLs when SWAG reverse proxy is installed with a domain. Changes in module_env_init.sh: - Read SWAG_URL from /armbian/swag/config/SWAG_URL if it exists - SWAG_URL becomes a global variable available throughout armbian-config Changes in config.runtime.sh: - Add DISPLAY_URL variable that uses SWAG_URL if available, otherwise falls back to LOCALIPADD - Replace all http:// and https:// with in menu URLs Benefits: - When SWAG is installed with a domain (e.g., example.com), all software menu URLs show the actual domain instead of local IP - Users see https://example.com:port instead of https://192.168.1.100:port - More professional and accurate for domain-based deployments - Automatic fallback to IP when SWAG is not installed This makes the entire software menu system domain-aware when SWAG reverse proxy is configured. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add SWAG reverse proxy URL to the runtime menu configuration. Changes: - Add SWAG01 entry to WebHosting section - Display https:// (uses SWAG domain or falls back to IP) - SWAG shows in software menu with proper HTTPS URL When SWAG is installed with a domain, users will see: - https://example.com (if SWAG configured with example.com) - https://192.168.1.100 (fallback to local IP if no SWAG domain) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Automatically configure SWAG reverse proxy for Transmission when: - SWAG container is installed and running - SWAG has built-in proxy config for transmission (transmission.subfolder.conf) - Transmission container starts successfully Flow: 1. Transmission is installed and starts 2. Check if SWAG container exists 3. Read SWAG domain from SWAG_URL file (fallback to IP) 4. Enable SWAG's built-in transmission.subfolder.conf proxy config 5. Reload SWAG nginx to apply the proxy 6. Show user the HTTPS URL with credentials Result: - Transmission accessible via: https://test.armbian.com/transmission - Instead of: http://192.168.1.100:9091 - Uses SWAG's built-in proxy configuration (no custom config needed) - SSL/HTTPS provided by SWAG Let's Encrypt certificates This provides automatic secure access to Transmission through SWAG reverse proxy with proper domain URLs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Fix the SWAG proxy configuration to use the global SWAG_URL variable instead of reading from a file. Changes: - Remove file reading logic with problematic cat command - Use global SWAG_URL variable (set in module_env_init.sh) - Fallback to LOCALIPADD if SWAG_URL is not set This is cleaner and more reliable, using the established global variable pattern instead of file I/O. Fixes cat errors during transmission installation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Fix SWAG reverse proxy configuration to handle SWAG's sample files. SWAG provides proxy configs as .sample files that need to be copied before use. Updated transmission module to: 1. Check for transmission.subfolder.conf.sample file 2. Copy sample to actual config (transmission.subfolder.conf) if needed 3. Enable the proxy by creating .enabled file 4. Reload nginx to apply changes This fixes the issue where SWAG has sample configs but not actual configs. Also created comprehensive diagnostic script for testing proxy setup. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Update runtime config menu to display SWAG subfolder paths instead of port numbers when SWAG reverse proxy is enabled for a service. Changes: - Add get_service_url() helper function that checks: * If SWAG is running * If SWAG proxy is enabled for the service * Returns appropriate URL format - Update Transmission menu entry to use helper function: * With SWAG proxy: https://test.armbian.com/transmission * Without SWAG: http://192.168.1.100:9091 Logic: 1. SWAG not running → Show port (local access) 2. SWAG running, proxy disabled → Show port (local access) 3. SWAG running, proxy enabled → Show subfolder (HTTPS domain access) This provides user-friendly menu URLs that reflect the actual access method. Local installs remain port-based, SWAG-proxied installs show domain paths. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add service name to module_options so runtime config can determine which services have SWAG proxy configurations available. Changes: - Add [module_transmission,servicename]=transmission to module_options - Update config.runtime.sh to use module_options[module_transmission,servicename] instead of hardcoded transmission string This provides a standardized way for modules to declare their SWAG proxy service name, making it easier to: - Identify which services have SWAG proxy support - Add more services to SWAG proxy system - Maintain consistency across modules Pattern for other services: 1. Add [module_SERVICE,servicename]=servicename to module_options 2. Update config.runtime.sh to use 3. Ensure SWAG has corresponding proxy config file Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add docker_configure_swag_proxy() to module_docker_utils.sh * Takes servicename as parameter * Returns 0 on success, 1 on error, 2 when SWAG not available * Handles copying sample configs and enabling proxy automatically - Update transmission module to use new function * Reduces code from 28 lines to 1 line * Makes SWAG configuration reusable for all modules Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add servicename module option - Call docker_configure_swag_proxy() after container starts - Homepage will be automatically accessible via SWAG reverse proxy Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The TZ (timezone) variable is required for proper time display in the Homepage dashboard. This matches the configuration used in other modules like transmission, grafana, etc. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds HOMEPAGE_VAR_LOCALIPADD and HOMEPAGE_VAR_SWAG_URL environment
variables that can be used in Homepage YAML configuration files.
Usage in config files:
- http://{{HOMEPAGE_VAR_LOCALIPADD}}:9091
- https://{{HOMEPAGE_VAR_SWAG_URL}}/service
This prevents hardcoded localhost URLs in service configurations.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add servicename module option - Call docker_configure_swag_proxy() after container starts - Portainer will be automatically accessible via SWAG reverse proxy Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The SWAG proxy configuration files have hardcoded ports that need to match the actual service ports. Updated the function to: - Accept optional port parameter - Edit the upstream_port in the nginx config using sed - Pass correct ports from modules: * transmission: 9091 * homepage: 3000 (internal port) * portainer: 9000 This fixes the issue where SWAG proxy was pointing to wrong ports. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Two issues were causing the 502 error: 1. Missing network: Portainer wasn't on the lsio Docker network, so SWAG couldn't reach it by container name 2. Wrong port/protocol: Portainer Web UI uses port 9443 with HTTPS, not port 9000 (which is for Edge agent) Fixes: - Added --net=lsio to Portainer docker run command - Updated docker_configure_swag_proxy() to accept protocol parameter - Changed Portainer proxy config to use port 9443 and https protocol Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Two changes to fix Portainer URL in the menu: 1. config.runtime.sh: Updated Portainer menu URL to use HTTPS - With SWAG: https://SWAG_URL/portainer - Without SWAG: https://IP:9443 (not http://IP:9000) 2. module_portainer.sh: Changed port from 9000 to 9443 - Port 9000 is the Edge Agent port (not main UI) - Port 9443 is the Web UI port that users actually access This fixes the menu link to point to the correct Portainer Web UI. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add servicename module option - Call docker_configure_swag_proxy() with port 8080 (internal HTTP port) - Update menu URL to use get_service_url function for proper SWAG support - Netbox external port is 8222, internal port is 8080 When SWAG is enabled: https://SWAG_URL/netbox Without SWAG: http://IP:8222 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add servicename module option - Call docker_configure_swag_proxy() with port 8080 (internal HTTP port) - Update menu URL to use get_service_url function for proper SWAG support - Immich external port is 8077, internal port is 8080 When SWAG is enabled: https://SWAG_URL/immich Without SWAG: http://IP:8077 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
igorpecovnik
force-pushed
the
update/swag-module
branch
from
db04f68 to
dcd1c52
Compare
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
1 participant
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
Modernized the SWAG (Secure Web Application Gateway) module to follow current armbian-config patterns with improved error handling, user experience, and robustness.
Changes Made
1. Modern Dialog Interface
dialog --titlecalls with wrapper functions:dialog_inputbox()for URL and username inputdialog_passwordbox()for password entrydialog_yesno()for confirmationsdialog_msgbox()for error/success messagesdialog_infobox()for informational displays2. Enhanced Error Handling
3. Improved User Experience
4. Code Quality
swag_*prefix)Benefits
Testing
Related Files
🤖 Generated with Claude Code