feat(downloaders): Add support for unlimited numbers of qBt instances in the mover-tuner scripts #2556
Conversation
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
Contributor
Reviewer's GuideAdds array-based configuration and format detection to mover-tuning start/end scripts to support an arbitrary number of qBittorrent instances while keeping backward compatibility with the legacy 1–2 instance config, and fixes version comparison plus config validation accordingly. Class diagram for legacy vs array qBittorrent configuration modelclassDiagram
class LegacyConfig {
<<config>>
bool ENABLE_QBIT_2
string QBIT_NAME_1
string QBIT_HOST_1
string QBIT_USER_1
string QBIT_PASS_1
string QBIT_NAME_2
string QBIT_HOST_2
string QBIT_USER_2
string QBIT_PASS_2
}
class ArrayConfig {
<<config>>
string[] NAMES
string[] HOSTS
string[] USERS
string[] PASSWORDS
}
class ScriptSettings {
<<config>>
int DAYS_FROM
int DAYS_TO
string CACHE_MOUNT
string QBIT_MOVER_PATH
}
class ConfigHandler {
<<utility>>
string detect_config_format()
int get_instance_count()
string get_instance_details(index)
void validate_config()
void check_script_version()
void check_config_version()
}
ConfigHandler --> LegacyConfig : supports_legacy
ConfigHandler --> ArrayConfig : supports_array
ConfigHandler --> ScriptSettings : validates
class InstanceProcessing {
<<logic>>
void main()
void process_qbit_instance(name, host, user, password)
}
InstanceProcessing --> ConfigHandler : uses
InstanceProcessing --> LegacyConfig : reads_if_legacy
InstanceProcessing --> ArrayConfig : reads_if_array
InstanceProcessing --> ScriptSettings : uses_for_limits
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
github-actions
bot
added
the
Area: Backend
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The updated version checks in both scripts remove the previous
|| trueguard on thegreppipeline; withset -euo pipefailenabled this will cause the whole script to exit when the local version is newer (i.e., whengrepreturns 1), so you should restore a non-fatal guard or explicitly handle the non-match case without relying on a failing pipeline. - In
get_instance_detailsyou concatenate instance fields with|and then split usingIFS='|' read ...; this will break if any username or password ever contains|, so it would be safer to return fields via global variables, nameref parameters, or a small associative array instead of encoding them into a delimited string.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The updated version checks in both scripts remove the previous `|| true` guard on the `grep` pipeline; with `set -euo pipefail` enabled this will cause the whole script to exit when the local version is newer (i.e., when `grep` returns 1), so you should restore a non-fatal guard or explicitly handle the non-match case without relying on a failing pipeline.
- In `get_instance_details` you concatenate instance fields with `|` and then split using `IFS='|' read ...`; this will break if any username or password ever contains `|`, so it would be safer to return fields via global variables, nameref parameters, or a small associative array instead of encoding them into a delimited string.
## Individual Comments
### Comment 1
<location> `includes/downloaders/mover-tuning-start.sh:76-85` </location>
<code_context>
+ local format
+ format=$(detect_config_format)
+
+ if [[ "$format" == "array" ]]; then
+ echo "${#HOSTS[@]}"
+ else
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Array-based config validates lengths but not that individual host/user/pass entries are non-empty.
This means a config with empty host/user/password values would still be accepted and only fail later at use time. Please add a per-index check in the array branch (e.g., loop over `i < ${#HOSTS[@]}`) to ensure `HOSTS[i]`, `USERS[i]`, and `PASSWORDS[i]` are all non-empty before proceeding.
Suggested implementation:
```
get_instance_count() {
local format
format=$(detect_config_format)
if [[ "$format" == "array" ]]; then
local i
for (( i=0; i<${#HOSTS[@]}; i++ )); do
if [[ -z "${HOSTS[i]}" || -z "${USERS[i]}" || -z "${PASSWORDS[i]}" ]]; then
echo "Invalid array-based configuration: HOSTS[$i], USERS[$i], and PASSWORDS[$i] must all be non-empty." >&2
exit 1
fi
done
echo "${#HOSTS[@]}"
```
1. Ensure that `USERS` and `PASSWORDS` arrays are always defined when `HOSTS` is used in array mode; if there is any code path where they might be unset, they should be initialized (e.g., to empty arrays) before calling `get_instance_count`.
2. If `get_instance_count` is used in contexts where exiting the script is not desirable, you may want to refactor this validation into a separate function (e.g., `validate_array_config`) that callers can handle differently (e.g., by checking a return code) instead of exiting here.
</issue_to_address>
### Comment 2
<location> `includes/downloaders/mover-tuning-end.sh:63-72` </location>
<code_context>
+ local format
+ format=$(detect_config_format)
+
+ if [[ "$format" == "array" ]]; then
+ echo "${#HOSTS[@]}"
+ else
</code_context>
<issue_to_address>
**suggestion:** Same array validation gap exists in the end script, leading to similar edge cases.
This mirrors the start script: it only enforces matching lengths for `HOSTS/USERS/PASSWORDS`, not that individual entries are non-empty. Once you add per-entry validation to the start script, please apply the same checks here so misconfigurations are caught consistently and early.
Suggested implementation:
```
if [[ "$format" == "array" ]]; then
# Validate that HOSTS, USERS, and PASSWORDS arrays are consistent and non-empty
if (( ${#HOSTS[@]} == 0 )); then
echo "No HOSTS configured for array-based configuration." >&2
exit 1
fi
if (( ${#HOSTS[@]} != ${#USERS[@]} || ${#HOSTS[@]} != ${#PASSWORDS[@]} )); then
echo "Configuration error: HOSTS, USERS, and PASSWORDS arrays must have the same length." >&2
exit 1
fi
for i in "${!HOSTS[@]}"; do
if [[ -z "${HOSTS[$i]}" || -z "${USERS[$i]}" || -z "${PASSWORDS[$i]}" ]]; then
echo "Configuration error: HOSTS, USERS, and PASSWORDS entries must be non-empty (index $i)." >&2
exit 1
fi
done
echo "${#HOSTS[@]}"
```
1. The snippet you provided shows two `get_instance_count` definitions and a seemingly incomplete `if`/`fi` structure. In the actual file, ensure there is only one `get_instance_count` function and that its `if`/`else`/`fi` structure is syntactically correct.
2. If the start script uses specific error message wording or a shared helper for validation, you may want to adjust the error messages here to match that convention for consistency.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+63
to
+72
| if [[ "$format" == "array" ]]; then | ||
| echo "${#HOSTS[@]}" | ||
| else | ||
| # Legacy format: count based on ENABLE_QBIT_2 | ||
| if [[ "${ENABLE_QBIT_2:-false}" == true ]]; then | ||
| echo "2" | ||
| else | ||
| echo "1" | ||
| fi | ||
| fi |
Contributor
There was a problem hiding this comment.
suggestion: Same array validation gap exists in the end script, leading to similar edge cases.
This mirrors the start script: it only enforces matching lengths for HOSTS/USERS/PASSWORDS, not that individual entries are non-empty. Once you add per-entry validation to the start script, please apply the same checks here so misconfigurations are caught consistently and early.
Suggested implementation:
if [[ "$format" == "array" ]]; then
# Validate that HOSTS, USERS, and PASSWORDS arrays are consistent and non-empty
if (( ${#HOSTS[@]} == 0 )); then
echo "No HOSTS configured for array-based configuration." >&2
exit 1
fi
if (( ${#HOSTS[@]} != ${#USERS[@]} || ${#HOSTS[@]} != ${#PASSWORDS[@]} )); then
echo "Configuration error: HOSTS, USERS, and PASSWORDS arrays must have the same length." >&2
exit 1
fi
for i in "${!HOSTS[@]}"; do
if [[ -z "${HOSTS[$i]}" || -z "${USERS[$i]}" || -z "${PASSWORDS[$i]}" ]]; then
echo "Configuration error: HOSTS, USERS, and PASSWORDS entries must be non-empty (index $i)." >&2
exit 1
fi
done
echo "${#HOSTS[@]}"
- The snippet you provided shows two
get_instance_countdefinitions and a seemingly incompleteif/fistructure. In the actual file, ensure there is only oneget_instance_countfunction and that itsif/else/fistructure is syntactically correct. - If the start script uses specific error message wording or a shared helper for validation, you may want to adjust the error messages here to match that convention for consistency.
Contributor
|
Deploying with ⚡ Cloudflare Pages
|
- Fixed the grep pipeline issue - Fixed the pipe delimiter issue in get_instance_details
Contributor
|
Deploying with ⚡ Cloudflare Pages
|
Contributor
|
Deploying with ⚡ Cloudflare Pages
|
Contributor
|
Deploying with ⚡ Cloudflare Pages
|
TRaSH-
changed the title
Contributor
|
@sourcery-ai review |
Contributor
|
Deploying with ⚡ Cloudflare Pages
|
mover-tuning-start.sh: Full validation with Unraid notifications for configuration errors Shows detailed error messages with array lengths mover-tuning-end.sh: Basic validation (still checks for errors to prevent execution with bad config) Only logs to terminal, no Unraid notifications
Contributor
|
Deploying with ⚡ Cloudflare Pages
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Pull Request
Purpose
Add support for unlimited numbers of qBittorrent instances in the mover-tuner scripts
Approach
mover-tuning-start.sh
Version: 1.0.0 → 1.1.0
Added support for more than 2 qBittorrent instances
Added config format detection functions for backward Compatibility
Added array-based configuration support
Fixed version comparison logic
Enhanced validation for array configuration
mover-tuning-end.sh
Version: 1.0.0 → 1.1.0
Added support for more than 2 qBittorrent instances
Added config format detection functions for backward Compatibility
Added array-based configuration support
Fixed version comparison logic
Enhanced validation for array configuration
Version: 1.0.0 → 1.1.0
Changed BitTorrent config from legacy to array format
Backward Compatibility
Open Questions and Pre-Merge TODOs
Requirements
Summary by Sourcery
Add array-based configuration and format detection to the mover-tuning scripts to support an arbitrary number of qBittorrent instances while maintaining backward compatibility with the legacy two-instance setup.
New Features:
Enhancements:
Documentation: