From ef903c86d135e2665ed8b7954c1fdbcf900d6ab0 Mon Sep 17 00:00:00 2001 From: ComputerGuru Date: Sun, 31 May 2026 19:21:43 -0700 Subject: [PATCH] fix: wire settings.json to actually drive runtime behavior Settings page saved to /config/settings.json but nothing downstream read that file. Schedule changes were silently ignored; max_quality and sleep_interval changes were silently ignored. The "Settings saved successfully" flash was a lie. Fix: - sync.sh reads max_quality + sleep_interval from settings.json on each run (jq -er ... // empty, falling back to env vars on missing/malformed file) - entrypoint.sh reads sync_schedule from settings.json before setting up cron, and writes the crond PID to /var/run/crond.pid so Flask can SIGHUP it - app.py adds apply_schedule(): rewrites /etc/crontabs/root, signals crond via the recorded PID, restarts crond if the PID is stale, drops the crontab when schedule is set to "manual". save_settings_route invokes it only when the schedule actually changed; any failure flashes a warning so the save still succeeds with the user informed - bare `except: pass` in get_settings replaced with explicit exception types + stderr warning so debugging malformed settings is possible - sync.sh: one bad channel no longer aborts the whole loop under set -e - Dockerfile adds jq for the JSON reads in sync.sh / entrypoint.sh - README: two stale github.com URLs fixed to Gitea; new Running Tests section under Building From Source - tests/test_settings.py: 3 pytest cases covering get_settings()'s three branches (missing file, valid file, malformed JSON) Settings hierarchy unchanged: env-var defaults seed the UI; settings.json wins when present and parseable. Timezone (TZ) is not applied live - tzdata is locked in at process start. Same behavior as before; not in scope for this commit. --- Dockerfile | 1 + README.md | 14 +++++- app.py | 105 ++++++++++++++++++++++++++++++++++++++++- entrypoint.sh | 12 +++++ requirements-dev.txt | 1 + sync.sh | 18 ++++++- tests/__init__.py | 0 tests/test_settings.py | 70 +++++++++++++++++++++++++++ 8 files changed, 216 insertions(+), 5 deletions(-) create mode 100644 requirements-dev.txt create mode 100644 tests/__init__.py create mode 100644 tests/test_settings.py diff --git a/Dockerfile b/Dockerfile index 2d32821..56c2771 100644 --- a/Dockerfile +++ b/Dockerfile @@ -7,6 +7,7 @@ RUN apk add --no-cache \ ffmpeg \ bash \ curl \ + jq \ tzdata \ dcron \ && pip3 install --no-cache-dir yt-dlp flask --break-system-packages diff --git a/README.md b/README.md index 35d5c8d..d4a14c8 100644 --- a/README.md +++ b/README.md @@ -175,14 +175,24 @@ YouTube is blocking downloads. Solution: Add cookies.txt (see Configuration sect ## Building From Source ```bash -git clone https://github.com/azcomputerguru/youtube-sync-docker +git clone https://git.azcomputerguru.com/azcomputerguru/youtube-sync-docker cd youtube-sync-docker docker build -t youtube-sync . ``` +### Running Tests + +Tests are dev-only and not bundled into the production image. Install the +dev dependencies and run pytest from the repo root: + +```bash +pip install -r requirements-dev.txt +pytest tests/ +``` + ## Support -Issues and pull requests: https://github.com/azcomputerguru/youtube-sync-docker +Issues and pull requests: https://git.azcomputerguru.com/azcomputerguru/youtube-sync-docker ## License diff --git a/app.py b/app.py index f55cc93..ba49a4e 100644 --- a/app.py +++ b/app.py @@ -5,7 +5,9 @@ Provides a web UI for managing YouTube channel downloads """ import os +import signal import subprocess +import sys import json import re from datetime import datetime @@ -23,6 +25,8 @@ CHANNELS_FILE = os.path.join(CONFIG_DIR, 'channels.txt') COOKIES_FILE = os.path.join(CONFIG_DIR, 'cookies.txt') SETTINGS_FILE = os.path.join(CONFIG_DIR, 'settings.json') LOG_FILE = '/var/log/youtube-sync.log' +CRONTAB_FILE = '/etc/crontabs/root' +CROND_PID_FILE = '/var/run/crond.pid' def get_settings(): """Load settings from file or return defaults""" @@ -37,8 +41,14 @@ def get_settings(): try: with open(SETTINGS_FILE, 'r') as f: return json.load(f) - except: - pass + except (OSError, json.JSONDecodeError) as e: + # Malformed or unreadable settings.json: log and fall back to defaults + # rather than crashing the UI. The user can re-save from the Settings page + # to rewrite a clean file. + print( + f"[WARNING] Could not read {SETTINGS_FILE}: {e}; using env-var defaults", + file=sys.stderr, + ) return defaults @@ -47,6 +57,75 @@ def save_settings(settings): with open(SETTINGS_FILE, 'w') as f: json.dump(settings, f, indent=2) +def apply_schedule(new_schedule): + """ + Rewrite /etc/crontabs/root with the new schedule and reload crond so the + change takes effect without a container restart. + + - new_schedule == "manual": remove the crontab entirely. + - otherwise: write a single line invoking /app/sync.sh on the given schedule. + + crond is reloaded by SIGHUP'ing the PID recorded by entrypoint.sh. If crond + isn't running (e.g. container started in manual mode) and we now have a real + schedule, start it. + + Raises whatever the underlying OS calls raise; callers are expected to + catch and surface a warning to the user. + """ + if new_schedule == 'manual': + # Drop the crontab so crond stops firing the job. Leave crond running — + # cheaper than killing/restarting it and it'll just idle. + if os.path.exists(CRONTAB_FILE): + os.remove(CRONTAB_FILE) + else: + os.makedirs(os.path.dirname(CRONTAB_FILE), exist_ok=True) + line = f"{new_schedule} /app/sync.sh >> {LOG_FILE} 2>&1\n" + with open(CRONTAB_FILE, 'w') as f: + f.write(line) + + # Reload crond. dcron (Alpine) re-reads crontabs on SIGHUP. + pid = None + if os.path.exists(CROND_PID_FILE): + try: + with open(CROND_PID_FILE, 'r') as f: + pid = int(f.read().strip()) + except (OSError, ValueError) as e: + print( + f"[WARNING] Could not read crond pid file {CROND_PID_FILE}: {e}", + file=sys.stderr, + ) + pid = None + + if pid is not None: + try: + os.kill(pid, signal.SIGHUP) + return + except ProcessLookupError: + # PID file is stale — crond exited. Fall through to start it fresh + # below if we have a non-manual schedule. + print( + f"[WARNING] crond pid {pid} no longer running; restarting", + file=sys.stderr, + ) + pid = None + + # No live crond. Start one if we have an active schedule to run. + if new_schedule != 'manual': + proc = subprocess.Popen( + ['crond', '-f', '-l', '2'], + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + ) + try: + with open(CROND_PID_FILE, 'w') as f: + f.write(str(proc.pid)) + except OSError as e: + # Non-fatal: cron is running, we just can't reload it on the next change. + print( + f"[WARNING] crond started (pid {proc.pid}) but pid file write failed: {e}", + file=sys.stderr, + ) + def extract_channel_id(url_or_id): """ Extract channel ID from various YouTube URL formats or validate direct ID @@ -280,6 +359,7 @@ def settings(): @app.route('/settings/save', methods=['POST']) def save_settings_route(): """Save settings""" + previous = get_settings() settings = { 'sync_schedule': request.form.get('sync_schedule', '0 2 * * *'), 'max_quality': request.form.get('max_quality', '1080'), @@ -289,6 +369,27 @@ def save_settings_route(): save_settings(settings) flash('Settings saved successfully', 'success') + + # Apply the new cron schedule immediately if it changed. max_quality / + # sleep_interval are read fresh by sync.sh on each run, so no action needed + # for them. Timezone changes still require a container restart (system tzdata + # is locked in at container start); we don't claim otherwise. + if settings['sync_schedule'] != previous.get('sync_schedule'): + try: + apply_schedule(settings['sync_schedule']) + except (OSError, subprocess.SubprocessError) as e: + # Save succeeded; the schedule reload didn't. Let the user know + # it'll take effect on next restart rather than silently failing. + print( + f"[WARNING] Could not reload cron schedule: {e}", + file=sys.stderr, + ) + flash( + 'Schedule saved, but live reload failed. ' + 'New schedule will take effect after container restart.', + 'warning', + ) + return redirect(url_for('settings')) @app.route('/cookies', methods=['GET', 'POST']) diff --git a/entrypoint.sh b/entrypoint.sh index 800552c..344dd67 100644 --- a/entrypoint.sh +++ b/entrypoint.sh @@ -8,6 +8,16 @@ echo "YouTube Channel Sync Docker" echo "==========================================" echo "Download Directory: $DOWNLOAD_DIR" echo "Config Directory: $CONFIG_DIR" + +# settings.json (managed via the web UI) overrides the env-var defaults +# baked into the image / docker-compose. Same hierarchy as app.py:get_settings(). +SETTINGS_FILE="${CONFIG_DIR}/settings.json" +if [ -f "$SETTINGS_FILE" ]; then + if VAL=$(jq -er '.sync_schedule // empty' "$SETTINGS_FILE" 2>/dev/null); then + SYNC_SCHEDULE="$VAL" + fi +fi + echo "Sync Schedule: $SYNC_SCHEDULE" echo "Max Quality: ${MAX_QUALITY}p" echo "Sleep Interval: ${SLEEP_INTERVAL}s" @@ -45,6 +55,8 @@ if [ "$SYNC_SCHEDULE" != "manual" ]; then # Start crond in the background crond -f -l 2 & CRON_PID=$! + # Record PID so the Flask UI can SIGHUP crond when the schedule changes. + echo "$CRON_PID" > /var/run/crond.pid echo "[INFO] Cron daemon started (PID: $CRON_PID)" else echo "[INFO] Manual mode enabled. Use Web UI to trigger syncs." diff --git a/requirements-dev.txt b/requirements-dev.txt new file mode 100644 index 0000000..b197d32 --- /dev/null +++ b/requirements-dev.txt @@ -0,0 +1 @@ +pytest>=7.0 diff --git a/sync.sh b/sync.sh index abd9901..17b1755 100644 --- a/sync.sh +++ b/sync.sh @@ -8,9 +8,21 @@ DOWNLOAD_DIR="${DOWNLOAD_DIR:-/downloads}" CONFIG_DIR="${CONFIG_DIR:-/config}" COOKIES_FILE="${CONFIG_DIR}/cookies.txt" CHANNELS_FILE="${CONFIG_DIR}/channels.txt" +SETTINGS_FILE="${CONFIG_DIR}/settings.json" MAX_QUALITY="${MAX_QUALITY:-1080}" SLEEP_INTERVAL="${SLEEP_INTERVAL:-2}" +# Override env-var defaults with values from settings.json when present and parseable. +# Missing or malformed settings.json silently falls back to env-var defaults. +if [ -f "$SETTINGS_FILE" ]; then + if VAL=$(jq -er '.max_quality // empty' "$SETTINGS_FILE" 2>/dev/null); then + MAX_QUALITY="$VAL" + fi + if VAL=$(jq -er '.sleep_interval // empty' "$SETTINGS_FILE" 2>/dev/null); then + SLEEP_INTERVAL="$VAL" + fi +fi + # Check if channels file exists if [ ! -f "$CHANNELS_FILE" ]; then echo "[ERROR] Channels file not found: $CHANNELS_FILE" @@ -57,6 +69,9 @@ while IFS='|' read -r CHANNEL_ID CHANNEL_NAME || [ -n "$CHANNEL_ID" ]; do echo "Channel ID: $CHANNEL_ID" echo "==========================================" + # Don't let one bad channel kill the whole loop. yt-dlp can exit non-zero + # for individual videos even with --ignore-errors (e.g. age-gated, geo-blocked, + # member-only). We log and continue rather than aborting the entire sync. yt-dlp \ $COOKIES_PARAM \ --format "bestvideo[ext=mp4][height<=${MAX_QUALITY}]+bestaudio[ext=m4a]/best[ext=mp4]/best" \ @@ -73,7 +88,8 @@ while IFS='|' read -r CHANNEL_ID CHANNEL_NAME || [ -n "$CHANNEL_ID" ]; do --ignore-errors \ --sleep-interval "$SLEEP_INTERVAL" \ --max-sleep-interval 5 \ - "https://www.youtube.com/channel/$CHANNEL_ID/videos" + "https://www.youtube.com/channel/$CHANNEL_ID/videos" \ + || echo "[WARNING] channel $CHANNEL_NAME ($CHANNEL_ID) failed; continuing with remaining channels" # Create tvshow.nfo for Emby/Plex echo "" diff --git a/tests/__init__.py b/tests/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/test_settings.py b/tests/test_settings.py new file mode 100644 index 0000000..49459d9 --- /dev/null +++ b/tests/test_settings.py @@ -0,0 +1,70 @@ +""" +Tests for the settings hierarchy in app.get_settings(). + +Hierarchy under test: env-var defaults are returned when settings.json is +missing or malformed; the file wins when it exists and parses cleanly. +""" + +import json +import os +import sys +import tempfile + +import pytest + +# Make the app module importable without installing it. +sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) + +import app as app_module # noqa: E402 + + +@pytest.fixture +def tmp_settings_path(monkeypatch, tmp_path): + """Point app.SETTINGS_FILE at a temp path for the duration of the test.""" + path = tmp_path / "settings.json" + monkeypatch.setattr(app_module, "SETTINGS_FILE", str(path)) + return path + + +def test_get_settings_returns_defaults_when_file_missing(tmp_settings_path, monkeypatch): + monkeypatch.setenv("SYNC_SCHEDULE", "5 5 * * *") + monkeypatch.setenv("MAX_QUALITY", "720") + monkeypatch.setenv("SLEEP_INTERVAL", "7") + monkeypatch.setenv("TZ", "UTC") + assert not tmp_settings_path.exists() + + result = app_module.get_settings() + + assert result == { + "sync_schedule": "5 5 * * *", + "max_quality": "720", + "sleep_interval": "7", + "timezone": "UTC", + } + + +def test_get_settings_returns_file_contents_when_present(tmp_settings_path): + payload = { + "sync_schedule": "0 4 * * *", + "max_quality": "1440", + "sleep_interval": "3", + "timezone": "America/Phoenix", + } + tmp_settings_path.write_text(json.dumps(payload)) + + assert app_module.get_settings() == payload + + +def test_get_settings_falls_back_on_malformed_json(tmp_settings_path, monkeypatch): + monkeypatch.setenv("SYNC_SCHEDULE", "0 2 * * *") + monkeypatch.setenv("MAX_QUALITY", "1080") + monkeypatch.setenv("SLEEP_INTERVAL", "2") + monkeypatch.setenv("TZ", "America/Phoenix") + tmp_settings_path.write_text("{ this is not valid json ") + + result = app_module.get_settings() + + assert result["sync_schedule"] == "0 2 * * *" + assert result["max_quality"] == "1080" + assert result["sleep_interval"] == "2" + assert result["timezone"] == "America/Phoenix"