Repository navigation
feat: add windows powershell setup and uninstall scripts - #18
Conversation
Translates `setup.sh` and `uninstall.sh` into full-parity `setup.ps1` and `uninstall.ps1` scripts for Windows. Implements native PowerShell replacements (`Invoke-RestMethod`, `ConvertTo-Json`) for API configuration logic, checks for `docker`, and uses `[System.Security.Cryptography.RandomNumberGenerator]` for secure generation of keys. Includes documentation updates. Co-authored-by: nordicnode <128633122+nordicnode@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Code Review by Qodo
1.
|
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR introduces complete automation for provisioning and removing a TorBox Media Server Docker stack on Windows. The ChangesTorBox Media Server Setup Automation
TorBox Media Server Uninstall Automation
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoAdd Windows PowerShell setup and uninstall scripts for TorBox Media Server WalkthroughsDescription• Add full Windows-first installer (setup.ps1) with Docker checks and interactive prompts. • Generate configs/.env and start Docker Compose services with optional non-interactive mode. • Provide Windows uninstaller (uninstall.ps1) with optional backup and Docker cleanup. Diagramgraph TD
U["User runs setup.ps1"] --> D{"Deps OK?"} --> C["Gather config / creds"] --> P{"Ports free?"} --> G["Generate env + app configs"] --> S["docker compose up"] --> A["Configure services via APIs"] --> F["Print URLs + credentials"]
High-Level AssessmentThe following are alternative approaches to this PR: 1. Unify setup into cross-platform PowerShell (pwsh) only
2. Move service auto-configuration into a dedicated config container/job
Recommendation: The PR’s approach (native PowerShell scripts) is appropriate for Windows compatibility and makes installation accessible without extra runtimes. To reduce long-term maintenance risk, consider extracting shared constants/templates (ports, service names, API payload templates) into reusable data files so the PowerShell and shell implementations can stay aligned with fewer duplicated edits. File ChangesEnhancement (1)
Other (1)
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
setup.ps1 (2)
581-632: 💤 Low valueUnused variables in Plex library configuration.
$prefs(line 604) and$libs(line 621) are assigned but never used. Consider removing the unused assignment or documenting the intent if they're placeholders for future functionality.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@setup.ps1` around lines 581 - 632, The Invoke-ConfigurePlexLibraries function assigns unused variables $prefs and $libs; remove those useless assignments or use them appropriately. Edit Invoke-ConfigurePlexLibraries to either drop the unused Invoke-RestMethod call that sets $prefs (the $prefs variable and its line) and the $libs assignment, or replace them with a clear comment explaining they’re intentionally unused placeholders and suppress unused-variable warnings; keep the token-reading logic (Preferences.xml, $token) and the existing Write-LogInfo/Write-LogWarn behavior unchanged.
93-95: 💤 Low valueUnnecessary dependency check for
curl.The script never uses the
curlcommand - all HTTP calls useInvoke-RestMethod. Additionally, PowerShell aliasescurltoInvoke-WebRequest, so this check would pass even without curl.exe installed.Suggested fix
- if (-not (Get-Command "curl" -ErrorAction SilentlyContinue)) { - $missing += "curl" - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@setup.ps1` around lines 93 - 95, Remove the unnecessary curl dependency check: delete the if block that calls Get-Command "curl" and appends "curl" to $missing (the lines using Get-Command "curl" and $missing += "curl"); since the script uses Invoke-RestMethod and PowerShell aliases curl, no replacement check is needed—just ensure $missing is not otherwise reliant on that entry elsewhere in the script.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@setup.ps1`:
- Around line 320-336: The Invoke-GenerateDockerCompose function currently
copies the compose file without checking source existence and then runs docker
compose config in the CWD, validating the wrong file; fix it by first verifying
the source file at (Join-Path $ScriptDir "docker-compose.yml") exists and fail
early with Write-LogWarn/Write-LogStep if not, then copy to $ComposeFile, and
validate the copied file explicitly (either run docker compose -f $ComposeFile
config -q or change location to $InstallDir before running docker compose
config) so docker validates the file in $InstallDir; keep existing Remove-Item
logic for $OverrideFile and preserve the try/catch around the docker call
(update log messages to reference $ComposeFile where appropriate).
- Around line 663-667: The fields array entry currently sets username to a URL
using "http://$Type:$Port" which can be mis-parsed by PowerShell and is likely
the wrong value for qBittorrent/Decypharr auth; update the string to use
explicit interpolation like "http://${Type}:${Port}" (or
"http://$(${Type}):$(${Port})") to fix the parsing, and then verify the
semantics of the username/password entries: if Decypharr/qBittorrent auth is
disabled, remove or null out the username/password fields (or leave them empty);
if auth is enabled, replace the URL in the "username" field with the actual auth
username and ensure "password" contains the corresponding secret, keeping host
and port in their respective fields (fields array entries: @{ name = "host"; ...
}, @{ name = "port"; ... }, @{ name = "username"; ... }).
In `@uninstall.ps1`:
- Around line 85-87: The script currently calls Remove-Item on $InstallDir then
always logs success with Write-LogInfo; change this to verify deletion using
Test-Path after the Remove-Item call and log accordingly: after Remove-Item
-Path $InstallDir -Recurse -Force -ErrorAction SilentlyContinue, add an if (-not
(Test-Path $InstallDir)) { Write-LogInfo "Removed: $InstallDir" } else {
Write-LogError "Failed to remove: $InstallDir"; optionally attempt a second
Remove-Item or include the removal error details } so success is only logged
when Test-Path confirms the directory is gone.
- Around line 99-103: The uninstall script currently uses a host-wide command
(docker system prune -a --volumes --force) when $removeImages is 'y', which can
remove unrelated Docker resources; change this to scope cleanup to the TorBox
compose project instead: replace the global prune line in uninstall.ps1 with a
project-scoped command such as invoking docker compose down --rmi all --volumes
--remove-orphans (or explicitly remove the known TorBox image names with docker
image rm <image>), ensuring the surrounding Write-LogInfo messages remain and
that the conditional on $removeImages is preserved.
- Around line 61-79: The uninstall script currently suppresses Docker teardown
failures (docker compose down and docker rm -f in the blocks guarded by
$EnvFile/$ComposeFile) but proceeds as if successful; update the logic in the
block that uses $EnvFile, $ComposeFile, $InstallDir and the $svcs loop so that
after running "docker compose down" you check $LASTEXITCODE and if non-zero
either exit the script with a non-zero code (fail fast) or set a persistent
"partial_uninstall" flag and return/exit before removing local files; do the
same for the fallback branch that runs docker rm -f for each entry in $svcs
(check each docker rm exit code or aggregate a failure flag), and call
Write-LogWarn/Write-LogInfo accordingly (preserve the existing Write-LogWarn
messages) so local file deletion is only performed when teardown is verified or
the script explicitly records a partial uninstall state.
---
Nitpick comments:
In `@setup.ps1`:
- Around line 581-632: The Invoke-ConfigurePlexLibraries function assigns unused
variables $prefs and $libs; remove those useless assignments or use them
appropriately. Edit Invoke-ConfigurePlexLibraries to either drop the unused
Invoke-RestMethod call that sets $prefs (the $prefs variable and its line) and
the $libs assignment, or replace them with a clear comment explaining they’re
intentionally unused placeholders and suppress unused-variable warnings; keep
the token-reading logic (Preferences.xml, $token) and the existing
Write-LogInfo/Write-LogWarn behavior unchanged.
- Around line 93-95: Remove the unnecessary curl dependency check: delete the if
block that calls Get-Command "curl" and appends "curl" to $missing (the lines
using Get-Command "curl" and $missing += "curl"); since the script uses
Invoke-RestMethod and PowerShell aliases curl, no replacement check is
needed—just ensure $missing is not otherwise reliant on that entry elsewhere in
the script.
🪄 Autofix (Beta)
✅ Autofix completed
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 2 file(s) based on 5 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 2 file(s) based on 5 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
uninstall.ps1 (2)
112-129:⚠️ Potential issue | 🟠 Major | ⚡ Quick winImage removal is unreachable when directory deletion succeeds.
At lines 99-105, when
$partialUninstallis false, the installation directory (containing.envanddocker-compose.yml) is deleted. Subsequently, theTest-Pathcheck at line 120 will always fail because the compose files no longer exist. Users who opt to remove images will see "Compose file not available" instead of actual cleanup.The image removal prompt and logic should execute before directory deletion to preserve access to the compose metadata.
🔧 Suggested restructure
Move lines 112-129 (image removal prompt and execution) to occur before lines 94-110 (directory deletion):
docker network rm "${projectName}_media-network" 2>&1 | Out-Null +# Ask about image removal BEFORE deleting directory (needs compose files) +if (-not $NonInteractive) { + $removeImages = Read-Host "Remove Docker images to free ~5-8 GB of disk space? [y/N]" +} else { + $removeImages = "n" +} + +if ($removeImages.ToLower() -eq 'y') { + Write-LogInfo "Removing Docker images..." + if ((Test-Path $EnvFile) -and (Test-Path $ComposeFile)) { + Set-Location $InstallDir + docker compose down --rmi all --volumes --remove-orphans 2>&1 | Out-Null + Write-LogInfo "Removed project images and volumes." + } else { + Write-LogWarn "Compose file not available. Skipping image removal." + } +} else { + Write-LogInfo "Docker images kept." +} + if ($partialUninstall) { Write-LogWarn "Docker teardown had failures. Skipping local file deletion to preserve state." Write-LogInfo "Installation directory preserved: $InstallDir" } else { Write-LogInfo "Removing installation directory..." # ... rest of deletion logic } - -if (-not $NonInteractive) { - $removeImages = Read-Host "Remove Docker images to free ~5-8 GB of disk space? [y/N]" -} else { - $removeImages = "n" -} -# ... remove the old block at end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@uninstall.ps1` around lines 112 - 129, Image cleanup is blocked because the compose metadata is deleted before the Docker image removal step runs. Move the remove-images prompt and execution logic in uninstall.ps1 (the block that uses Read-Host, Test-Path, and docker compose down) to run before the installation directory deletion path, so $EnvFile and $ComposeFile still exist when the cleanup check happens.
131-134:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCompletion message is misleading when teardown had failures.
When
$partialUninstallis true, the script warns about Docker failures and preserves the directory (lines 94-96), but then unconditionally prints "Uninstall complete" at line 131. This creates contradictory messaging.🔧 Proposed fix
-Write-Host "`nUninstall complete." -ForegroundColor Green +if ($partialUninstall) { + Write-Host "`nPartial uninstall complete (some issues encountered)." -ForegroundColor Yellow +} else { + Write-Host "`nUninstall complete." -ForegroundColor Green +} Write-Host "Your TorBox account and cloud-stored media are unaffected." Write-Host "To reinstall, run: .\setup.ps1"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@uninstall.ps1` around lines 131 - 134, The final completion message in uninstall.ps1 is unconditional and can contradict earlier failure handling when $partialUninstall is true. Update the end-of-script messaging so the “Uninstall complete” text is only shown for a fully successful teardown, and emit a clear partial-failure completion message instead when the Docker teardown path fails; use the existing $partialUninstall check and the warning logic near the cleanup flow to decide which message to print.setup.ps1 (1)
205-208:⚠️ Potential issue | 🔴 CriticalWindows timezone format incompatible with Linux containers.
[System.TimeZoneInfo]::Local.Idreturns Windows-specific timezone IDs (e.g.,"Pacific Standard Time") but the TZ environment variable passed to Docker containers expects IANA format (e.g.,"America/Los_Angeles"). This causes timezone misconfiguration in all containerized services (Radarr, Sonarr, Plex, Jellyfin, etc.).The proposed fix has syntax errors and assumes .NET 6.0+ / PowerShell 7+ without validation. Use a simpler approach with proper error handling:
Corrected timezone conversion
$global:PUID = 1000 $global:PGID = 1000 - $global:TZ = [System.TimeZoneInfo]::Local.Id + # Convert Windows timezone to IANA format for Linux containers + $windowsId = [System.TimeZoneInfo]::Local.Id + $ianaId = "" + if ([System.TimeZoneInfo]::TryConvertWindowsIdToIanaId($windowsId, [ref]$ianaId)) { + $global:TZ = $ianaId + } else { + Write-LogWarn "Unable to convert Windows timezone '$windowsId' to IANA format; falling back to UTC" + $global:TZ = "UTC" + }Alternatively, default to UTC and document that Windows timezone mapping requires PowerShell 7+ (.NET 6+).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@setup.ps1` around lines 205 - 208, The `$global:TZ` assignment using `[System.TimeZoneInfo]::Local.Id` in the setup.ps1 script captures Windows-specific timezone format (e.g., "Pacific Standard Time") instead of the IANA format (e.g., "America/Los_Angeles") required by Docker containers, causing timezone misconfiguration in services like Radarr, Sonarr, Plex, and Jellyfin. Either implement a reliable conversion mechanism from Windows timezone IDs to IANA format with proper error handling and version checks, or simplify by defaulting `$global:TZ` to "UTC" and document that users requiring specific timezone handling need PowerShell 7+ with .NET 6+ for advanced timezone mapping support.
♻️ Duplicate comments (1)
uninstall.ps1 (1)
61-67:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd Docker CLI availability check before attempting teardown.
If Docker is not installed or not in PATH, all subsequent
dockercommands will fail silently (output piped toOut-Null) and the script may report misleading results.🛡️ Proposed fix
Write-LogInfo "Stopping and removing Docker containers..." $partialUninstall = $false +if (-not (Get-Command docker -ErrorAction SilentlyContinue)) { + Write-LogError "Docker CLI not found. Cannot proceed with container removal." + $partialUninstall = $true +} elseif ((Test-Path $EnvFile) -and (Test-Path $ComposeFile)) { -if ((Test-Path $EnvFile) -and (Test-Path $ComposeFile)) { Set-Location $InstallDir🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@uninstall.ps1` around lines 61 - 67, The script attempts to execute the docker compose down command without first verifying that Docker is installed and accessible in the PATH. Add a check before the docker compose down command (within the conditional block that tests $EnvFile and $ComposeFile) to verify that the Docker CLI is available. If Docker is not available, handle this gracefully by logging an appropriate message and setting $partialUninstall to true to indicate the teardown could not be completed. This prevents silent failures where the docker command is not found but its failure is masked by the output redirection to Out-Null.
🧹 Nitpick comments (1)
setup.ps1 (1)
464-467: ⚡ Quick winDynamic variable lookup is fragile and fails silently.
The pattern
Get-Variable -Name "$($Name)AdminUser"relies on an implicit naming convention between the$Nameparameter (e.g.,"Radarr") and the global variable prefix ($global:RadarrAdminUser). With-ErrorAction SilentlyContinue, any mismatch causes silent failure, leaving authentication unconfigured.Consider using explicit parameter passing or a hashtable lookup instead:
Proposed refactor using hashtable lookup
function Invoke-ConfigureArrAuth { param ( [string]$Name, [string]$Url, [string]$ApiKey, - [string]$ApiVer = "v3" + [string]$ApiVer = "v3", + [string]$AdminUser, + [string]$AdminPass ) try { $headers = @{ "X-Api-Key" = $ApiKey; "Content-Type" = "application/json" } $settingsUrl = if ($ApiVer -eq "v1") { "$Url/api/v1/config/host" } else { "$Url/api/v3/config/host" } $config = Invoke-RestMethod -Uri $settingsUrl -Method Get -Headers $headers -ErrorAction Stop - $userVar = "$($Name)AdminUser" - $passVar = "$($Name)AdminPass" - $user = (Get-Variable -Name $userVar -ValueOnly -ErrorAction SilentlyContinue) - $pass = (Get-Variable -Name $passVar -ValueOnly -ErrorAction SilentlyContinue) - - if ($user -and $pass) { + if ($AdminUser -and $AdminPass) { $config.authenticationMethod = "forms" $config.authenticationRequired = "enabledForRequests" - $config.username = $user - $config.password = $pass + $config.username = $AdminUser + $config.password = $AdminPassThen update callers:
Invoke-ConfigureArrAuth -Name "Radarr" -Url $radarrUrl -ApiKey $global:RadarrApiKey -AdminUser $global:RadarrAdminUser -AdminPass $global:RadarrAdminPass🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@setup.ps1` around lines 464 - 467, The dynamic variable lookup using Get-Variable with $userVar and $passVar relies on an implicit naming convention between the $Name parameter and global variable prefixes, and with -ErrorAction SilentlyContinue it fails silently if mismatches occur. Replace the Get-Variable calls for retrieving AdminUser and AdminPass with explicit function parameters instead. Modify the Invoke-ConfigureArrAuth function signature to accept AdminUser and AdminPass as explicit parameters, and update all callers to pass these values directly as arguments (e.g., $global:RadarrAdminUser and $global:RadarrAdminPass) rather than relying on dynamic variable name construction.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@setup.ps1`:
- Around line 205-208: The `$global:TZ` assignment using
`[System.TimeZoneInfo]::Local.Id` in the setup.ps1 script captures
Windows-specific timezone format (e.g., "Pacific Standard Time") instead of the
IANA format (e.g., "America/Los_Angeles") required by Docker containers, causing
timezone misconfiguration in services like Radarr, Sonarr, Plex, and Jellyfin.
Either implement a reliable conversion mechanism from Windows timezone IDs to
IANA format with proper error handling and version checks, or simplify by
defaulting `$global:TZ` to "UTC" and document that users requiring specific
timezone handling need PowerShell 7+ with .NET 6+ for advanced timezone mapping
support.
In `@uninstall.ps1`:
- Around line 112-129: Image cleanup is blocked because the compose metadata is
deleted before the Docker image removal step runs. Move the remove-images prompt
and execution logic in uninstall.ps1 (the block that uses Read-Host, Test-Path,
and docker compose down) to run before the installation directory deletion path,
so $EnvFile and $ComposeFile still exist when the cleanup check happens.
- Around line 131-134: The final completion message in uninstall.ps1 is
unconditional and can contradict earlier failure handling when $partialUninstall
is true. Update the end-of-script messaging so the “Uninstall complete” text is
only shown for a fully successful teardown, and emit a clear partial-failure
completion message instead when the Docker teardown path fails; use the existing
$partialUninstall check and the warning logic near the cleanup flow to decide
which message to print.
---
Duplicate comments:
In `@uninstall.ps1`:
- Around line 61-67: The script attempts to execute the docker compose down
command without first verifying that Docker is installed and accessible in the
PATH. Add a check before the docker compose down command (within the conditional
block that tests $EnvFile and $ComposeFile) to verify that the Docker CLI is
available. If Docker is not available, handle this gracefully by logging an
appropriate message and setting $partialUninstall to true to indicate the
teardown could not be completed. This prevents silent failures where the docker
command is not found but its failure is masked by the output redirection to
Out-Null.
---
Nitpick comments:
In `@setup.ps1`:
- Around line 464-467: The dynamic variable lookup using Get-Variable with
$userVar and $passVar relies on an implicit naming convention between the $Name
parameter and global variable prefixes, and with -ErrorAction SilentlyContinue
it fails silently if mismatches occur. Replace the Get-Variable calls for
retrieving AdminUser and AdminPass with explicit function parameters instead.
Modify the Invoke-ConfigureArrAuth function signature to accept AdminUser and
AdminPass as explicit parameters, and update all callers to pass these values
directly as arguments (e.g., $global:RadarrAdminUser and
$global:RadarrAdminPass) rather than relying on dynamic variable name
construction.
…e re-run - Use $script: scope for -y/--non-interactive and -d/--dry-run flags so they take effect when run as .\setup.ps1 ($global: was shadowed by the script-scoped defaults, leaving the flags non-functional) - Write config/.env files as UTF-8 without BOM (Set-Content -Encoding UTF8 on Windows PowerShell 5.1 emits a BOM that corrupts the first .env var for Docker Compose and breaks *arr config.xml parsing) - Configure the Decypharr (qBittorrent mock) download client with the same fields as setup.sh: username=internal URL, password=API key, category, useSsl, and remaining qBittorrent settings, plus ?forceSave=true - Use valid authenticationRequired value "enabled" (was the invalid "enabledForRequests") and read admin creds with -Scope Global - Detect existing/incomplete installations and preserve existing API keys and admin/Decypharr credentials across re-runs, with timestamped backups Amp-Thread-ID: https://ampcode.com/threads/T-019ecea1-1bef-72e5-9474-cdba93533390 Co-authored-by: Amp <amp@ampcode.com>
Adds full Windows compatibility to TorBox Media Server setup.
Changes include:
setup.shto nativesetup.ps1*arrAPI setups usingInvoke-RestMethoduninstall.shtouninstall.ps1README.mdcontaining Windows setup and uninstallation instructionsPR created automatically by Jules for task 4758185216703385500 started by @nordicnode
Summary by CodeRabbit
setup.ps1automation script to provision the TorBox Media Server Docker Compose stack with prerequisite/port conflict checks, repeat-run support (backup + env/config recovery), credential/API key generation, config file creation, optional container start, and guided post-install API setup (including automatic service wiring where possible).uninstall.ps1script to remove the local deployment with optional confirmation/backup, robust Docker teardown (compose down with fallback container removal), safe deletion safeguards, and optional cleanup of images/volumes.