Add support for dialog via new wrapper functions#762
Open
igorpecovnik wants to merge 28 commits intomainfrom
Open
Add support for dialog via new wrapper functions#762igorpecovnik wants to merge 28 commits intomainfrom
igorpecovnik wants to merge 28 commits intomainfrom
Conversation
github-actions
bot
added
size/large
Contributor
|
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:
WalkthroughAdds a dialog abstraction layer in tools/modules/functions/config_interface.sh introducing multiple wrapper functions (dialog_menu, dialog_inputbox, dialog_passwordbox, dialog_yesno, dialog_msgbox, dialog_infobox, dialog_gauge, dialog_checklist, etc.) and associated module metadata. Updates bin/armbian-config to only set DIALOG when it is empty. Replaces direct $DIALOG/whiptail invocations across many modules (network, software, system, images, etc.) to use the new wrappers, preserving messages and control flow while adjusting argument shapes and return handling at call sites. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
igorpecovnik
changed the title
![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: 14
🤖 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/config_interface.sh`:
- Around line 603-608: The read-fallback menu current validation allows
choice_index == i, but i is one past the last valid option; update the check in
the menu validation (the conditional that uses choice_index and i in the read
fallback logic in config_interface.sh) to ensure choice_index is strictly less
than i (or compare against i-1) instead of <= i so you never permit the
out-of-bounds index; keep the rest of the branches using use_no_items and
options as-is.
- Around line 523-529: Add a missing wrapper function named dialog_passwordbox
that mirrors the existing wrapper pattern used by dialog_menu, dialog_inputbox,
dialog_yesno, dialog_msgbox, dialog_infobox and dialog_gauge: implement
dialog_passwordbox() to accept the same arguments as the other wrappers, invoke
the configured dialog backend (using the same internal call pattern the other
wrappers use to run the dialog tool), capture and return the dialog output/exit
code consistently with the other wrappers, and add corresponding metadata
entries in module_options (["dialog_passwordbox,author"],
["dialog_passwordbox,desc"], ["dialog_passwordbox,example"],
["dialog_passwordbox,feature"], ["dialog_passwordbox,status"]) following the
style and content used for the dialog_menu block so callers in module_netbox.sh,
module_pi-hole.sh, module_network_config.sh and module_network_simple.sh will
find the function and metadata.
In `@tools/modules/network/module_network_config.sh`:
- Line 101: The dialog_menu invocation that sets SELECTED_SSID includes the flag
--backtitle "" which the wrapper misparses into an empty token; update the call
to dialog_menu (the SELECTED_SSID assignment) to remove the --backtitle ""
argument so only supported value-pair flags are passed to dialog_menu, leaving
the rest of the parameters/array ("${LIST[@]}") unchanged.
In `@tools/modules/network/module_network_simple.sh`:
- Line 65: Several places accidentally use nested command substitution like
$($(dialog_inputbox ...)) which tries to execute the dialog output as a command;
replace those with a single command substitution so the dialog output is
assigned directly (e.g., change $($(dialog_inputbox ...)) to $(dialog_inputbox
...)); update every occurrence that uses dialog_inputbox or dialog_passwordbox
(including the capture for mac_address and the other dialog captures referenced
in the comment) so variables receive the dialog output instead of being
executed.
- Line 222: The dialog_msgbox calls are passing a raw --title flag in positional
slots which breaks the wrapper; update both dialog_msgbox invocations (symbol:
dialog_msgbox) to remove the --title "Error" flag and instead pass the title as
the wrapper's positional title argument, making sure to also supply the expected
height/width positional arguments in their correct order (use the other
dialog_msgbox call patterns in the file as reference for the exact slots); in
short, replace the --title usage with the wrapper's positional height, width,
then title parameters so the dialog renders correctly.
In `@tools/modules/software/module_duplicati.sh`:
- Around line 49-50: The assignment uses nested command substitution which
executes user input; change the pattern from
DUPLICATI_ENCRYPTION_KEY=$($(dialog_inputbox ...)) to simply
DUPLICATI_ENCRYPTION_KEY=$(dialog_inputbox ...) (i.e., remove the extra $() so
the dialog_inputbox output is captured as a string, not executed). Update any
other occurrences that use $($(dialog_inputbox ...)) or similar nested $() for
passwords/keys (reference: DUPLICATI_ENCRYPTION_KEY and dialog_inputbox usages)
to the single-level substitution to avoid executing user-supplied content.
In `@tools/modules/software/module_images.sh`:
- Line 279: The dialog_menu wrapper cannot parse combined option+value tokens
like "--default-item STABLE", so change the call to pass the default as a
separate argument token: call dialog_menu with "--default-item" followed by
"STABLE" (i.e., ... "--default-item" "STABLE" -- "${options[@]}") so the parser
treats the option and its value distinctly; update the call site of dialog_menu
accordingly.
In `@tools/modules/software/module_pi-hole.sh`:
- Around line 117-120: Add a new dialog_passwordbox wrapper in
tools/modules/functions/config_interface.sh (modeled after dialog_inputbox)
named dialog_passwordbox that returns the entered string safely; then in
module_pi-hole.sh replace the unsafe docker exec -it "${container}" sh -c
"pihole setpassword ${SELECTED_PASSWORD}" with a safer invocation that does not
interpolate the password into a shell command—use the dialog wrapper to capture
SELECTED_PASSWORD, check [[ -n $SELECTED_PASSWORD ]], and call docker exec with
either a direct command (no sh -c) passing the password as an argument or feed
the password via stdin/pipe to the pihole setpassword command to avoid shell
interpretation and injection risks (operate on the existing SELECTED_PASSWORD
and container variables and update the lines that set SELECTED_PASSWORD and call
docker exec).
In `@tools/modules/software/module_swag.sh`:
- Around line 92-93: Replace the plain-text prompt that sets SWAG_PASSWORD via
dialog_inputbox with the masked/password dialog wrapper so the secret is not
echoed; call the project's password dialog function (e.g., dialog_passwordbox or
the existing password wrapper in the repo) instead of dialog_inputbox, passing
the same prompt text ("Enter new password for ${SWAG_USER}" and hint), the
generated default string (tr -dc ... | head -c 10) as the default value, and
capture the result into SWAG_PASSWORD exactly as before; ensure the function
signature matches the wrapper (height/width args if required) so behavior and
defaults remain unchanged.
In `@tools/modules/software/module_transmission.sh`:
- Around line 41-42: The credential prompts use unsafe double command
substitution—replace the outer $() so the result of dialog_inputbox is assigned
directly rather than executed; specifically change the lines that set
TRANSMISSION_USER and TRANSMISSION_PASS which currently use $($(dialog_inputbox
...)) to call dialog_inputbox and assign its output without the extra $(), and
apply the same fix to all other modules that use the pattern (e.g.,
module_sshserver.sh, module_nfs.sh, module_armbian_runners.sh,
module_pi-hole.sh, module_network_simple.sh, module_duplicati.sh) to eliminate
command injection risk while still capturing dialog_inputbox output.
In `@tools/modules/system/module_armbian_rsyncd.sh`:
- Around line 28-34: Update the dialog_checklist prompt and option labels in the
dialog_checklist invocation (the target_sync assignment) to reflect rsync
targets and fix typos: replace the unrelated prompt text "Select an
Option"/"Choose your favorite programming language" with something like "Select
rsync target" and a brief description about choosing which Armbian rsync target
to mirror; correct option strings "oldarhive" → "oldarchive" and "Nighly" →
"Nightly" (and "cache" description "Nighly and community images cache" →
"Nightly and community images cache"), ensuring the option descriptions for
"apt", "dl", "beta", "archive", "oldarchive", and "cache" are accurate and
user-facing.
In `@tools/modules/system/module_armbian_runners.sh`:
- Around line 41-65: The prompts use nested command substitution like
gh_token=$($(dialog_inputbox ...)) which executes user input; change each
occurrence to a single substitution and proper quoting (e.g.
gh_token="$(dialog_inputbox ...)" ) for variables shown (gh_token, runner_name,
start, stop, label_primary, label_secondary, organisation) and anywhere
dialog_inputbox is invoked; search other modules (module_transmission.sh,
module_sshserver.sh, module_nfs.sh, module_network_simple.sh,
module_duplicati.sh, module_pi-hole.sh) and replace var=$($(dialog_inputbox
...)) with var="$(dialog_inputbox ...)" to avoid executing user input.
In `@tools/modules/system/module_nfs.sh`:
- Around line 34-35: The subnet prompt is using nested command substitution with
$($(dialog_inputbox ...)) which tries to execute the user's input; change the
assignment to capture the dialog output as a string instead (use a single
command substitution and proper quoting) so variable subnet receives the literal
input from dialog_inputbox; update the local subnet assignment (referencing
dialog_inputbox and LOCALSUBNET) and ensure downstream usage in the LIST command
(nmap ... | grep ... | grep -v "${LOCALIPADD}") still works with the quoted
subnet variable.
In `@tools/modules/system/module_sshserver.sh`:
- Line 34: The assignment to USER_NAME uses an invalid nested command
substitution (USER_NAME=$($(dialog_inputbox ...))) which executes the input;
change it to capture the output directly by removing the outer $(), i.e. assign
the result of dialog_inputbox to USER_NAME (refer to the USER_NAME variable and
dialog_inputbox invocation in the file) to match the pattern used in the
adjacent lines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 66d53db0-5a6c-456f-8116-922576eaeefd
📒 Files selected for processing (27)
bin/armbian-configtools/modules/docs/config.docs.shtools/modules/functions/config_interface.shtools/modules/network/module_network_config.shtools/modules/network/module_network_simple.shtools/modules/software/module_cockpit.shtools/modules/software/module_duplicati.shtools/modules/software/module_haos.shtools/modules/software/module_images.shtools/modules/software/module_immich.shtools/modules/software/module_netbox.shtools/modules/software/module_pi-hole.shtools/modules/software/module_qbittorrent.shtools/modules/software/module_swag.shtools/modules/software/module_transmission.shtools/modules/software/module_wireguard.shtools/modules/system/adjust_motd.shtools/modules/system/change_system_hostname.shtools/modules/system/manage_dtoverlays.shtools/modules/system/manage_odroid_board.shtools/modules/system/module_armbian_firmware.shtools/modules/system/module_armbian_rsyncd.shtools/modules/system/module_armbian_runners.shtools/modules/system/module_armbian_upgrades.shtools/modules/system/module_nfs.shtools/modules/system/module_overlayfs.shtools/modules/system/module_sshserver.sh
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
tools/modules/functions/config_interface.sh (2)
603-608:⚠️ Potential issue | 🟠 MajorRead fallback still allows an out-of-range menu index.
Line 603 still accepts
choice_index == i, butiis already one past the last valid entry, so this can produce an invalid lookup.Minimal fix
- if [[ "$choice_index" =~ ^[0-9]+$ ]] && [ "$choice_index" -ge 1 ] && [ "$choice_index" -le "$i" ]; then + if [[ "$choice_index" =~ ^[0-9]+$ ]] && [ "$choice_index" -ge 1 ] && [ "$choice_index" -le "$((i-1))" ]; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/modules/functions/config_interface.sh` around lines 603 - 608, The input validation allows choice_index == i which is one past the last valid option; update the condition in the selection check (the if that tests choice_index, i, options and use_no_items) so it requires choice_index be <= i-1 (or use -lt "$i") instead of <= "$i" to prevent out-of-range access of the options array when echoing "${options[((choice_index-1))]}" or "${options[((choice_index-1)*2)]}". Ensure you only change the numeric bound check for choice_index and keep the rest of the logic (use_no_items, options indexing) intact.
658-675:⚠️ Potential issue | 🔴 Critical
dialog_passwordboxpositional parsing is incompatible with existing 5-arg callers.The function signature expects 4 positional arguments (title, prompt, height, width), but callers across the codebase invoke it with 5 arguments passing a default/initial value as the 3rd parameter:
module_network_config.sh:103, 128:dialog_passwordbox "title" "prompt" "" 7 50and with default"12345678"module_network_simple.sh:238:dialog_passwordbox "title" "" "" 7 50module_pi-hole.sh:117:dialog_passwordbox "title" "" "" 7 50This mismatch causes the function to interpret the intended default value as height, the height argument as width, and the width argument to be absorbed into
extra_args, corrupting the dialog/whiptail invocations.Compatibility fix (accept optional default/init argument)
dialog_passwordbox() { local title="$1" local prompt="$2" - local height="${3:-0}" - local width="${4:-80}" - shift 4 || true + local default="${3:-}" + local height="${4:-0}" + local width="${5:-80}" + shift 5 || true @@ case "$DIALOG" in "whiptail") - whiptail --title "$title" "${extra_args[@]}" --passwordbox "$prompt" $height $width 3>&1 1>&2 2>&3 + whiptail --title "$title" "${extra_args[@]}" --passwordbox "$prompt" $height $width "$default" 3>&1 1>&2 2>&3 ;; "dialog") # dialog outputs selection to stderr by default; swap stdout/stderr (3>&1 1>&2 2>&3) to capture stderr to stdout for command substitution - dialog --title "$title" "${extra_args[@]}" --passwordbox "$prompt" $height $width 3>&1 1>&2 2>&3 + dialog --title "$title" "${extra_args[@]}" --passwordbox "$prompt" $height $width "$default" 3>&1 1>&2 2>&3 ;; "read") # For read mode, use read -s to hide input read -s -p "$prompt: " result echo "" - echo "$result" + echo "${result:-$default}" ;; esac }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/modules/functions/config_interface.sh` around lines 658 - 675, The dialog_passwordbox function currently treats the 3rd positional as height which breaks callers that pass an optional initial/default value; update dialog_passwordbox to accept an optional init/default argument after prompt (e.g. title, prompt, init, height, width), set local init="${3:-}" height="${4:-0}" width="${5:-80}", adjust the shift to 5 (shift 5 || true) and keep collecting remaining args into extra_args, and ensure the dialog/whiptail calls pass the init/default where appropriate (using init when non-empty) so existing 5-argument callers work correctly; reference dialog_passwordbox and its case branches for where to apply the change.
🤖 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/config_interface.sh`:
- Around line 714-716: The read fallback currently only accepts "y" or "yes" and
ignores custom yes labels (variables yes_label/no_label); update the choice
check in config_interface.sh so it also accepts the displayed yes_label
case-insensitively. For example, after read -p "$prompt [$yes_label/$no_label]:
" choice, convert both choice and yes_label to lowercase and test if choice
equals "y" or "yes" or equals the lowercased yes_label (or use a
case-insensitive regex that includes yes_label) so that functions using custom
labels like "Reboot"/"Cancel" are recognized as affirmative when the user types
the shown yes label.
- Around line 132-133: The dialog_menu call currently masks its exit code by
assigning its output directly to a local variable; change the pattern in the
blocks using dialog_menu (where OPTION and exitstatus are used) to declare
locals first (local OPTION; local exitstatus) then assign OPTION="$(dialog_menu
...)" on the next line and immediately capture exitstatus=$? so the wrapped
command's exit status (e.g., Cancel => 1) is preserved; apply the same change to
the other occurrence(s) referenced (the block around OPTION/exitstatus and the
block around the lines ~165–167) and ensure subsequent logic checks exitstatus
rather than relying on the assignment returning 0.
---
Duplicate comments:
In `@tools/modules/functions/config_interface.sh`:
- Around line 603-608: The input validation allows choice_index == i which is
one past the last valid option; update the condition in the selection check (the
if that tests choice_index, i, options and use_no_items) so it requires
choice_index be <= i-1 (or use -lt "$i") instead of <= "$i" to prevent
out-of-range access of the options array when echoing
"${options[((choice_index-1))]}" or "${options[((choice_index-1)*2)]}". Ensure
you only change the numeric bound check for choice_index and keep the rest of
the logic (use_no_items, options indexing) intact.
- Around line 658-675: The dialog_passwordbox function currently treats the 3rd
positional as height which breaks callers that pass an optional initial/default
value; update dialog_passwordbox to accept an optional init/default argument
after prompt (e.g. title, prompt, init, height, width), set local init="${3:-}"
height="${4:-0}" width="${5:-80}", adjust the shift to 5 (shift 5 || true) and
keep collecting remaining args into extra_args, and ensure the dialog/whiptail
calls pass the init/default where appropriate (using init when non-empty) so
existing 5-argument callers work correctly; reference dialog_passwordbox and its
case branches for where to apply the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 45cd4962-6ddd-4f98-a435-0cd929646b67
📒 Files selected for processing (1)
tools/modules/functions/config_interface.sh
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/modules/functions/config_interface.sh (1)
346-350:⚠️ Potential issue | 🟠 MajorReturn dialog decision from
get_user_continue*, not callback status.These helpers currently return
$next_actionstatus. Callers that gate behavior with$?can mis-handle destructive paths because they don’t get the yes/no decision code.🔧 Proposed fix
function get_user_continue() { local message="$1" local next_action="$2" + local dialog_rc if dialog_yesno "" "$message" "Yes" "No" 15 80; then + dialog_rc=0 $next_action else + dialog_rc=$? $next_action "No" fi + return "$dialog_rc" } @@ function get_user_continue_secure() { @@ if [[ "$found" -eq 1 ]]; then + local dialog_rc if dialog_yesno "" "$message" "Yes" "No" 10 80; then + dialog_rc=0 $next_action else + dialog_rc=$? $next_action "No" fi + return "$dialog_rc" else echo "Error: Invalid function"Also applies to: 429-433
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/modules/functions/config_interface.sh` around lines 346 - 350, The helper functions get_user_continue* currently execute the callback ($next_action) and return its exit status, which hides the actual dialog decision; change them to capture the exit code from dialog_yesno (the yes/no decision), then invoke $next_action as a side effect but explicitly return the dialog_yesno result (0 for Yes, non-zero for No) so callers can inspect the user choice via $?. Locate the dialog invocation and callback call (dialog_yesno "" "$message" ... and $next_action) and update logic to save dialog_yesno's exit code into a variable, call $next_action (optionally passing "No"), and then return the saved dialog decision; apply the same change to the other identical block later in the file (the get_user_continue* occurrence around the other snippet).
♻️ Duplicate comments (2)
tools/modules/functions/config_interface.sh (2)
714-716:⚠️ Potential issue | 🟡 MinorRead-mode
dialog_yesnoshould honor custom yes-labels.Line 715 accepts only
y/yes, so prompts likeReboot/Cancelare not recognized as affirmative in read fallback.🔧 Proposed fix
"read") read -p "$prompt [$yes_label/$no_label]: " choice - [[ "$choice" =~ ^[Yy]$ ]] || [[ "$choice" =~ ^[Yy][Ee][Ss]$ ]] + local normalized_choice="${choice,,}" + local normalized_yes="${yes_label,,}" + [[ "$normalized_choice" == "$normalized_yes" ]] || [[ "$normalized_choice" == "y" ]] || [[ "$normalized_choice" == "yes" ]] ;;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/modules/functions/config_interface.sh` around lines 714 - 716, The read-mode fallback for dialog_yesno currently only accepts "y" or "yes" and ignores custom yes_label/no_label; update the logic where read -p "$prompt [$yes_label/$no_label]: " choice is used to instead normalize the user's input (trim and lowercase) and compare it against the lowercase yes_label and also against the first character of yes_label (lowercased) so labels like "Reboot" or "Cancel" are recognized as affirmative; adjust the conditional that currently tests [[ "$choice" =~ ^[Yy]$ ]] || [[ "$choice" =~ ^[Yy][Ee][Ss]$ ]] to use comparisons against "${yes_label,,}" and "${yes_label,,:0:1}" (or equivalent POSIX-safe case-insensitive checks) in the dialog_yesno branch.
132-133:⚠️ Potential issue | 🟠 MajorPreserve
dialog_menuexit codes when storing selection.Line 132 and Line 165 use
local OPTION=$(dialog_menu ...), which masks cancel/back status in Bash and can route control flow into the success branch.🔧 Proposed fix
- local OPTION=$(dialog_menu "$TITLE" "$status" 0 80 9 --ok-button Select --cancel-button Exit -- "${menu_options[@]}") - local exitstatus=$? + local OPTION + OPTION=$(dialog_menu "$TITLE" "$status" 0 80 9 --ok-button Select --cancel-button Exit -- "${menu_options[@]}") + local exitstatus=$? - local OPTION=$(dialog_menu "$top_parent_id $parent_id" "$status" 0 80 9 --ok-button Select --cancel-button Back -- "${submenu_options[@]}") - - local exitstatus=$? + local OPTION + OPTION=$(dialog_menu "$top_parent_id $parent_id" "$status" 0 80 9 --ok-button Select --cancel-button Back -- "${submenu_options[@]}") + local exitstatus=$?#!/bin/bash set -euo pipefail echo "== Occurrences of local assignment with dialog_menu ==" rg -n 'local\s+\w+\s*=\s*\$\(dialog_menu' tools/modules/functions/config_interface.sh echo echo "== Bash behavior check (local assignment masks command-substitution status) ==" bash -lc ' f() { local a=$(false); echo "local assignment status: $?" b=$(false); echo "plain assignment status: $?" } f 'Also applies to: 165-167
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/modules/functions/config_interface.sh` around lines 132 - 133, The current pattern "local OPTION=$(dialog_menu ...)" masks dialog_menu's exit code because the local builtin returns success; fix by declaring the variable first then invoking dialog_menu and capturing its status: replace occurrences of "local OPTION=$(dialog_menu ...)" with a two-step sequence like "local OPTION" followed by "OPTION=$(dialog_menu ...)" and then immediately capture "local exitstatus=$?" (or reuse exitstatus) so dialog_menu's cancel/back codes are preserved; apply the same change for the other occurrence around the same block that currently uses local assignment (both places referencing OPTION and exitstatus).
🤖 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/config_interface.sh`:
- Around line 658-663: The dialog_passwordbox function signature treats the
third argument as height but callers pass a default password as the third
parameter; update dialog_passwordbox (and the sibling dialog_inputbox at the
similar block) to accept parameters as (title, prompt, default,
height="${4:-0}", width="${5:-80}") and adjust the shift so that the default
value is preserved (e.g., shift 5 || true) and downstream usage uses the default
variable instead of treating it as height; also update any internal references
to use the new positional names (default, height, width) so existing callers
(like module_network_config.sh) continue to work without changing their call
sites.
- Around line 623-633: The dialog wrapper functions (e.g., dialog_inputbox)
currently use "shift 5 || true" then local extra_args=("$@"), which leaves $@
unchanged when fewer than 5 args are passed and causes earlier parameters to be
captured as extra args; replace the shift-based extraction with parameter
expansion to directly slice trailing args (for dialog_inputbox use local
extra_args=("${@:6}")), and apply the same pattern to the other dialog wrappers
referenced (the blocks at lines around 659-667, 701-705, 734-738, 767-771,
799-803) so each function uses the correct starting index (N+1) matching its
count of explicit positional parameters.
---
Outside diff comments:
In `@tools/modules/functions/config_interface.sh`:
- Around line 346-350: The helper functions get_user_continue* currently execute
the callback ($next_action) and return its exit status, which hides the actual
dialog decision; change them to capture the exit code from dialog_yesno (the
yes/no decision), then invoke $next_action as a side effect but explicitly
return the dialog_yesno result (0 for Yes, non-zero for No) so callers can
inspect the user choice via $?. Locate the dialog invocation and callback call
(dialog_yesno "" "$message" ... and $next_action) and update logic to save
dialog_yesno's exit code into a variable, call $next_action (optionally passing
"No"), and then return the saved dialog decision; apply the same change to the
other identical block later in the file (the get_user_continue* occurrence
around the other snippet).
---
Duplicate comments:
In `@tools/modules/functions/config_interface.sh`:
- Around line 714-716: The read-mode fallback for dialog_yesno currently only
accepts "y" or "yes" and ignores custom yes_label/no_label; update the logic
where read -p "$prompt [$yes_label/$no_label]: " choice is used to instead
normalize the user's input (trim and lowercase) and compare it against the
lowercase yes_label and also against the first character of yes_label
(lowercased) so labels like "Reboot" or "Cancel" are recognized as affirmative;
adjust the conditional that currently tests [[ "$choice" =~ ^[Yy]$ ]] || [[
"$choice" =~ ^[Yy][Ee][Ss]$ ]] to use comparisons against "${yes_label,,}" and
"${yes_label,,:0:1}" (or equivalent POSIX-safe case-insensitive checks) in the
dialog_yesno branch.
- Around line 132-133: The current pattern "local OPTION=$(dialog_menu ...)"
masks dialog_menu's exit code because the local builtin returns success; fix by
declaring the variable first then invoking dialog_menu and capturing its status:
replace occurrences of "local OPTION=$(dialog_menu ...)" with a two-step
sequence like "local OPTION" followed by "OPTION=$(dialog_menu ...)" and then
immediately capture "local exitstatus=$?" (or reuse exitstatus) so dialog_menu's
cancel/back codes are preserved; apply the same change for the other occurrence
around the same block that currently uses local assignment (both places
referencing OPTION and exitstatus).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0169d239-22f7-439c-ab35-c94d8a39e41d
📒 Files selected for processing (2)
tools/modules/functions/config_interface.shtools/modules/network/module_network_config.sh
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
tools/modules/functions/config_interface.sh (3)
132-133:⚠️ Potential issue | 🟠 MajorExit status masking:
local var=$(...)always yields$?=0.Combining declaration and assignment on one line makes
$?reflect the exit status oflocal(which is always 0), notdialog_menu. When the user presses Cancel (exit code 1), the condition$exitstatus = 0is true and the loop continues instead of exiting.🔧 Proposed fix
- local OPTION=$(dialog_menu "$TITLE" "$status" 0 80 9 --ok-button Select --cancel-button Exit -- "${menu_options[@]}") - local exitstatus=$? + local OPTION + OPTION=$(dialog_menu "$TITLE" "$status" 0 80 9 --ok-button Select --cancel-button Exit -- "${menu_options[@]}") + local exitstatus=$?Also applies to lines 165-167 where the same pattern is used.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/modules/functions/config_interface.sh` around lines 132 - 133, The issue is that combining declaration and assignment (local OPTION=$(dialog_menu ...)) masks the exit code of dialog_menu because local always returns 0; to fix, split declaration and command so the real exit status is captured: declare local OPTION and local exitstatus separately, then run OPTION=$(dialog_menu ...) and immediately capture exitstatus=$? (or local exitstatus=$? on the next line), and use that exitstatus for flow control; apply the same change for the other occurrence (the block around OPTION/exitstatus at lines 165-167).
714-716:⚠️ Potential issue | 🟡 MinorRead fallback ignores custom
yes_label.The read fallback only accepts
y/yes, but callers can specify custom labels like "Reboot"/"Cancel". Typing the displayed yes label will be treated as negative.🔧 Proposed fix
"read") read -p "$prompt [$yes_label/$no_label]: " choice - [[ "$choice" =~ ^[Yy]$ ]] || [[ "$choice" =~ ^[Yy][Ee][Ss]$ ]] + local choice_lower="${choice,,}" + local yes_lower="${yes_label,,}" + [[ "$choice_lower" == "y" ]] || [[ "$choice_lower" == "yes" ]] || [[ "$choice_lower" == "$yes_lower" ]] ;;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/modules/functions/config_interface.sh` around lines 714 - 716, The prompt handling currently only accepts "y"/"yes" regardless of caller-supplied yes_label/no_label: update the validation after read -p so it accepts the user-entered choice when it matches yes_label (case-insensitive) or the first character of yes_label (case-insensitive) in addition to the existing y/yes checks; locate the block using the variables prompt, yes_label, no_label and choice and replace the hardcoded regex with a comparison against yes_label (and/or its first character) to respect custom labels like "Reboot".
658-683:⚠️ Potential issue | 🟠 Major
dialog_passwordboxsignature mismatch with callers.The implementation treats arg3 as
height, but callers (e.g., in network modules) pass a default password value as arg3. This causes dimension corruption and breaks password defaulting.🔧 Proposed fix to align with caller expectations
dialog_passwordbox() { local title="$1" local prompt="$2" - local height="${3:-0}" - local width="${4:-80}" - shift 4 || true - - # Parse remaining arguments as extra args - local extra_args=("$@") + local default="${3:-}" + local height="${4:-0}" + local width="${5:-80}" + local extra_args=("${@:6}") case "$DIALOG" in "whiptail") - whiptail --title "$title" "${extra_args[@]}" --passwordbox "$prompt" $height $width 3>&1 1>&2 2>&3 + whiptail --title "$title" "${extra_args[@]}" --passwordbox "$prompt" $height $width "$default" 3>&1 1>&2 2>&3 ;; "dialog") - dialog --title "$title" "${extra_args[@]}" --insecure --passwordbox "$prompt" $height $width 3>&1 1>&2 2>&3 + dialog --title "$title" "${extra_args[@]}" --insecure --passwordbox "$prompt" $height $width "$default" 3>&1 1>&2 2>&3 ;; "read") read -s -p "$prompt: " result echo "" - echo "$result" + echo "${result:-$default}" ;; esac }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/modules/functions/config_interface.sh` around lines 658 - 683, The dialog_passwordbox function signature is misaligned with callers: treat the 3rd argument as the default password (not height) and shift subsequent args for height/width; update parameter parsing to: local default="${3:-}" local height="${4:-0}" local width="${5:-80}" and shift 5 || true, then pass that default into the passwordbox command invocation for whiptail/dialog (use the passwordbox initial value parameter) and, for "read" mode, prefill the prompt (e.g., print the default or use read -e -s with the default) so callers that pass a default password as arg3 work correctly; reference function name dialog_passwordbox and the whiptail/dialog/read branches when applying the change.
🧹 Nitpick comments (3)
test_dialog_abstraction.sh (3)
8-9: Fragile source path assumes script location.The source path
${SCRIPT_DIR}/tools/modules/...assumes the test script is at the repository root. If the script is moved or executed from a different location, sourcing will fail.♻️ Suggested improvement
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -source "${SCRIPT_DIR}/tools/modules/functions/config_interface.sh" +# Find repo root by looking for a known marker file +REPO_ROOT="$SCRIPT_DIR" +while [[ ! -f "$REPO_ROOT/bin/armbian-config" && "$REPO_ROOT" != "/" ]]; do + REPO_ROOT="$(dirname "$REPO_ROOT")" +done +source "${REPO_ROOT}/tools/modules/functions/config_interface.sh"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test_dialog_abstraction.sh` around lines 8 - 9, The current sourcing uses SCRIPT_DIR and assumes the repo root, which breaks if the script is moved; change the sourcing to compute a stable repo root and source from that. Specifically, derive REPO_ROOT using git rev-parse --show-toplevel (with a fallback to a sensible path based on SCRIPT_DIR) and then replace the hardcoded source of "${SCRIPT_DIR}/tools/modules/functions/config_interface.sh" with "${REPO_ROOT}/tools/modules/functions/config_interface.sh"; update the symbols SCRIPT_DIR and the source line to use REPO_ROOT so the script reliably finds config_interface.sh regardless of where it is run or moved.
137-151: Existence tests don't actually require DIALOG setting.The
type dialog_foocommand checks if a function exists regardless of what$DIALOGis set to. SettingDIALOG="whiptail"for these tests is misleading since it has no effect on the test outcome.Consider either:
- Removing the dialog_type parameter from existence tests
- Or adding actual behavior tests that exercise the wrapper with each DIALOG type
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test_dialog_abstraction.sh` around lines 137 - 151, The existence checks (test_inputbox_basic, test_menu_basic, test_yesno_basic, test_msgbox_basic, test_checklist_basic, test_radiolist_basic, test_passwordbox_basic, test_gauge_basic) are only verifying that a function exists via type dialog_* and do not depend on the DIALOG variable, so remove the misleading second argument ("whiptail") from the corresponding run_test calls (the calls invoking run_test "dialog_inputbox exists" "whiptail" test_inputbox_basic etc.) — either drop the dialog_type parameter entirely or pass an explicit empty/neutral value so the test description matches behavior; ensure you update each run_test invocation that references the existence checks (the eight test_*_basic names) to call run_test with only the description and test function.
92-131: Redirection tests don't validate actual wrapper behavior.The tests define mock inline functions that echo hardcoded strings, rather than testing the real
dialog_inputboxwrapper fromconfig_interface.sh. Additionally,test_read_fallback(lines 128-131) is identical totest_inputbox_basicand provides no additional coverage.Consider using
type -a dialog_inputboxor inspecting the function body to verify the real implementation contains the expected redirection pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test_dialog_abstraction.sh`:
- Around line 67-77: The tests call missing wrapper functions dialog_checklist
and dialog_radiolist (seen in test_checklist_basic and test_radiolist_basic) but
only wrappers in config_interface.sh are dialog_gauge, dialog_infobox,
dialog_inputbox, dialog_menu, dialog_msgbox, dialog_passwordbox, and
dialog_yesno; either remove/disable these two tests or add implementations for
dialog_checklist and dialog_radiolist in config_interface.sh matching the
pattern of the existing wrappers (exported functions that delegate to dialog
with appropriate options and parameters), then update the tests to reference the
correct wrapper names if you prefer existing functionality (e.g., map checklist
to dialog_menu or implement the specific checklist/radiolist behavior).
- Around line 1-40: The file uses space indentation but the project requires
tabs for shell scripts; convert all indented lines in this script (e.g., the
body lines of run_test, the TESTS_* variable assignments, the exported DIALOG
line, and all echo/if/then/else blocks) from space characters to tab characters
so the script conforms to indent_style=tab and indent_size=8 in .editorconfig;
ensure you only replace leading-space indentation (not spaces inside strings)
and preserve existing line content and spacing otherwise.
---
Duplicate comments:
In `@tools/modules/functions/config_interface.sh`:
- Around line 132-133: The issue is that combining declaration and assignment
(local OPTION=$(dialog_menu ...)) masks the exit code of dialog_menu because
local always returns 0; to fix, split declaration and command so the real exit
status is captured: declare local OPTION and local exitstatus separately, then
run OPTION=$(dialog_menu ...) and immediately capture exitstatus=$? (or local
exitstatus=$? on the next line), and use that exitstatus for flow control; apply
the same change for the other occurrence (the block around OPTION/exitstatus at
lines 165-167).
- Around line 714-716: The prompt handling currently only accepts "y"/"yes"
regardless of caller-supplied yes_label/no_label: update the validation after
read -p so it accepts the user-entered choice when it matches yes_label
(case-insensitive) or the first character of yes_label (case-insensitive) in
addition to the existing y/yes checks; locate the block using the variables
prompt, yes_label, no_label and choice and replace the hardcoded regex with a
comparison against yes_label (and/or its first character) to respect custom
labels like "Reboot".
- Around line 658-683: The dialog_passwordbox function signature is misaligned
with callers: treat the 3rd argument as the default password (not height) and
shift subsequent args for height/width; update parameter parsing to: local
default="${3:-}" local height="${4:-0}" local width="${5:-80}" and shift 5 ||
true, then pass that default into the passwordbox command invocation for
whiptail/dialog (use the passwordbox initial value parameter) and, for "read"
mode, prefill the prompt (e.g., print the default or use read -e -s with the
default) so callers that pass a default password as arg3 work correctly;
reference function name dialog_passwordbox and the whiptail/dialog/read branches
when applying the change.
---
Nitpick comments:
In `@test_dialog_abstraction.sh`:
- Around line 8-9: The current sourcing uses SCRIPT_DIR and assumes the repo
root, which breaks if the script is moved; change the sourcing to compute a
stable repo root and source from that. Specifically, derive REPO_ROOT using git
rev-parse --show-toplevel (with a fallback to a sensible path based on
SCRIPT_DIR) and then replace the hardcoded source of
"${SCRIPT_DIR}/tools/modules/functions/config_interface.sh" with
"${REPO_ROOT}/tools/modules/functions/config_interface.sh"; update the symbols
SCRIPT_DIR and the source line to use REPO_ROOT so the script reliably finds
config_interface.sh regardless of where it is run or moved.
- Around line 137-151: The existence checks (test_inputbox_basic,
test_menu_basic, test_yesno_basic, test_msgbox_basic, test_checklist_basic,
test_radiolist_basic, test_passwordbox_basic, test_gauge_basic) are only
verifying that a function exists via type dialog_* and do not depend on the
DIALOG variable, so remove the misleading second argument ("whiptail") from the
corresponding run_test calls (the calls invoking run_test "dialog_inputbox
exists" "whiptail" test_inputbox_basic etc.) — either drop the dialog_type
parameter entirely or pass an explicit empty/neutral value so the test
description matches behavior; ensure you update each run_test invocation that
references the existence checks (the eight test_*_basic names) to call run_test
with only the description and test function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ec6c8af7-5036-47ad-8c4c-d434e5fa1ebc
📒 Files selected for processing (3)
test_dialog_abstraction.shtools/modules/functions/config_interface.shtools/modules/network/module_network_simple.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/modules/network/module_network_simple.sh
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tools/modules/functions/config_interface.sh (1)
657-680:⚠️ Potential issue | 🟠 Major
dialog_passwordboxsignature inconsistent withdialog_inputboxand caller expectations.The
dialog_inputboxwrapper accepts(title, prompt, default, height, width, ...)whiledialog_passwordboxaccepts(title, prompt, height, width, ...). Callers may pass a default/initial value as arg3, which would be incorrectly interpreted as height.Consider aligning the signature with
dialog_inputbox:🔧 Suggested fix
dialog_passwordbox() { local title="$1" local prompt="$2" - local height="${3:-0}" - local width="${4:-80}" - local extra_args=("${@:5}") + local default="${3:-}" + local height="${4:-0}" + local width="${5:-80}" + local extra_args=("${@:6}") # Parse remaining arguments as extra args case "$DIALOG" in "whiptail") - whiptail --title "$title" "${extra_args[@]}" --passwordbox "$prompt" $height $width 3>&1 1>&2 2>&3 + whiptail --title "$title" "${extra_args[@]}" --passwordbox "$prompt" $height $width "$default" 3>&1 1>&2 2>&3 ;; "dialog") # dialog outputs selection to stderr by default; swap stdout/stderr (3>&1 1>&2 2>&3) to capture stderr to stdout for command substitution - dialog --title "$title" "${extra_args[@]}" --insecure --passwordbox "$prompt" $height $width 3>&1 1>&2 2>&3 + dialog --title "$title" "${extra_args[@]}" --insecure --passwordbox "$prompt" $height $width "$default" 3>&1 1>&2 2>&3 ;; "read") # For read mode, use read -s to hide input read -s -p "$prompt: " result echo "" - echo "$result" + echo "${result:-$default}" ;; esac },
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/modules/functions/config_interface.sh` around lines 657 - 680, dialog_passwordbox currently takes (title, prompt, height, width, ...) which is inconsistent with dialog_inputbox; change dialog_passwordbox to accept (title, prompt, default, height, width, ...) so callers passing an initial value as arg3 won't break — update the parameter parsing to set local title="$1", prompt="$2", default="${3:-}", height="${4:-0}", width="${5:-80}", and extra_args=("${@:6}"), and ensure the "read" branch respects the default (pre-fill or ignore securely) and other branches pass the same argument ordering (extra_args and height/width) to whiptail/dialog.
🤖 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/functions/config_interface.sh`:
- Around line 657-680: dialog_passwordbox currently takes (title, prompt,
height, width, ...) which is inconsistent with dialog_inputbox; change
dialog_passwordbox to accept (title, prompt, default, height, width, ...) so
callers passing an initial value as arg3 won't break — update the parameter
parsing to set local title="$1", prompt="$2", default="${3:-}",
height="${4:-0}", width="${5:-80}", and extra_args=("${@:6}"), and ensure the
"read" branch respects the default (pre-fill or ignore securely) and other
branches pass the same argument ordering (extra_args and height/width) to
whiptail/dialog.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2c85f908-0f59-4007-8c47-448370d8c6f4
📒 Files selected for processing (3)
tools/modules/functions/config_interface.shtools/modules/software/module_pi-hole.shtools/modules/system/module_armbian_rsyncd.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- tools/modules/software/module_pi-hole.sh
- tools/modules/system/module_armbian_rsyncd.sh
- Fix dialog_menu to properly parse button options (--ok-button, --cancel-button, etc.) - Add file descriptor redirection (3>&1 1>&2 2>&3) for dialog to capture stderr output - Migrate all direct DIALOG calls to use wrapper functions: - get_user_continue -> dialog_yesno - get_user_continue_secure -> dialog_yesno - show_menu -> dialog_menu - show_infobox -> dialog_infobox - show_message -> dialog_msgbox - info_wait_continue -> dialog_gauge (new wrapper) This fixes the issue where dialog menus would display but selections wouldn't be captured properly, causing submenus to not open. Dialog outputs selections to stderr by default, so we need to swap stdout/stderr for command substitution to capture the user's choice.
Continue the dialog abstraction migration by updating additional modules
to use the wrapper functions instead of direct DIALOG calls.
Modules updated:
- Network: module_network_config.sh, module_network_simple.sh
- Software: module_cockpit.sh, module_duplicati.sh, module_haos.sh,
module_immich.sh, module_netbox.sh, module_pi-hole.sh,
module_qbittorrent.sh, module_swag.sh, module_transmission.sh
- System: adjust_motd.sh, change_system_hostname.sh, manage_dtoverlays.sh,
manage_odroid_board.sh, module_armbian_rsyncd.sh,
module_armbian_runners.sh, module_armbian_upgrades.sh,
module_nfs.sh, module_overlayfs.sh, module_sshserver.sh
Also updates:
- bin/armbian-config: Add dialog tool detection
- tools/modules/docs/config.docs.sh: Minor documentation update
This ensures consistent dialog behavior across all modules and proper
output handling for both dialog and whiptail.
Add missing dialog_passwordbox wrapper function to support password input dialogs in modules like module_netbox.sh, module_pi-hole.sh, module_network_config.sh and module_network_simple.sh. Features: - Accepts standard wrapper arguments (title, prompt, height, width) - Supports whiptail, dialog, and read (console) modes - Uses file descriptor redirection (3>&1 1>&2 2>&3) for dialog - In read mode, uses 'read -s' to hide password input - Includes module_options metadata for discovery Follows the same pattern as other wrapper functions (dialog_menu, dialog_inputbox, etc.) for consistency.
Fix menu validation logic in the read fallback case to prevent out-of-bounds array access. The loop counter i is incremented after each iteration, so when the loop completes i is one past the last valid index. Changed validation from -le "$i" to -lt "$i" to ensure choice_index is strictly less than i (the count of items), preventing access to invalid array indices. Example: - 3 menu items: loop runs with i=1,2,3, then i becomes 4 - Valid indices: 1, 2, 3 - Old check: -le 4 allowed 4 (out of bounds) ❌ - New check: -lt 4 only allows 1, 2, 3 ✅
Remove --backtitle "" argument from dialog_menu call in WiFi network selection. The dialog wrapper only supports specific button flags (--ok-button, --cancel-button, --yes-button, --no-button) and misparses --backtitle, leaving it without its argument and placing the empty string in the options array. This fixes proper parsing of the WiFi network list dialog in module_network_config.sh.
Fixed multiple dialog-related issues in network configuration: 1. **dialog_inputbox missing height/width**: Added height=8, width=50 to all dialog_inputbox calls. Without these, height defaults to 0 which causes dialog to fail silently or output 'Box options:' error. 2. **dialog_passwordbox extra parameter**: Removed empty 3rd parameter that was causing 'Unknown option 50' error when entering WiFi password. Function signature: dialog_passwordbox title prompt height width (no default) 3. **Show asterisks in password field**: Added --insecure flag to dialog passwordbox so it shows asterisks (****) like whiptail does, instead of showing nothing while typing. Changes: - module_network_simple.sh: Fixed 6 dialog_inputbox calls and 1 dialog_passwordbox call - config_interface.sh: Added --insecure flag to dialog_passwordbox for dialog tool This fixes: 'after selecting DHCP/Static it goes back', 'Unknown option 50', and improves UX by showing visual feedback when typing passwords.
Fixed security bug where nested command substitution would execute user input as a command instead of capturing it as a string. The pattern has two levels of command substitution: - Inner captures the dialog output - Outer tries to execute that output as a command This meant any text entered by the user would be executed as shell code, which is a command injection vulnerability. Fixed files: - module_duplicati.sh: DUPLICATI_ENCRYPTION_KEY, DUPLICATI_WEBUI_PASSWORD - module_transmission.sh: TRANSMISSION_USER, TRANSMISSION_PASS - module_armbian_runners.sh: gh_token, runner_name, start, stop, labels, organisation - module_sshserver.sh: USER_NAME - module_nfs.sh: subnet Changed from: VAR= Changed to: VAR= All files pass syntax checks after the fix.
Fixed bug where dialog_menu exit codes were being masked by using 'local VAR=' pattern, which always returns exit code 0 (the success of the local declaration), not the exit code of the command. Problem: local OPTION= # 127 captures exit status of 'local', not dialog_menu local exitstatus=127 # Always 0, even if user pressed Cancel/ESC Solution: local OPTION # Declare first local exitstatus # Declare first OPTION= # Assign on separate line exitstatus=127 # Now captures actual dialog_menu exit status Fixed occurrences: - generate_top_menu() line 132: Main menu dialog - generate_menu() line 165: Submenu dialog This ensures that: - Pressing ESC or Cancel in menus is properly detected (exitstatus=1) - The menu navigation works correctly - Exit code handling matches user expectations All files pass syntax checks after the fix.
Fixed bug where dialog wrapper functions used 'shift N || true' followed by
'local extra_args=($@)', which incorrectly captured positional parameters
as extra_args when fewer than N arguments were passed.
Problem:
local title="$1"
local prompt="$2"
shift 5 || true # Does nothing if < 5 args passed
local extra_args=($@) # Still contains all original params!
Solution:
local extra_args=("${@:6}") # Directly slice from position 6 onwards
This uses Bash parameter expansion to extract arguments starting at
position N, which correctly skips the first N-1 positional parameters.
Fixed functions:
- dialog_inputbox: 5 params, uses ${@:6}
- dialog_passwordbox: 4 params, uses ${@:5}
- dialog_yesno: 6 params, uses ${@:7}
- dialog_msgbox: 4 params, uses ${@:5}
- dialog_infobox: 4 params, uses ${@:5}
- dialog_gauge: 4 params, uses ${@:5}
All files pass syntax checks after the fix.
…eclaration" This reverts commit 15f3dbe.
Fixed bug where the read fallback in dialog_yesno only accepted 'y' or 'yes' as affirmative responses, even when custom yes_label was displayed to the user. Problem: read -p "$prompt [$yes_label/$no_label]: " choice [[ "$choice" =~ ^[Yy]$ ]] || [[ "$choice" =~ ^[Yy][Ee][Ss]$ ]] # Shows '[Reboot/Cancel]' but only accepts 'y' or 'yes' - confusing\! Solution: local choice_lower="$choice" local yes_label_lower="$yes_label" [[ "$choice_lower" == "y" ]] || [[ "$choice_lower" == "yes" ]] || [[ "$choice_lower" == "$yes_label_lower" ]] Now the read fallback accepts: - 'y' or 'Y' (standard) - 'yes' or 'YES' (standard) - The custom yes_label case-insensitively (e.g., 'reboot', 'REBOOT', 'Reboot') This makes the read fallback behavior consistent with what's displayed to the user. File: tools/modules/functions/config_interface.sh dialog_yesno function
Fixed incorrect prompt text and multiple typos in the rsyncd configuration dialog. Changes: - Prompt: "Choose your favorite programming language" → "Choose which Armbian rsync targets to mirror" - Title: "Select an Option" → "Select rsync target" - Option name: "oldarhive" → "oldarchive" (spelling error) - Description: "Very old Archive" → "Very old archive" (capitalization) - Description: "Nighly and community images cache" → "Nightly and community images cache" (spelling error) These changes make the dialog accurate and user-facing for rsync target selection. File: tools/modules/system/module_armbian_rsyncd.sh
Fixed bug where --default-item option was not recognized by the dialog_menu parser, causing the default value to be treated as a menu item instead. The parser only handled specific options that require value arguments: - --ok-button - --cancel-button - --yes-button - --no-button Added --default-item to this list so it properly consumes both the flag and its value as a single option pair. Example: Before: --default-item STABLE was parsed as flag only, 'STABLE' became data After: --default-item STABLE is correctly parsed as flag+value pair This fixes the prebuild images menu default selection in module_images.sh. File: tools/modules/functions/config_interface.sh dialog_menu function
Fixed security vulnerability where the Pi-hole admin password was being
interpolated into a shell command string, allowing potential command injection.
Changes:
1. Removed nested command substitution: $((dialog_passwordbox ...)) → $(dialog_passwordbox ...)
- Was executing dialog output as a command instead of capturing it
2. Removed unsafe sh -c wrapper: docker exec ... sh -c "pihole setpassword ${password}"
→ docker exec ... pihole setpassword "${password}"
- Was interpolating password into shell string (command injection risk)
- Now passes password as quoted argument directly to pihole command
This prevents malicious input like:
- '; rm -rf /' (previously would execute shell commands)
- $(malicious_command) (previously would execute command substitution)
- (previously would execute backtick commands)
File: tools/modules/software/module_pi-hole.sh
Replace dialog_inputbox with dialog_passwordbox for SWAG_PASSWORD to prevent password from being echoed in plain text. Pre-generates the default password and presents it in a confirmation dialog; if user declines, opens a masked password input dialog for custom entry.
igorpecovnik
force-pushed
the
fix-dialog-wrapper-functions
branch
from
9d7ecac to
73284cd
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
tools/modules/functions/config_interface.sh (2)
662-667:⚠️ Potential issue | 🟠 MajorAlign
dialog_passwordboxparameter slicing with migrated callers.Current signature interprets arg3 as height. Callers like
module_pi_holepass(title, prompt, default, height, width), so dimensions are misparsed and an extra positional token is injected.Proposed fix
dialog_passwordbox() { local title="$1" local prompt="$2" - local height="${3:-0}" - local width="${4:-80}" - local extra_args=("${@:5}") + local default="${3:-}" + local height="${4:-0}" + local width="${5:-80}" + local extra_args=("${@:6}") @@ - whiptail --title "$title" "${extra_args[@]}" --passwordbox "$prompt" $height $width 3>&1 1>&2 2>&3 + whiptail --title "$title" "${extra_args[@]}" --passwordbox "$prompt" $height $width "$default" 3>&1 1>&2 2>&3 @@ - dialog --title "$title" "${extra_args[@]}" --insecure --passwordbox "$prompt" $height $width 3>&1 1>&2 2>&3 + dialog --title "$title" "${extra_args[@]}" --insecure --passwordbox "$prompt" $height $width "$default" 3>&1 1>&2 2>&3 @@ - echo "$result" + echo "${result:-$default}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/modules/functions/config_interface.sh` around lines 662 - 667, The dialog_passwordbox parameter parsing is off: callers pass (title, prompt, default, height, width) but the function currently treats arg3 as height and slices extra args from 5; update dialog_passwordbox to treat arg3 as default, arg4 as height, arg5 as width, and collect any extra args starting at position 6 so dimensions and extra positional tokens are correctly assigned (refer to dialog_passwordbox's parameter handling and extra_args assignment).
137-138:⚠️ Potential issue | 🔴 CriticalPreserve
dialog_menuexit status by splitting declaration and assignment.On Line 137 and Line 170,
local OPTION=$(...)masks cancel/back exit codes, so menu flow can incorrectly continue as success.Proposed fix
- local OPTION=$(dialog_menu "$TITLE" "$status" 0 80 9 --ok-button Select --cancel-button Exit -- "${menu_options[@]}") - local exitstatus=$? + local OPTION + OPTION=$(dialog_menu "$TITLE" "$status" 0 80 9 --ok-button Select --cancel-button Exit -- "${menu_options[@]}") + local exitstatus=$? @@ - local OPTION=$(dialog_menu "$top_parent_id $parent_id" "$status" 0 80 9 --ok-button Select --cancel-button Back -- "${submenu_options[@]}") - - local exitstatus=$? + local OPTION + OPTION=$(dialog_menu "$top_parent_id $parent_id" "$status" 0 80 9 --ok-button Select --cancel-button Back -- "${submenu_options[@]}") + local exitstatus=$?#!/bin/bash rg -n 'local\s+OPTION=\$\(dialog_menu' tools/modules/functions/config_interface.sh -C2Also applies to: 170-173
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/modules/functions/config_interface.sh` around lines 137 - 138, The issue is that using "local OPTION=$(dialog_menu ...)" masks the dialog_menu exit code; split the declaration and assignment so you declare the local variable first (local OPTION) then call dialog_menu and assign its output to OPTION (OPTION=$(dialog_menu ...)) and immediately capture its exit status into exitstatus (local exitstatus=$?) — apply the same change for the other occurrence around the block that references OPTION and exitstatus (the second instance near line 170) so dialog_menu cancel/back codes are preserved.tools/modules/network/module_network_simple.sh (1)
225-225:⚠️ Potential issue | 🟠 MajorUse wrapper positional args for
dialog_msgbox(not--titleflags).These two calls pass raw CLI flags where wrapper positional parameters are expected, which can break rendering/exit behavior.
🔧 Proposed fix
- dialog_msgbox "Message" "SSID must be 1-32 characters." --title "Error" + dialog_msgbox "Error" "SSID must be 1-32 characters." 8 60 ... - dialog_msgbox "Message" "Passphrase must be between 8 and 63 characters!" --title "Error" + dialog_msgbox "Error" "Passphrase must be between 8 and 63 characters!" 8 60#!/bin/bash # Verify dialog_msgbox wrapper signature and invalid --title usage in this module. rg -nP --type sh '^dialog_msgbox\(\)\s*\{' tools/modules/functions/config_interface.sh -A20 rg -nP --type sh 'dialog_msgbox .*--title' tools/modules/network/module_network_simple.sh -C2Expected result:
- Wrapper signature shows title/message/height/width positional parameters.
--title-style callsites are identified for correction.Also applies to: 245-245
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/modules/network/module_network_simple.sh` at line 225, The call to dialog_msgbox is using a raw --title flag instead of the wrapper's positional signature; update the dialog_msgbox invocations in module_network_simple (the lines using "dialog_msgbox \"Message\" ... --title \"Error\"") to pass the title as the first positional argument and the message as the second (and height/width as third/fourth if required) to match the dialog_msgbox wrapper function signature (verify dialog_msgbox definition in config_interface.sh) — fix both occurrences (the one around the shown diff and the other at the indicated duplicate) to remove the --title flag and supply title/message via positional parameters.
🤖 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/functions/config_interface.sh`:
- Around line 662-667: The dialog_passwordbox parameter parsing is off: callers
pass (title, prompt, default, height, width) but the function currently treats
arg3 as height and slices extra args from 5; update dialog_passwordbox to treat
arg3 as default, arg4 as height, arg5 as width, and collect any extra args
starting at position 6 so dimensions and extra positional tokens are correctly
assigned (refer to dialog_passwordbox's parameter handling and extra_args
assignment).
- Around line 137-138: The issue is that using "local OPTION=$(dialog_menu ...)"
masks the dialog_menu exit code; split the declaration and assignment so you
declare the local variable first (local OPTION) then call dialog_menu and assign
its output to OPTION (OPTION=$(dialog_menu ...)) and immediately capture its
exit status into exitstatus (local exitstatus=$?) — apply the same change for
the other occurrence around the block that references OPTION and exitstatus (the
second instance near line 170) so dialog_menu cancel/back codes are preserved.
In `@tools/modules/network/module_network_simple.sh`:
- Line 225: The call to dialog_msgbox is using a raw --title flag instead of the
wrapper's positional signature; update the dialog_msgbox invocations in
module_network_simple (the lines using "dialog_msgbox \"Message\" ... --title
\"Error\"") to pass the title as the first positional argument and the message
as the second (and height/width as third/fourth if required) to match the
dialog_msgbox wrapper function signature (verify dialog_msgbox definition in
config_interface.sh) — fix both occurrences (the one around the shown diff and
the other at the indicated duplicate) to remove the --title flag and supply
title/message via positional parameters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ecca6ae8-7974-4138-9dda-dca4af15df27
📒 Files selected for processing (27)
bin/armbian-configtools/modules/docs/config.docs.shtools/modules/functions/config_interface.shtools/modules/network/module_network_config.shtools/modules/network/module_network_simple.shtools/modules/software/module_cockpit.shtools/modules/software/module_duplicati.shtools/modules/software/module_haos.shtools/modules/software/module_images.shtools/modules/software/module_immich.shtools/modules/software/module_netbox.shtools/modules/software/module_pi-hole.shtools/modules/software/module_qbittorrent.shtools/modules/software/module_swag.shtools/modules/software/module_transmission.shtools/modules/software/module_wireguard.shtools/modules/system/adjust_motd.shtools/modules/system/change_system_hostname.shtools/modules/system/manage_dtoverlays.shtools/modules/system/manage_odroid_board.shtools/modules/system/module_armbian_firmware.shtools/modules/system/module_armbian_rsyncd.shtools/modules/system/module_armbian_runners.shtools/modules/system/module_armbian_upgrades.shtools/modules/system/module_nfs.shtools/modules/system/module_overlayfs.shtools/modules/system/module_sshserver.sh
🚧 Files skipped from review as they are similar to previous changes (11)
- bin/armbian-config
- tools/modules/software/module_swag.sh
- tools/modules/system/module_armbian_runners.sh
- tools/modules/system/change_system_hostname.sh
- tools/modules/software/module_wireguard.sh
- tools/modules/software/module_transmission.sh
- tools/modules/system/manage_odroid_board.sh
- tools/modules/system/module_nfs.sh
- tools/modules/software/module_cockpit.sh
- tools/modules/software/module_netbox.sh
- tools/modules/system/adjust_motd.sh
Added dialog_checklist wrapper function to config_interface.sh to fix the adjust_motd module and other modules that use checklists. The dialog_checklist function was being called directly in multiple modules without a wrapper, causing them to fail when using dialog/whiptail modes. New function features: - Title, prompt, height, width, list_height parameters - Properly parses value flags: --ok-button, --cancel-button, --yes-button, --no-button - Properly parses boolean flags: --separate-output, --nocancel - Supports triplets: tag description on/off (e.g., "apt" "Packages" ON) - Proper file descriptor redirection for dialog (3>&1 1>&2 2>&3) - Read fallback with numbered list selection Fixed bug where boolean flags were incorrectly treated as value flags, which would consume the next checklist item as the flag's argument value. Modules using dialog_checklist: - adjust_motd.sh: MOTD component selection (clear, header, sysinfo, tips, etc.) - module_armbian_rsyncd.sh: Rsync target selection - module_armbian_upgrades.sh: Upgrade options - manage_dtoverlays.sh: Device tree overlay management This fixes 'adjust motd does not work' issue.
Fixed double slash in rsyncd path configuration where the default path ended with a trailing slash, resulting in paths like: /armbian/openssh-server/storage//apt Changed default path from '/armbian/openssh-server/storage/' to '/armbian/openssh-server/storage' so when concatenated with '/' it produces correct paths like '/armbian/openssh-server/storage/apt'. File: tools/modules/system/module_armbian_rsyncd.sh
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
tools/modules/functions/config_interface.sh (2)
663-667:⚠️ Potential issue | 🟠 Major
dialog_passwordboxlikely regressed caller compatibility by dropping the default-value parameter.The wrapper currently treats arg3 as height. That can misparse callers that pass
(title, prompt, default, height, width)and produce invalid dimensions.🔧 Compatibility-aligned fix
dialog_passwordbox() { local title="$1" local prompt="$2" - local height="${3:-0}" - local width="${4:-80}" - local extra_args=("${@:5}") + local default="${3:-}" + local height="${4:-0}" + local width="${5:-80}" + local extra_args=("${@:6}") @@ "whiptail") - whiptail --title "$title" "${extra_args[@]}" --passwordbox "$prompt" $height $width 3>&1 1>&2 2>&3 + whiptail --title "$title" "${extra_args[@]}" --passwordbox "$prompt" $height $width "$default" 3>&1 1>&2 2>&3 ;; "dialog") - dialog --title "$title" "${extra_args[@]}" --insecure --passwordbox "$prompt" $height $width 3>&1 1>&2 2>&3 + dialog --title "$title" "${extra_args[@]}" --insecure --passwordbox "$prompt" $height $width "$default" 3>&1 1>&2 2>&3 ;; @@ - echo "$result" + echo "${result:-$default}" ;; esac }#!/bin/bash # Verify real call signatures used by modules rg -nP '\bdialog_passwordbox\s*\(' tools/modules --type sh -C1Also applies to: 672-683
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/modules/functions/config_interface.sh` around lines 663 - 667, The dialog_passwordbox wrapper now treats the 3rd parameter as height, breaking callers that pass (title, prompt, default, height, width); update dialog_passwordbox to accept an optional default as the 3rd arg: detect whether $3 is a numeric height (regex like ^[0-9]+$) or a non-numeric default string, then assign variables accordingly (set default when $3 is non-numeric, otherwise set height from $3); adjust parsing of width and extra_args (the array extra_args=("${@:5}") must be shifted when default is present) so callers with or without default continue to work, and apply the same fix to the related parsing logic in the sibling block handling the alternative flow.
137-138:⚠️ Potential issue | 🟠 MajorPreserve
dialog_menuexit codes by avoidinglocal OPTION=$(...).On Line 137 and Line 170,
local OPTION=$(dialog_menu ...)can mask the wrapped command’s non-zero status, so Cancel/Back may be treated as success.🔧 Minimal fix
- local OPTION=$(dialog_menu "$TITLE" "$status" 0 80 9 --ok-button Select --cancel-button Exit -- "${menu_options[@]}") - local exitstatus=$? + local OPTION + OPTION=$(dialog_menu "$TITLE" "$status" 0 80 9 --ok-button Select --cancel-button Exit -- "${menu_options[@]}") + local exitstatus=$?- local OPTION=$(dialog_menu "$top_parent_id $parent_id" "$status" 0 80 9 --ok-button Select --cancel-button Back -- "${submenu_options[@]}") - - local exitstatus=$? + local OPTION + OPTION=$(dialog_menu "$top_parent_id $parent_id" "$status" 0 80 9 --ok-button Select --cancel-button Back -- "${submenu_options[@]}") + local exitstatus=$?#!/bin/bash # Verify masked-exit-status pattern still exists in this file rg -nP 'local\s+\w+\s*=\s*\$\(dialog_menu\b' tools/modules/functions/config_interface.sh sed -n '130,176p' tools/modules/functions/config_interface.shAlso applies to: 170-173
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/modules/functions/config_interface.sh` around lines 137 - 138, The issue is that using "local OPTION=$(dialog_menu ...)" masks the dialog_menu exit status because "local" runs as a separate command; to fix, split declaration from assignment so the command's exit code is preserved: declare local OPTION (and local exitstatus if desired) first (e.g., local OPTION), then run OPTION=$(dialog_menu ...) immediately after and capture the exit code with exitstatus=$?; apply the same change for both occurrences that use "local OPTION=$(dialog_menu ...)" so dialog_menu's Cancel/Back return codes are handled correctly.
🤖 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/functions/config_interface.sh`:
- Around line 663-667: The dialog_passwordbox wrapper now treats the 3rd
parameter as height, breaking callers that pass (title, prompt, default, height,
width); update dialog_passwordbox to accept an optional default as the 3rd arg:
detect whether $3 is a numeric height (regex like ^[0-9]+$) or a non-numeric
default string, then assign variables accordingly (set default when $3 is
non-numeric, otherwise set height from $3); adjust parsing of width and
extra_args (the array extra_args=("${@:5}") must be shifted when default is
present) so callers with or without default continue to work, and apply the same
fix to the related parsing logic in the sibling block handling the alternative
flow.
- Around line 137-138: The issue is that using "local OPTION=$(dialog_menu ...)"
masks the dialog_menu exit status because "local" runs as a separate command; to
fix, split declaration from assignment so the command's exit code is preserved:
declare local OPTION (and local exitstatus if desired) first (e.g., local
OPTION), then run OPTION=$(dialog_menu ...) immediately after and capture the
exit code with exitstatus=$?; apply the same change for both occurrences that
use "local OPTION=$(dialog_menu ...)" so dialog_menu's Cancel/Back return codes
are handled correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 24b54857-128e-4447-b970-116abebe0eda
📒 Files selected for processing (2)
tools/modules/functions/config_interface.shtools/modules/system/adjust_motd.sh
- Use find with -mindepth 2 -maxdepth 2 to locate peer configs in subdirectories - Use null-delimited find output with proper IFS handling to preserve spaces - Store peer names as single array elements for --no-items dialog - Prevents word-splitting that caused names like 'PEER men,' to be parsed as options
--no-items causes issues with whiptail. By building LIST with tag/description pairs (each peer added twice), both dialog and whiptail display correctly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tools/modules/software/module_wireguard.sh (1)
258-265: Consider handling empty peer list gracefully.If no
peer_*.conffiles exist, the dialog will show an empty menu. Consider adding a check to inform the user when no peers are available.💡 Suggested improvement
local LIST=() while IFS= read -r -d '' peer_conf; do peer="${peer_conf#peer_}" peer="${peer%.conf}" [[ -n "$peer" ]] && LIST+=("$peer" "$peer") done < <(find "${WIREGUARD_BASE}/config/" -mindepth 2 -maxdepth 2 -name "peer_*.conf" -type f -printf "%f\0") + if [[ ${`#LIST`[@]} -eq 0 ]]; then + echo "No peers found in ${WIREGUARD_BASE}/config/" + return 1 + fi local LIST_LENGTH=$((${`#LIST`[@]} / 2))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/modules/software/module_wireguard.sh` around lines 258 - 265, The peer-selection code builds LIST and computes LIST_LENGTH then calls dialog_menu (creating SELECTED_PEER) without handling the case where no peer_*.conf files were found; add a guard after computing LIST_LENGTH to check if LIST_LENGTH is zero and, if so, show a clear message to the user (e.g., via dialog/info or echo) and return or exit the function early instead of calling dialog_menu. Update the block around LIST, LIST_LENGTH, and SELECTED_PEER so dialog_menu is only invoked when LIST_LENGTH > 0.
🤖 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_wireguard.sh`:
- Around line 257-266: The code builds LIST and calls dialog_menu when $2 is
empty but never sets SELECTED_PEER when a peer name is passed in; add an else
branch to assign SELECTED_PEER="$2" (and optionally validate existence of
"${WIREGUARD_BASE}/config/peer_${SELECTED_PEER}.conf" or ensure non-empty) so
subsequent checks like if [[ -n ${SELECTED_PEER} ]] and downstream logic in the
module_wireguard.sh routine work when the peer name is provided directly; keep
the existing dialog_menu path unchanged and only set SELECTED_PEER in the else
for direct-argument handling.
---
Nitpick comments:
In `@tools/modules/software/module_wireguard.sh`:
- Around line 258-265: The peer-selection code builds LIST and computes
LIST_LENGTH then calls dialog_menu (creating SELECTED_PEER) without handling the
case where no peer_*.conf files were found; add a guard after computing
LIST_LENGTH to check if LIST_LENGTH is zero and, if so, show a clear message to
the user (e.g., via dialog/info or echo) and return or exit the function early
instead of calling dialog_menu. Update the block around LIST, LIST_LENGTH, and
SELECTED_PEER so dialog_menu is only invoked when LIST_LENGTH > 0.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 357d7cef-0ef4-42c2-8962-3aa7b4fbac7c
📒 Files selected for processing (2)
tools/modules/software/module_wireguard.shtools/modules/system/module_armbian_rsyncd.sh
When calling module_wireguard qrcode <peer_name>, SELECTED_PEER was never set if a peer name was provided directly. Added else branch to assign SELECTED_PEER from so the subsequent logic works for both interactive selection and direct argument passing.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_wireguard.sh`:
- Around line 189-192: The check currently tests [[ -z $2 ]] then immediately
tests $? which reads the exit code of the -z test instead of the dialog result
and also never assigns NUMBER_OF_PEERS when $2 is provided; fix by assigning
NUMBER_OF_PEERS="$2" when $2 is present and by capturing the dialog exit status
into a separate variable (e.g., dialog_rc) when calling dialog_inputbox inside
the if branch, then use that dialog_rc (or assume success for the $2 branch) in
the subsequent if [[ $dialog_rc -eq 0 ]] block to decide whether to proceed with
the docker setup (refer to NUMBER_OF_PEERS and the dialog_inputbox invocation in
module_wireguard.sh).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b1c394b8-0136-495e-8646-d0de120f4d88
📒 Files selected for processing (1)
tools/modules/software/module_wireguard.sh
…rgument - Capture dialog_inputbox exit status into dialog_rc variable - Assign NUMBER_OF_PEERS from when provided as argument - Ensure subsequent logic works for both interactive and direct argument modes
…odule - Fix sanitize() regex to allow hyphens in module names (fixes module_openssh-server) - Refactor openssh-server module to match netdata module structure: - Add proper Docker availability check at function start - Use docker --format filters for reliable container/image detection - Change exit 1 to return 1 for proper function behavior - Add error handling with || true for docker commands - Improve waiting loop to check State.Status - Add purge command to help output
Add container_type field to openssh-server and watchtower modules to indicate they use Docker containers. This enables proper display and filtering of container-based software in the UI, consistent with the wireguard module.
Avahi-daemon is a native system package installed via pkg_install, not a Docker container. Remove the incorrect container_type field to fix documentation.
Remove container_type field from iperf3, iptraf-ng, and nload which are native system packages installed via pkg_install, not Docker containers. These packages were incorrectly displaying Docker icons in documentation and UI. The container_type field should only be used for actual Docker-based modules.
The badge format expects background and foreground colors in the correct order for proper rendering. Signed-off-by: Igor Pecovnik <igor@armbian.com>
igorpecovnik
force-pushed
the
fix-dialog-wrapper-functions
branch
from
e5280f2 to
409192c
Compare
Extract duplicated container startup wait loop pattern into a reusable function to reduce code duplication and improve maintainability. Changes: - Add wait_for_container_ready() function to config_interface.sh - Supports flexible readiness checks (build_version or running state) - Supports optional extra conditions for complex scenarios - Replace duplicated loops in 7 modules (swag, pi-hole, wireguard, duplicati, qbittorrent, transmission, sshserver) - Net reduction of 27 lines of code Benefits: - Single source of truth for container wait logic - Easier to modify timeout behavior globally - Consistent error messages across all modules - More maintainable codebase
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
This PR brings significant improvements to dialog handling, container metadata, security, and code quality across the Armbian config tooling.
📊 Statistics
🔧 Dialog Wrapper Functions (13 commits)
Core Improvements
dialog_menu- previously lost due to variable scoping issues--default-itemand other dialog optionsdialog_checklistwrapper - missing function now available with full feature parityshiftoperations that caused edge case failuresModule-Specific Dialog Fixes
dialog_passwordbox🐳 Container Metadata (4 commits)
Documentation & UX
container_typefield to all Docker-based modules in JSON configsModules with Container Metadata
All Docker-based modules now show container type in menus and documentation
🔒 Security (1 commit)
Command Injection Fix
sh -c "pihole setpassword ${var}"to safepihole setpassword "${var}"🐛 Bug Fixes (12+ commits)
WireGuard Module
NUMBER_OF_PEERSargument processingNetwork Modules
rsyncd Module
SSH Server Module
Other Modules
✨ Code Quality (1 commit)
Refactoring
wait_for_container_ready()helper functionModules Using New Helper
📁 Changed Files (32 total)
Core Functions
tools/modules/functions/config_interface.sh- Dialog wrappers & helperstools/modules/functions/package.sh- Exit code propagationDocumentation
tools/config-markdown.py- Container badgestools/json/config.software.json- Container metadatatools/json/config.system.json- Container metadataNetwork Modules (2 files)
tools/modules/network/module_network_config.shtools/modules/network/module_network_simple.shSoftware Modules (14 files)
System Modules (12 files)
Other
bin/armbian-config- Dialog detection logic✅ Benefits
🧪 Test Plan
Whiptail ramains default. If you want to test
dialog, do it this way: