feat(deployment): no env_file #14889
Conversation
|
Per the discussion on discord do we want to add |
mmomjian
added
changelog:feature
changelog:enhancement
and removed
changelog:feature
labels
| # 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 |
There was a problem hiding this comment.
Should we change this to an empty string (TZ=‘’) and log a warning on server startup if empty?
| @@ -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: | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
| @@ -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: | |||
There was a problem hiding this comment.
Likewise, I think it's confusing to have a .env file that only applies these variables.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
With container consolidations, at this point the
env_filedirective is causing increased support burden for no benefit. I propose that we remove it and expressly declare the necessary variables.The only thing that we lose by removing env_file from ML is the
TZ, which I don't believe is needed. open to any discussion. This should be a non-breaking change, even for our copy pastersNot too sure how exactly to apply it to the dev and prod YAMLs but should be very easy, I can work on it if we move forward. One big benefit is eliminating a setup step for portainer users.