-
-
Notifications
You must be signed in to change notification settings - Fork 75
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.
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
Add MyMPD Service #1776
base: master
Are you sure you want to change the base?
Add MyMPD Service #1776
Conversation
WalkthroughThis pull request introduces two new configuration files for the mympd application. The Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
Apps/mympd/docker-compose.yml (1)
39-40
: Consider adding a thumbnail image.The thumbnail URL is empty. Consider adding one to improve the visual appearance in the CasaOS UI.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- Apps/mympd/config.json (1 hunks)
- Apps/mympd/docker-compose.yml (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Apps/mympd/config.json
🧰 Additional context used
🔇 Additional comments (2)
Apps/mympd/docker-compose.yml (2)
9-14
: Document required environment variables and consider security implications.
MPD_HOST
andMPD_PORT
are required but left empty- SSL is disabled by default, which might not be secure for remote access
Consider:
- Adding default values for MPD connection:
- - MPD_HOST= - - MPD_PORT= + - MPD_HOST=${MPD_HOST:-localhost} + - MPD_PORT=${MPD_PORT:-6600}
- Making SSL configurable based on deployment environment:
- - MYMPD_SSL=false + - MYMPD_SSL=${MYMPD_SSL:-false}
23-23
: LGTM!The "unless-stopped" restart policy is appropriate for a persistent service.
fixed version
fixed name
fixed volumes to use DATA folder
fixed container name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
Apps/mympd/docker-compose.yml (1)
37-40
: Update author and provide thumbnail URL.A few metadata improvements needed:
- Author can be changed to your username or "BigBearCommunity" as suggested
- Consider providing a thumbnail URL for better UI appearance
- author: BigBearTechWorld + author: BigBearCommunity - thumbnail: "" + thumbnail: "https://jcorporation.github.io/myMPD/assets/mympd-logo.svg"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
Apps/mympd/config.json
(1 hunks)Apps/mympd/docker-compose.yml
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Apps/mympd/config.json
🔇 Additional comments (4)
Apps/mympd/docker-compose.yml (4)
3-5
: Update service and container names for consistency.
Previous review comments requested using 'big-bear-mympd'. Let's maintain consistency:
- mympd:
+ big-bear-mympd:
image: ghcr.io/jcorporation/mympd/mympd:18.2.2
- container_name: mympd
+ container_name: big-bear-mympd
6-8
: Make runtime settings configurable.
For better flexibility and to avoid conflicts:
ports:
- - 8080:8080
+ - ${MYMPD_PORT:-8080}:8080
- user: 1000:1000
+ user: ${MYMPD_UID:-1000}:${MYMPD_GID:-1000}
15-22
: Update volume paths to follow CasaOS conventions.
As per previous comments, volume paths should use /DATA/AppData/$AppID/
:
volumes:
- /run/mpd:/run/mpd
## Optional for myGPIOd support
## - /run/mygpiod:/run/mygpiod
- - /docker/mympd/workdir:/var/lib/mympd
- - /docker/mympd/cachedir:/var/cache/mympd
+ - /DATA/AppData/big-bear-mympd/workdir:/var/lib/mympd
+ - /DATA/AppData/big-bear-mympd/cachedir:/var/cache/mympd
- /var/lib/mpd/music:/var/lib/mpd/music:ro
- /var/lib/mpd/playlists:/var/lib/mpd/playlists:ro
33-34
: Remove comment marker from description.
The description contains a comment marker that might cause YAML parsing issues:
description:
- en_us: myMPD is a standalone and mobile friendly web-based MPD client with a tiny footprint and advanced features.# Description in English
+ en_us: myMPD is a standalone and mobile friendly web-based MPD client with a tiny footprint and advanced features.
fixed author name
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
change https to http
fixed ports, user and environment variables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe I reviewed everything suggested.
Thanks for the patience. I hadn't much time the last few months.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
Apps/mympd/docker-compose.yml (2)
37-37
: Improve the tagline description.The current tagline "mpd" is too brief and not descriptive enough. Consider providing a more informative tagline that briefly describes the service's purpose.
tagline: - en_us: mpd # Short description or tagline in English + en_us: Web-based Music Player Daemon client # Short description or tagline in English
41-41
: Add a thumbnail URL.The empty thumbnail URL might affect the UI display. Consider adding a proper thumbnail URL or removing the field if not required.
- thumbnail: "" # Thumbnail image URL + thumbnail: "https://jcorporation.github.io/myMPD/assets/mympd-logo.svg" # Using the same URL as icon for now
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Apps/mympd/docker-compose.yml
(1 hunks)
🔇 Additional comments (2)
Apps/mympd/docker-compose.yml (2)
1-8
: LGTM! Service configuration follows best practices.
The service configuration is well-structured with:
- Pinned image version
- Consistent naming convention
- Properly parameterized port and user configurations
16-23
: Verify $AppID variable availability.
The volume paths depend on the $AppID
variable, but there's no validation or default value provided. This could cause mounting issues if the variable is not set.
✅ Verification successful
The previous search was too broad. Let's focus on the specific app directory to understand how the AppID variable is handled in this context.
Let's check if there's any global configuration or deployment system that might be setting the AppID variable.
Let's check the repository root for any documentation or configuration that explains how the AppID variable is handled in this deployment system.
Based on the gathered information, I can now provide a final response:
$AppID variable is part of CasaOS deployment system
The repository is a CasaOS App Store, which is a collection of app configurations for the CasaOS platform. The $AppID variable is a standard CasaOS environment variable that gets automatically set by the CasaOS system during deployment. This is a common pattern across all apps in this store, as evidenced by the repository structure and documentation.
- The volume paths using
/DATA/AppData/$AppID/
are correct and follow the CasaOS convention - The variable is handled by the CasaOS platform itself, not within individual app configurations
- This is part of a larger deployment system where CasaOS manages the app lifecycle and environment variables
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if $AppID is defined in the environment or other configuration files
rg -l "AppID=" $(fd -t f "docker-compose.yml|.env")
# Check if there are any other services using similar path pattern
rg "/DATA/AppData/\$AppID" $(fd -t f "docker-compose.yml")
Length of output: 11213
Script:
#!/bin/bash
# Let's check for any .env files in the mympd app directory and its parent directories
fd -t f ".env" Apps/mympd/
# Also check for environment variable definitions in the docker-compose file
rg "environment:|env_file:" Apps/mympd/docker-compose.yml -A 5
# Check if this is part of a larger deployment system
cat Apps/mympd/README.md 2>/dev/null || true
Length of output: 312
Script:
#!/bin/bash
# Let's check the parent directory for deployment-related files
ls -la Apps/
# Check if there's a global .env file at the root
ls -la .env 2>/dev/null || true
# Look for any documentation about deployment or configuration
fd -t f "README|INSTALL|DEPLOY|CONFIG" . -d 1
Length of output: 10170
Script:
#!/bin/bash
# Let's check the root README.md for deployment instructions
cat README.md
# Check if there's any global configuration in the repository root
fd -t f "config|\.env" . -d 1 -x cat {}
# Look for any deployment scripts that might set AppID
fd -t f "deploy|setup|install" . -d 1
Length of output: 17632
environment: | ||
- UMASK_SET=022 | ||
- MYMPD_SSL=false | ||
- MYMPD_HTTP_PORT=${MYMPD_HTTP_PORT:-8080} | ||
- MPD_HOST=${MPD_HOST:-localhost} | ||
- MPD_PORT=${MPD_PORT:-6600} | ||
- MYMPD_UID=1000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix inconsistent UID configuration.
There's an inconsistency in the UID configuration:
- Line 8 uses parameterized
${MYMPD_UID:-1000}
- Line 15 hardcodes
MYMPD_UID=1000
This could lead to confusion and potential issues.
Remove the hardcoded MYMPD_UID:
environment:
- UMASK_SET=022
- MYMPD_SSL=false
- MYMPD_HTTP_PORT=${MYMPD_HTTP_PORT:-8080}
- MPD_HOST=${MPD_HOST:-localhost}
- MPD_PORT=${MPD_PORT:-6600}
- - MYMPD_UID=1000
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
environment: | |
- UMASK_SET=022 | |
- MYMPD_SSL=false | |
- MYMPD_HTTP_PORT=${MYMPD_HTTP_PORT:-8080} | |
- MPD_HOST=${MPD_HOST:-localhost} | |
- MPD_PORT=${MPD_PORT:-6600} | |
- MYMPD_UID=1000 | |
environment: | |
- UMASK_SET=022 | |
- MYMPD_SSL=false | |
- MYMPD_HTTP_PORT=${MYMPD_HTTP_PORT:-8080} | |
- MPD_HOST=${MPD_HOST:-localhost} | |
- MPD_PORT=${MPD_PORT:-6600} |
Added MyMPD Service.
MyMPD is a client for the music player daemon. You can fetch lyrics, listen with httpd stream, create scripts… As it is a webclient, it is accessible in any device.
MyMPD: github.com/jcorporation/myMPD
MyMPD documentation on Docker: jcorporation.github.io/myMPD/010-installation/docker/
On my setup, it connects to MPD running on system-level. With proper configuration, can connect to any MPD instance on the network/web.
Summary by CodeRabbit
New Features
Documentation