Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: moved downloader proxy settings to parent instead of per debrid #914

Merged
merged 1 commit into from
Nov 27, 2024
+12 −27
Merged

fix: moved downloader proxy settings to parent instead of per debrid #914

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions .env.example
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,15 +66,11 @@ RIVEN_EMBY_URL=http://localhost:8096
#-------------------------------------

RIVEN_DOWNLOADERS_VIDEO_EXTENSIONS=["mp4","mkv","avi"]
RIVEN_DOWNLOADERS_PREFER_SPEED_OVER_QUALITY=false
RIVEN_DOWNLOADERS_PROXY_URL=
RIVEN_DOWNLOADERS_REAL_DEBRID_ENABLED=false
RIVEN_DOWNLOADERS_REAL_DEBRID_API_KEY=
RIVEN_DOWNLOADERS_REAL_DEBRID_PROXY_ENABLED=false
RIVEN_DOWNLOADERS_REAL_DEBRID_PROXY_URL=
RIVEN_DOWNLOADERS_ALL_DEBRID_ENABLED=false
RIVEN_DOWNLOADERS_ALL_DEBRID_API_KEY=
RIVEN_DOWNLOADERS_ALL_DEBRID_PROXY_ENABLED=false
RIVEN_DOWNLOADERS_ALL_DEBRID_PROXY_URL=
RIVEN_DOWNLOADERS_TORBOX_ENABLED=false
RIVEN_DOWNLOADERS_TORBOX_API_KEY=

Expand Down
1 change: 0 additions & 1 deletion src/program/services/downloaders/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ class Downloader:
def __init__(self):
self.key = "downloader"
self.initialized = False
self.speed_mode = settings_manager.settings.downloaders.prefer_speed_over_quality
self.services = {
RealDebridDownloader: RealDebridDownloader(),
TorBoxDownloader: TorBoxDownloader(),
Expand Down
5 changes: 1 addition & 4 deletions src/program/services/downloaders/alldebrid.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ def validate(self) -> bool:

self.api = AllDebridAPI(
api_key=self.settings.api_key,
proxy_url=self.settings.proxy_url if self.settings.proxy_enabled else None
proxy_url=self.PROXY_URL if self.PROXY_URL else None
)

if not self._validate_premium():
Expand All @@ -96,9 +96,6 @@ def _validate_settings(self) -> bool:
if not self.settings.api_key:
logger.warning("AllDebrid API key is not set")
return False
if self.settings.proxy_enabled and not self.settings.proxy_url:
logger.error("Proxy is enabled but no proxy URL is provided")
return False
return True

def _validate_premium(self) -> bool:
Expand Down
4 changes: 2 additions & 2 deletions src/program/services/downloaders/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,11 @@
# constraints for filesizes, follows the format tuple(min, max)
FILESIZE_MOVIE_CONSTRAINT: tuple[int, int] = (
movie_min_filesize if movie_min_filesize >= 0 else 0,
movie_max_filesize if movie_max_filesize >= 0 else float("inf")
movie_max_filesize if movie_max_filesize > 0 else float("inf")
)
FILESIZE_EPISODE_CONSTRAINT: tuple[int, int] = (
episode_min_filesize if episode_min_filesize >= 0 else 0,
episode_max_filesize if episode_max_filesize >= 0 else float("inf")
episode_max_filesize if episode_max_filesize > 0 else float("inf")
)


Expand Down
5 changes: 1 addition & 4 deletions src/program/services/downloaders/realdebrid.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ def validate(self) -> bool:

self.api = RealDebridAPI(
api_key=self.settings.api_key,
proxy_url=self.settings.proxy_url if self.settings.proxy_enabled else None
proxy_url=self.PROXY_URL if self.PROXY_URL else None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Migration path is already handled, but documentation needs attention

Based on the verification results:

  1. The code already has proper migration support:

    • test_settings_migration.py shows that old proxy settings are correctly migrated
    • The shared downloader base class properly reads from the centralized proxy settings
    • Settings model has been updated to support the new structure

  2. However, there's a documentation gap:

    • No proxy configuration documentation in README.md
    • No proxy-related changes documented in CHANGELOG.md
    • No environment variable documentation for proxy settings

Please add documentation for:

  • The new centralized proxy configuration in README.md
  • Migration notes in CHANGELOG.md
  • Any relevant environment variables
🔗 Analysis chain

Consider migration path and documentation updates.

The centralization of proxy settings is a good architectural improvement. However, there are a few considerations to ensure a smooth transition:

  1. Migration Guide: Users with existing per-debrid proxy configurations will need guidance on migrating to the new centralized configuration.
  2. Documentation: The PR checklist indicates documentation updates are pending. Please ensure the following are documented:
    • New proxy configuration approach
    • Updated environment variables
    • Migration steps for existing users
  3. Testing: The PR checklist also indicates tests are pending. Please ensure test coverage for:
    • Proxy configuration inheritance
    • Backward compatibility (if applicable)
    • Error cases

Let's verify the documentation and test requirements:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for documentation and test updates

# Check for documentation updates
echo "Checking for documentation files..."
fd -e md -e rst

# Check for test files
echo "Checking for test files..."
fd -g "*test*.py"

# Check for environment variable documentation
echo "Checking for environment variable documentation..."
fd -g ".env*"

Length of output: 944

Script:

#!/bin/bash
# Let's check for proxy-related changes and documentation

# Check for proxy-related changes in Python files
echo "Checking for proxy-related code changes..."
rg -l "proxy" --type py

# Check README.md content for proxy configuration
echo "Checking README for proxy documentation..."
rg -i "proxy" README.md

# Check for proxy-related test cases
echo "Checking for proxy-related tests..."
rg -i "proxy" src/tests/

# Check changelog for proxy-related updates
echo "Checking CHANGELOG.md for proxy updates..."
rg -i "proxy" CHANGELOG.md

Length of output: 1175

Script:

#!/bin/bash
# Let's examine the proxy-related code changes in more detail

# Check the shared downloader code for proxy implementation
echo "Checking shared downloader implementation..."
rg -A 5 "proxy" src/program/services/downloaders/shared.py

# Check settings model for proxy configuration
echo "Checking settings model..."
rg -A 5 "proxy" src/program/settings/models.py

# Check settings migration test details
echo "Checking settings migration test..."
cat src/tests/test_settings_migration.py

Length of output: 3194

)

return self._validate_premium()
Expand All @@ -110,9 +110,6 @@ def _validate_settings(self) -> bool:
if not self.settings.api_key:
logger.warning("Real-Debrid API key is not set")
return False
if self.settings.proxy_enabled and not self.settings.proxy_url:
logger.error("Proxy is enabled but no proxy URL is provided")
return False
return True

def _validate_premium(self) -> bool:
Expand Down
4 changes: 3 additions & 1 deletion src/program/services/downloaders/shared.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
from abc import ABC, abstractmethod
from datetime import datetime
from typing import List, Optional
from typing import Optional

from RTN import ParsedData, parse

Expand All @@ -9,10 +9,12 @@
TorrentContainer,
TorrentInfo,
)
from program.settings.manager import settings_manager


