Conversation
Co-authored-by: Blueion76 <128919662+Blueion76@users.noreply.github.com>
Co-authored-by: Blueion76 <128919662+Blueion76@users.noreply.github.com>
…ource gating, URL validation Co-authored-by: Blueion76 <128919662+Blueion76@users.noreply.github.com>
…rter Add Spotify and Deezer playlist import support
There was a problem hiding this comment.
Pull request overview
Adds optional Spotify and Deezer playlist import functionality to OctoGen, enabling users to import external playlists into Navidrome via new importer clients and env-var configuration.
Changes:
- Add Spotify and Deezer importer clients and wire them into the main orchestration flow.
- Extend environment configuration + validation to support enabling imports and specifying playlist URLs.
- Update documentation and dependencies for the new import features.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| requirements.txt | Adds Spotipy and Deezer client dependencies for playlist import support. |
| octogen/main.py | Initializes importers from env config, validates config, and runs Spotify/Deezer import during regular playlist generation. |
| octogen/config.py | Extends config loader to include Spotify/Deezer env vars (secrets-aware). |
| octogen/api/spotify.py | New Spotipy-based importer for fetching playlist tracks. |
| octogen/api/deezer.py | New Deezer client-based importer for fetching playlist tracks. |
| ENV_VARS.md | Documents new Spotify/Deezer import environment variables and updates “music source” notes/totals. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "spotify": { | ||
| "enabled": self._get_env_bool("SPOTIFY_IMPORT_ENABLED", False), | ||
| "client_id": os.getenv("SPOTIFY_CLIENT_ID", ""), | ||
| "client_secret": os.getenv("SPOTIFY_CLIENT_SECRET", ""), | ||
| "playlist_urls": os.getenv("SPOTIFY_PLAYLIST_URLS", ""), |
There was a problem hiding this comment.
Spotify credentials are read via os.getenv here, which bypasses the repo’s Docker secrets helper (octogen.utils.secrets.load_secret) that is used elsewhere (e.g., octogen/config.py). If docs claim Docker secrets support for SPOTIFY_CLIENT_ID/SPOTIFY_CLIENT_SECRET, update this loader to use load_secret (or adjust the documentation so it matches the actual behavior).
| "spotify": { | ||
| "enabled": os.getenv("SPOTIFY_IMPORT_ENABLED", "false").lower() == "true", | ||
| "client_id": load_secret("SPOTIFY_CLIENT_ID"), | ||
| "client_secret": load_secret("SPOTIFY_CLIENT_SECRET"), | ||
| "playlist_urls": os.getenv("SPOTIFY_PLAYLIST_URLS", ""), | ||
| }, | ||
| "deezer": { | ||
| "enabled": os.getenv("DEEZER_IMPORT_ENABLED", "false").lower() == "true", | ||
| "playlist_urls": os.getenv("DEEZER_PLAYLIST_URLS", ""), | ||
| }, |
There was a problem hiding this comment.
load_config_from_env() now returns spotify/deezer config blocks, but validate_config() / OctoGenConfig don’t model or validate these fields, so they’ll be silently ignored by the Pydantic validation path. Either extend the config models + validate_config() to include Spotify/Deezer settings, or keep these settings out of this config loader to avoid inconsistent behavior across entrypoints.
| "spotify": { | |
| "enabled": os.getenv("SPOTIFY_IMPORT_ENABLED", "false").lower() == "true", | |
| "client_id": load_secret("SPOTIFY_CLIENT_ID"), | |
| "client_secret": load_secret("SPOTIFY_CLIENT_SECRET"), | |
| "playlist_urls": os.getenv("SPOTIFY_PLAYLIST_URLS", ""), | |
| }, | |
| "deezer": { | |
| "enabled": os.getenv("DEEZER_IMPORT_ENABLED", "false").lower() == "true", | |
| "playlist_urls": os.getenv("DEEZER_PLAYLIST_URLS", ""), | |
| }, |
|
|
||
| import logging | ||
| import re | ||
| from typing import List, Dict, Optional |
There was a problem hiding this comment.
Optional is imported but not used in this module. Removing unused imports helps keep the code clean and avoids lint failures if/when linting is enabled.
| from typing import List, Dict, Optional | |
| from typing import List, Dict |
| def get_playlist_tracks(self, playlist_id_or_url: str) -> List[Dict]: | ||
| """Fetch all tracks from a Spotify playlist. | ||
|
|
||
| Args: | ||
| playlist_id_or_url: Spotify playlist URL or bare playlist ID | ||
|
|
||
| Returns: | ||
| List of dicts with 'artist' and 'title' keys | ||
| """ | ||
| playlist_id = self.extract_playlist_id(playlist_id_or_url) | ||
| tracks: List[Dict] = [] | ||
|
|
||
| try: | ||
| results = self.sp.playlist_items( | ||
| playlist_id, | ||
| fields="items(track(name,artists(name))),next", | ||
| limit=100, | ||
| ) | ||
|
|
||
| while results: | ||
| for item in results.get("items", []): | ||
| track = item.get("track") | ||
| if not track: | ||
| continue | ||
|
|
||
| title = (track.get("name") or "").strip() | ||
| artists = track.get("artists") or [] | ||
| artist = (artists[0].get("name") or "").strip() if artists else "" | ||
|
|
||
| if not artist or not title: | ||
| continue | ||
|
|
||
| tracks.append({"artist": artist, "title": title}) | ||
|
|
||
| next_url = results.get("next") | ||
| if next_url: | ||
| results = self.sp.next(results) | ||
| else: | ||
| break |
There was a problem hiding this comment.
get_playlist_tracks() fetches the entire playlist via pagination, but the caller only imports up to 100 tracks (create_playlist(..., max_songs=100)). Consider adding a max_tracks/limit parameter and breaking the pagination loop once enough tracks are collected to reduce API calls and runtime for very large playlists.
| def get_playlist_tracks(self, playlist_id_or_url: str) -> List[Dict]: | ||
| """Fetch all tracks from a Deezer playlist. | ||
|
|
||
| Args: | ||
| playlist_id_or_url: Deezer playlist URL or bare playlist ID (int or str) | ||
|
|
||
| Returns: | ||
| List of dicts with 'artist' and 'title' keys | ||
| """ | ||
| playlist_id = self.extract_playlist_id(str(playlist_id_or_url)) | ||
| tracks: List[Dict] = [] | ||
|
|
||
| try: | ||
| playlist = self.client.get_playlist(int(playlist_id)) | ||
| for track in playlist.tracks: | ||
| title = (getattr(track, "title", None) or "").strip() | ||
| artist_obj = getattr(track, "artist", None) | ||
| artist = (getattr(artist_obj, "name", None) or "").strip() if artist_obj else "" | ||
|
|
||
| if not artist or not title: | ||
| continue | ||
|
|
||
| tracks.append({"artist": artist, "title": title}) | ||
|
|
There was a problem hiding this comment.
Similar to Spotify import, this method collects all tracks from a playlist, but the orchestration code only imports up to 100. Consider accepting a max_tracks parameter (defaulting to 100) and stopping once reached to avoid unnecessary API work for large playlists.
| SPOTIFY_CLIENT_SECRET=xyz789... | ||
| ``` | ||
| **Notes**: | ||
| - Supports Docker secrets |
There was a problem hiding this comment.
The docs state SPOTIFY_CLIENT_SECRET “Supports Docker secrets”, but the main runtime config loader (OctoGenEngine._load_config_from_env in octogen/main.py) reads Spotify credentials via os.getenv and won’t pick up /run/secrets/*. Either update the code to use the secrets helper for Spotify credentials or adjust this note so documentation matches behavior.
| - Supports Docker secrets |
|
@copilot apply changes based on the comments in this thread |
…potify/Deezer config models Agent-Logs-Url: https://github.com/Blueion76/OctoGen/sessions/166b764d-e0bf-48db-9c57-4a75f43df148 Co-authored-by: Blueion76 <128919662+Blueion76@users.noreply.github.com>
Applied all changes from the review thread in commit
|
No description provided.