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

Move to more variables #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Move to more variables #4

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
8 changes: 4 additions & 4 deletions startup.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,10 @@

SUBSONIC_HOME=/var/subsonic
SUBSONIC_HOST=0.0.0.0
SUBSONIC_PORT=4040
SUBSONIC_HTTPS_PORT=0
SUBSONIC_CONTEXT_PATH=/
SUBSONIC_MAX_MEMORY=200
SUBSONIC_PORT=${HTTP_PORT}
Copy link
Owner

Choose a reason for hiding this comment

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

Where do these variables come from? Where is "CONTEXT_PATH" defined, for example?

Copy link
Contributor Author

@jthoel jthoel Jan 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

They get passed during the docker runtime. For example, when I made the changes to my local copy, I was able to run the following to pass the URL change and change the memory. Adding the http port and https port lets users do some more customization

docker run -d -p 4040:4040 \
 -v /var/lib/docker/volumes/subsonic/data:/var/subsonic \
 -v /mnt/music/:/music \
 -v /mnt/audiobooks/:/audiobooks \
 -e MAX_MEM=1024 \
 -e CONTEXT_PATH=/subsonic \
 --name=subsonic mpaganini/alpine-subsonic:latest

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, but then we need to run the docker run with specific commands or things will fail. Ideally, we would be able to override those, but it should run with sane defaults if not specified.

Copy link

@HewittJC HewittJC Mar 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

What if we just posted a docker-compose along side this as well. I'd be willing to add mine as a PR

To your point about things having to be set, you already have to set things or it wont work (i.e. volumes)

To give some context to why this would be good, I have a setup with a NextCloud server running in Docker along side the subsonic server. I would like to be able to mount things from my NextCloud into Subsonic. I currently have this working but until I rebuild the container so that the UID's match for the subsonic user and the www-data user in NextCloud, I have to do a strange permissions dance with every change to the files

Copy link
Owner

Choose a reason for hiding this comment

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

I grant the point that we already have to pass something to docker run (for the volume).

Another option would be to keep the ENV statements with sane default. This way, users can override only what they want with the docker run -e var=value command.

For example, users won't typically have to override HTTPS_PORT. We could do ENV HTTPS_PORT=443 inside the file. This would allow most users to just use docker run with the regular parameters without worrying about defining this port. If they need to redefine HTTPS_PORT, all they need to do is run docker run -e HTTPS_PORT=443 ... and we're set.

Does it make sense?

Copy link

Choose a reason for hiding this comment

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

Yea I think this is fair 👍 👌

SUBSONIC_HTTPS_PORT=${HTTPS_PORT}
Copy link
Owner

Choose a reason for hiding this comment

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

Now that we define these three environment variables based on other variables, we have two choices:

  1. Define default HTTPS_PORT, CONTEXT_PATH, and MAX_MEM with an ENV in the Dockerfile, so we don't have to type them when using docker run.

  2. Define defaults here, like:

SUBSONIC_HTTPS_PORT=${HTTPS_PORT:-0}
SUBSONIC_CONTEXT_PATH=${CONTEXT_PATH:-/}
SUBSONIC_MAX_MEMORY=${MAX_MEM:-200}

This would use shell constructs to set defaults if those variables are not set.

I'm OK with either. Just pick one of the two methods above (and if you have a few minutes to spare, run a quick test to make sure it works as expected). :)

SUBSONIC_CONTEXT_PATH=${CONTEXT_PATH}
SUBSONIC_MAX_MEMORY=${MAX_MEM}
SUBSONIC_PIDFILE=
SUBSONIC_DEFAULT_MUSIC_FOLDER=/var/music
SUBSONIC_DEFAULT_PODCAST_FOLDER=${SUBSONIC_HOME}/podcasts
Expand Down