class DownloaderBase(ABC):
"""The abstract base class for all Downloader implementations."""
PROXY_URL: str = settings_manager.settings.downloaders.proxy_url

@abstractmethod
def validate(self) -> bool:
Expand Down
2 changes: 1 addition & 1 deletion src/program/services/downloaders/torbox.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ def validate(self) -> bool:

self.api = TorBoxAPI(
api_key=self.settings.api_key,
proxy_url=self.settings.proxy_url if self.settings.proxy_enabled else None
proxy_url=self.PROXY_URL if self.PROXY_URL else None
)

return self._validate_premium()
Expand Down
8 changes: 1 addition & 7 deletions src/program/settings/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,31 +43,25 @@ def __exit__(self_, exc_type, exc_value, traceback):
class RealDebridModel(Observable):
enabled: bool = False
api_key: str = ""
proxy_enabled: bool = False
proxy_url: str = ""


class AllDebridModel(Observable):
enabled: bool = False
api_key: str = ""
proxy_enabled: bool = False
proxy_url: str = ""


class TorboxModel(Observable):
enabled: bool = False
api_key: str = ""
proxy_enabled: bool = False
proxy_url: str = ""


class DownloadersModel(Observable):
video_extensions: List[str] = ["mp4", "mkv", "avi"]
prefer_speed_over_quality: bool = True
movie_filesize_mb_min: int = 0 # MB
movie_filesize_mb_max: int = -1 # MB (-1 is no limit)
episode_filesize_mb_min: int = 0 # MB
episode_filesize_mb_max: int = -1 # MB (-1 is no limit)
proxy_url: str = ""
Gaisberg marked this conversation as resolved.
Show resolved Hide resolved
real_debrid: RealDebridModel = RealDebridModel()
all_debrid: AllDebridModel = AllDebridModel()
torbox: TorboxModel = TorboxModel()
Expand Down
4 changes: 2 additions & 2 deletions src/routers/secure/default.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ async def get_rd_user() -> RDUser:
headers = {"Authorization": f"Bearer {api_key}"}

proxy = (
settings_manager.settings.downloaders.real_debrid.proxy_url
if settings_manager.settings.downloaders.real_debrid.proxy_enabled
settings_manager.settings.downloaders.proxy_url
if settings_manager.settings.downloaders.proxy_url
else None
)

Expand Down