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

feat(deployment): no env_file #14889

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat(deployment): no env_file #14889

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3f13ef9
noenv
mmomjian Dec 23, 2024
ecf19a9
noenv2
mmomjian Dec 23, 2024
502b89c
fix TZ
mmomjian Dec 29, 2024
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
9 changes: 5 additions & 4 deletions docker/docker-compose.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,11 @@ services:
# Do not edit the next line. If you want to change the media storage location on your system, edit the value of UPLOAD_LOCATION in the .env file
- ${UPLOAD_LOCATION}:/usr/src/app/upload
- /etc/localtime:/etc/localtime:ro
env_file:
- .env
environment:
mmomjian marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise, I think it's confusing to have a .env file that only applies these variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that’s just how Docker env works, so I would argue our current approach is MORE confusing. It’s caused a lot of issue for people especially with stuff like Portainer

Copy link
Contributor

Choose a reason for hiding this comment

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

If there's a .env file, I expect it to apply to the containers in the Compose file. It's surprising for it to sometimes do this and sometimes not depending on which variable I'm adding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then perhaps it would be better to deprecate the .env entirely when we move to the get.immich generator and provide a static docker-compose?

Copy link
Contributor

Choose a reason for hiding this comment

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

You can't assume the user won't ever need to add an env after they start using the compose file. If and when they do, there should be a place to put that env that ideally isn't the compose file itself; not needing to modify the file directly is part of the point of the generator.

The .env file makes this a complete non-issue. I guess I'm not seeing the practical benefit for this change - what are the problems with the .env file?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is much easier to understand a compose file when the environment variables are inline. With regards to the generator, I don't think it's a hard requirement that the user can't/won't change it after the fact. If it gives the user a good baseline that's already a win. Regardless, I'm sure we can implement the "auto-upgrade" or "diff-ing" capabilities and ignore lower-priority line changes, like extra ML environment variables.

DB_USERNAME: ${DB_USERNAME}
DB_PASSWORD: ${DB_PASSWORD}
DB_USERNAME: ${DB_DATABASE_NAME}
TZ: ${TZ}
ports:
- '2283:2283'
depends_on:
Expand All @@ -40,8 +43,6 @@ services:
# service: cpu # set to one of [armnn, cuda, openvino, openvino-wsl] for accelerated inference - use the `-wsl` version for WSL2 where applicable
volumes:
- model-cache:/cache
env_file:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would be confusing. There are many environmental variables in the machine learning service, including commonly used ones like for preloading certain models.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if the new Get Immich generator is also doing away with the env, Isn’t it best practice to assign the env vars to the corresponding container?

- .env
restart: always
healthcheck:
disable: false
Expand Down
4 changes: 2 additions & 2 deletions docker/example.env
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ UPLOAD_LOCATION=./library
# The location where your database files are stored
DB_DATA_LOCATION=./postgres

# To set a timezone, uncomment the next line and change Etc/UTC to a TZ identifier from this list: https://en.wikipedia.org/wiki/List_of_tz_database_time_zones#List
# TZ=Etc/UTC
# To set a timezone, change Etc/UTC to a TZ identifier from this list: https://en.wikipedia.org/wiki/List_of_tz_database_time_zones#List
TZ=Etc/UTC
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we change this to an empty string (TZ=‘’) and log a warning on server startup if empty?


# The Immich version to use. You can pin this to a specific version like "v1.71.0"
IMMICH_VERSION=release
Expand Down
Loading