feat: add windows powershell setup and uninstall scripts#18
Open
nordicnode wants to merge 2 commits into
Open
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>
Contributor
|
👋 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. Arr auth wrong scope
|
📝 WalkthroughWalkthroughThis PR introduces complete automation for provisioning and removing a TorBox Media Server Docker stack on Windows. The ChangesTorBox Media Server Setup and Uninstall
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 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)
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+453
to
+466
| $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) { | ||
| $config.authenticationMethod = "forms" | ||
| $config.authenticationRequired = "enabledForRequests" | ||
| $config.username = $user | ||
| $config.password = $pass | ||
|
|
||
| Invoke-RestMethod -Uri $settingsUrl -Method Put -Headers $headers -Body ($config | ConvertTo-Json -Depth 10) -ErrorAction Stop | Out-Null | ||
| Write-LogInfo " $Name authentication configured (Forms)." | ||
| } |
There was a problem hiding this comment.
1. Arr auth wrong scope 🐞 Bug ≡ Correctness
Invoke-ConfigureArrAuth reads credentials with Get-Variable in function scope, but the credentials are stored as $global:*, so $user/$pass can resolve to $null and the auth-update block is skipped. This leaves Radarr/Sonarr/Prowlarr admin credentials generated/printed but not actually applied via the API call.
Agent Prompt
## Issue description
`Invoke-ConfigureArrAuth` builds variable names (e.g., `RadarrAdminUser`) and calls `Get-Variable` without a global scope, but the script stores these values as `$global:RadarrAdminUser`, `$global:RadarrAdminPass`, etc. As a result, the lookup can return `$null` and the authentication configuration never runs.
## Issue Context
The credentials are created in `Invoke-GatherConfig` using `$global:` scope, but later looked up without specifying that scope.
## Fix Focus Areas
- setup.ps1[453-466]
- setup.ps1[213-222]
## Suggested fix
- Replace the variable lookups with explicit global-scope reads, e.g.:
- `Get-Variable -Name $userVar -Scope Global -ValueOnly`
- `Get-Variable -Name $passVar -Scope Global -ValueOnly`
- Alternatively, pass the username/password into `Invoke-ConfigureArrAuth` instead of dynamic variable name lookup.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+205
to
+222
| $global:PUID = 1000 | ||
| $global:PGID = 1000 | ||
| $global:TZ = [System.TimeZoneInfo]::Local.Id | ||
|
|
||
| $global:RadarrApiKey = New-ApiKey | ||
| $global:SonarrApiKey = New-ApiKey | ||
| $global:ProwlarrApiKey = New-ApiKey | ||
|
|
||
| $global:RadarrAdminUser = "admin" | ||
| $global:RadarrAdminPass = New-AdminPass | ||
| $global:SonarrAdminUser = "admin" | ||
| $global:SonarrAdminPass = New-AdminPass | ||
| $global:ProwlarrAdminUser = "admin" | ||
| $global:ProwlarrAdminPass = New-AdminPass | ||
|
|
||
| $global:DecypharrUser = "torbox" | ||
| $global:DecypharrPass = New-AdminPass | ||
| return $true |
There was a problem hiding this comment.
2. Setup rerun breaks installs 🐞 Bug ☼ Reliability
Invoke-GatherConfig always generates new *arr API keys and admin passwords and Main never checks for an existing completed install to preserve them. Re-running setup.ps1 will change keys/passwords and can break existing service integrations, contradicting the repo’s documented “safe re-run” behavior.
Agent Prompt
## Issue description
`setup.ps1` unconditionally regenerates API keys and admin credentials on every run. If a user reruns setup to repair/reconfigure, their existing Radarr/Sonarr/Prowlarr keys can change and integrations break.
## Issue Context
- README currently states re-running setup is safe and preserves existing API keys.
- `setup.sh` implements `check_existing_installation` and loads existing keys from `.env`.
## Fix Focus Areas
- setup.ps1[205-222]
- setup.ps1[839-883]
- README.md[219-222]
- setup.sh[2751-2815]
## Suggested fix
- Add a Windows equivalent of `check_existing_installation`:
- If `$SetupCompleteFile` exists (or `.env` exists), prompt user (or auto-continue in non-interactive) to reuse existing values.
- Parse `.env` for existing `RADARR_API_KEY`, `SONARR_API_KEY`, `PROWLARR_API_KEY`, and admin creds, and only generate new ones if missing/invalid.
- Optionally create timestamped backups of `.env` / generated configs before overwriting.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+99
to
+103
| if ($removeImages.ToLower() -eq 'y') { | ||
| Write-LogInfo "Removing Docker images..." | ||
| # Simplified cleanup for Windows - removing known torbox images or prunning | ||
| docker system prune -a --volumes --force 2>&1 | Out-Null | ||
| Write-LogInfo "Pruned Docker images and volumes." |
There was a problem hiding this comment.
3. Uninstall prunes all docker 🐞 Bug ☼ Reliability
When the user opts to remove images, uninstall.ps1 executes `docker system prune -a --volumes --force`, which can delete unrelated images and volumes from the entire machine. This creates a high risk of collateral data loss for other Docker projects.
Agent Prompt
## Issue description
`uninstall.ps1` uses `docker system prune -a --volumes --force`, which removes *all* unused Docker resources on the machine, not just TorBox Media Server.
## Issue Context
The Linux uninstall script removes only images referenced by this project’s `docker-compose.yml`.
## Fix Focus Areas
- uninstall.ps1[99-103]
- uninstall.sh[193-213]
## Suggested fix
- Prefer scoped cleanup, e.g.:
- `docker compose --env-file .env down --remove-orphans --volumes` (project-scoped)
- Optionally remove only images referenced in the project compose file by parsing `docker-compose.yml` for `image:` entries and calling `docker rmi` on those.
- If you keep any prune option, make it explicit in the prompt that it affects the entire Docker environment and do *not* include `--volumes` by default.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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.
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.
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