-
-
Notifications
You must be signed in to change notification settings - Fork 80
Docker deployment of tidal-dl-ng #341
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
base: master
Are you sure you want to change the base?
Conversation
…DME.md to build and run a container that has tidal-dl-ng installed with ffmpeg and path setup in settings.json.
Great idea, thank you very much for this PR. To be honest: In my opinion it is not really efficient to keep a separate |
…ned folder creation a bit as well as removed now-useless settings.json from the repo.
I modified the |
Dockerfile
Outdated
RUN apt update | ||
|
||
# Installing nano just in case one need to edit something in the container, installing ffmpeg for tidal-dl-ng | ||
RUN apt install -y nano ffmpeg |
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.
Why do you need to install nano
? This container is hopefully stable and nobody needs to edit anything in it. I would not recommend to install nano to keep the container as small as possible.
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.
Indeed it is useless, it is a leftover code, from when I was tinkering, that I forgot to remove. I changed the Dockerfile accordingly, thanks for pointing that out! I also wonder if the modifications I made in the README.md are fitting with the rest of it.
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.
Great idea. Just some details needs to be tackled.
Dockerfile
Outdated
RUN pip install --upgrade tidal-dl-ng | ||
|
||
# Creating a user appuser to group users | ||
RUN useradd -m -u 1000 -g users appuser |
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.
Use -d </home/dir>
to create the home directory.
Dockerfile
Outdated
USER appuser | ||
|
||
# Configuring ffmpeg and deownload path | ||
RUN tidal-dl-ng cfg path_binary_ffmpeg /usr/bin/ffmpeg |
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.
Would not it be better to use something like $(which ffmpeg)
instead of hard coding the FFmpeg path?
Dockerfile
Outdated
|
||
# Configuring ffmpeg and deownload path | ||
RUN tidal-dl-ng cfg path_binary_ffmpeg /usr/bin/ffmpeg | ||
RUN tidal-dl-ng cfg download_base_path /home/appuser/music |
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.
Why don't you use variables (https://docs.docker.com/build/building/variables/) instead of path repetitions?
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.
Please avoid value repetition, like /home/appuser
, appuser
etc. and use variables instead (https://docs.docker.com/build/building/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.
It would definitely make sense to combine the two docker sections into one, otherwise it is confusing for non skilled users. What do you think?
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.
Do you think making Docker its own section is best ? I'm afraid that the README.md would be repetitive with: pip installation then pip usage then docker installation then docker usage. If you have an idea on how to arrange better the README.md, please let me know, I will take those changes into account.
README.md
Outdated
### 🐋 Using the Docker image | ||
Simply create a music folder to mount to the container you will create, then run : | ||
```bash | ||
docker run -v "/path/to/host/music/folder:/home/appuser/music" -v tidal-dl-volume:/home/appuser/.config/tidal_dl_ng/ -it <container-image-name>:latest <command> |
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.
By <command>
you mean any tdial-dl-ng
command, do not you? May tdn <command>
would be a better way to express it.
|
||
⚠️ The folder from the host that you map in the container must exist, if Docker creates it it might be owned by ```root``` and tidal-dl-ng **will not have the rights** to write in this folder ⚠️ | ||
|
||
💡 You can also run a bash shell if you want to tinker in the container. Nano and ffmpeg are installed and the ffmpeg path is preconfigured in the ```settings.json```. |
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.
Does it make sense to map the config directory also to a local directory instead only to have it within the Docker container?
…how to cleanly separate pip and docker sections
I made the modifications you suggested (which made the Dockerfile much better honestly), let me know if you want anything else changed. |
… user's home, creating music folder as user, using '-m' in useradd to create its home dir
Dockerfile
Outdated
# Creating music folder | ||
RUN mkdir -p ${MUSIC_PATH} | ||
|
||
# Configuring ffmpeg and deownload path fir tidal-dl-ng |
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.
# Configuring ffmpeg and deownload path fir tidal-dl-ng | |
# Configuring ffmpeg and download path fir tidal-dl-ng |
found a typo
ENV UID="1000" | ||
|
||
# User's home path | ||
ENV HOME_PATH="/home/appuser" |
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.
Please still avoid value repetitions. Here: appuser
@@ -42,6 +42,9 @@ If you like this projects and want to support it, feel free to buy me a coffee | |||
|
|||
## 💻 Installation / Upgrade | |||
|
|||
|
|||
### 🐍 Installing with pip |
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.
A blank is missing after ###
.
## ⌨️ Usage | ||
|
||
### 🐍 Using pip |
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.
This is misleading. Nobody is using pip here.
@@ -50,8 +53,19 @@ pip install --upgrade tidal-dl-ng | |||
pip install --upgrade tidal-dl-ng[gui] | |||
``` | |||
|
|||
### 🐋 Building the Docker image | |||
**Requirements**: Docker == 27.5.0 (other verions might work but are not tested!) | |||
All you need is the ```Dockerfile``` file from this repository and be in the same directory as it. |
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.
Why would you state this, if this file is already there?
All you need is the ```Dockerfile``` file from this repository and be in the same directory as it. | ||
To build the image using ```docker build``` command: | ||
```bash | ||
docker build -t <container-image-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.
Please replace <container-image-name>
with a suitable name to avoid further errors by the user.
```bash | ||
docker build -t <container-image-name> . | ||
``` | ||
You can give any name you want to the container. The Dockerfile should be easily modified to fit your needs but works great as is. |
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.
With a predefined name, you can omit this sentence. Let's keep this readme tight.
I created a simple Dockerfile starting from a Python Image to build a container with tidal-dl-ng installed and settings.json configured with ffmpeg path. Also modified the README.md in consequence to add instructions on how to build the container and how to run it, trying to make it as simple as possible. I hope this addition can be useful to some people and that this PR is done properly. Feel free to educate me on what to modify to do this better if needed :)