Create wled-tools.sh #4625
Create wled-tools.sh #4625
Conversation
Per discussion, add a discover-fueled tool to update/backup wled devices in the local network.
WalkthroughA new Bash script, Changes
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
tools/wled-tools.sh (6)
3-9: Unused Color Variable Defined
TheGREENvariable is defined but never used in the script. Consider removing it or utilizing it for consistency with the other color definitions.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 4-4: GREEN appears unused. Verify use (or export if used externally).
(SC2034)
27-47: Generic cURL Handler is Functional
Thecurl_handlerfunction encapsulates HTTP request error handling well. One minor point: ensure that the command passed to this function does not introduce word-splitting issues. Consider validating or sanitizing$commandif user input could ever be involved.
71-87: Device Discovery Function is Effective
Thediscover_devicesfunction leveragesavahi-browseand processes its output correctly. If you wish to enhance robustness, consider outputting discovered device entries as newline-separated values instead of a space‐delimited string (this can help avoid potential parsing issues when hostname fields contain spaces).
89-113: Backup Function – Quotation Issue in cURL Command Strings
In thebackup_onefunction, the assignments on lines 105 and 106 trigger shellcheck warnings because inner quotes are not properly escaped. This may lead to unexpected behavior when the URLs or destination paths contain spaces.
Consider applying the following diff to fix the quoting:- local curl_command_cfg="curl -s "$cfg_url" -o "$cfg_dest.tmp"" - local curl_command_presets="curl -s "$presets_url" -o "$presets_dest.tmp"" + local curl_command_cfg="curl -s \"$cfg_url\" -o \"$cfg_dest.tmp\"" + local curl_command_presets="curl -s \"$presets_url\" -o \"$presets_dest.tmp\""🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 105-105: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
[warning] 106-106: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
115-128: Firmware Update Function – Quotation in cURL Command Needs Correction
On line 125 inupdate_one, the command string is not properly quoted, which may lead to issues with files or URLs containing spaces. Applying proper escaping will make the command robust. For example:- local curl_command="curl -s -X POST -F "file=@$firmware" "$url"" + local curl_command="curl -s -X POST -F \"file=@$firmware\" \"$url\""🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 125-125: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
181-242: Command Execution Block Appears Consistent
The switch-case block executing the chosen command (discover,backup, orupdate) is logically structured and handles errors (e.g., missing targets) appropriately. Ensure that the assumptions (e.g., defaulting to port 80 for a target) are documented if they are intended design decisions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tools/wled-tools.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
tools/wled-tools.sh
[warning] 4-4: GREEN appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 105-105: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
[warning] 106-106: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
[warning] 125-125: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
🔇 Additional comments (1)
tools/wled-tools.sh (1)
11-25: Logging Function Implementation Looks Good
Thelogfunction correctly handles conditional terminal output based on thequietflag and whether the output is a terminal.
| while [[ $# -gt 0 ]]; do | ||
| case "$1" in | ||
| -h|--help) | ||
| show_help | ||
| exit 0 | ||
| ;; | ||
| -t|--target) | ||
| target="$2" | ||
| shift 2 | ||
| ;; | ||
| -D|--discover) | ||
| discover=true | ||
| shift | ||
| ;; | ||
| -d|--directory) | ||
| backup_dir="$2" | ||
| shift 2 | ||
| ;; | ||
| -f|--firmware) | ||
| firmware_file="$2" | ||
| shift 2 | ||
| ;; | ||
| -q|--quiet) | ||
| quiet=true | ||
| shift | ||
| ;; | ||
| backup|update|discover) | ||
| command="$1" | ||
| shift | ||
| ;; | ||
| *) | ||
| log "ERROR" "$RED" "Unknown argument: $1" | ||
| exit 1 | ||
| ;; | ||
| esac |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Missing Verbose Option Handling in Command-Line Arguments
While the help message (show_help) describes a -v/--verbose option, there is no case handling for it in the argument processing loop. If verbose output is intended, add an option block to set a verbose variable and use it accordingly in your functions (for example, to control additional debug output).
Missing Verbose Option Handling in Command-Line Arguments
The help message clearly references a -v/--verbose option to enable verbose output; however, the current argument parser in tools/wled-tools.sh (lines 143-177) lacks any handling for this flag. To align the implementation with the documentation, please add a case for -v|--verbose that sets a verbose variable (e.g., to true) and then shifts the arguments. For instance:
-v|--verbose)
verbose=true
shift
;;This update will ensure that verbose mode can be correctly activated as documented.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tools/wled-tools.sh (1)
142-177: 🛠️ Refactor suggestionMissing Verbose Option Handling
The PR objectives and help text documentation indicate support for a verbose flag (-v/--verbose), yet no corresponding case exists in the argument parsing block. To enable verbose logging, add handling for this flag (e.g., set a global variableverbose=true) and update the help text to list the verbose option.Proposed diff for the argument parsing (suggest inserting after the
-f|--firmwarecase and before-q|--quiet):- -f|--firmware) - firmware_file="$2" - shift 2 - ;; + -f|--firmware) + firmware_file="$2" + shift 2 + ;; + -v|--verbose) + verbose=true + shift + ;;Proposed diff for updating the help message (inside the heredoc in
show_help):-f, --firmware <File> Specify the firmware file for updating devices. + -v, --verbose Enable verbose output for debugging. -q, --quiet Suppress logging output (also makes discover output hostnames only).
🧹 Nitpick comments (4)
tools/wled-tools.sh (4)
4-8: Unused Variable Warning: GREEN
The variableGREENis defined at the top but is never referenced in the script. If you plan to use it in future logging or other features, consider adding an inline comment for clarity; otherwise, removing it could help clean up the code.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 4-4: GREEN appears unused. Verify use (or export if used externally).
(SC2034)
104-105: Fix Quoting in Curl Command Construction for Backup
The curl command strings in thebackup_onefunction use inner quotes without escaping, which may lead to unexpected behavior or shellcheck warnings. Using escaped quotes will ensure the URL and destination paths are correctly interpreted.- local curl_command_cfg="curl -s "$cfg_url" -o "$cfg_dest.tmp"" - local curl_command_presets="curl -s "$presets_url" -o "$presets_dest.tmp"" + local curl_command_cfg="curl -s \"$cfg_url\" -o \"$cfg_dest.tmp\"" + local curl_command_presets="curl -s \"$presets_url\" -o \"$presets_dest.tmp\""🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 104-104: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
[warning] 105-105: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
124-124: Fix Quoting in Curl Command for Firmware Update
The curl command in theupdate_onefunction is also affected by unescaped inner quotes. Escaping these quotes correctly will fix potential issues with command interpretation.- local curl_command="curl -s -X POST -F "file=@$firmware" "$url"" + local curl_command="curl -s -X POST -F \"file=@$firmware\" \"$url\""🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 124-124: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
107-111: Enhance Backup Error Handling
In thebackup_onefunction, both curl calls are executed without checking their exit statuses. To prevent partial backups (e.g., renaming temporary files even if one download fails), consider verifying that eachcurl_handlercall succeeds before moving the temporary files. For example:if ! curl_handler "$curl_command_cfg" "$hostname"; then log "ERROR" "$RED" "Failed to backup config for $hostname." return 1 fi if ! curl_handler "$curl_command_presets" "$hostname"; then log "ERROR" "$RED" "Failed to backup presets for $hostname." return 1 fi mv "$cfg_dest.tmp" "$cfg_dest" mv "$presets_dest.tmp" "$presets_dest"This additional check will help ensure data integrity during the backup process.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tools/wled-tools.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
tools/wled-tools.sh
[warning] 4-4: GREEN appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 104-104: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
[warning] 105-105: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
[warning] 124-124: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
There was a problem hiding this comment.
If you drop the extension, it leaves the door open to ever be rewritten in python for example.
Because for a user the language is not of interest.
I'm not running libreoffice.sh or ls.py either 🙂
Per discussion #4621, add a discover-fueled tool to update/backup wled devices in the local network.
Todo:
Help text:
Example command:
Summary by CodeRabbit