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.

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

Import dotenv file to os environment #9512

Merged
merged 1 commit into from
May 30, 2022

Conversation

ulyssessouza
Copy link
Collaborator

What I did
Import dotenv file to os environment to enable setting variables like DOCKER_BUILDKIT and others through .env file

Related issue
Resolves #9345

@ulyssessouza ulyssessouza marked this pull request as ready for review May 30, 2022 15:49
Copy link
Contributor

@glours glours left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@laurazard laurazard left a comment

Choose a reason for hiding this comment

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

Looks good

@briceburg
Copy link

briceburg commented Jun 9, 2022

It doesn't appear that any tests were given with this PR, which would be useful for determining the desired behavior.

As a user I was expecting that the default .env file would be considered, and thus would address #9210 as well. From what I gather, this PR only considers env files explicitly passed via the --env-file option, and not the default .env file (when it exists in the CWD/project directory).

EDIT

It does appear that the root .env file is being considered, as I've been able to successfully set COMPOSE_FILE in there and have it take effect. Not sure what the issue is re: DOCKER_HOST ; would still be nice to see tests.

@wbydc
Copy link

wbydc commented Jun 27, 2022

Hi, looks like this breaks env_file option a bit.

Previously I had .env with private values for global setup, and generated service-name.env for several services in compose, witch were overwriting some values like this:

# .env
KEY=DEFAULT_VALUE

# first-service.env
KEY=VALUE_FOR_FIRST_SERVICE

# second-service.env
KEY=VALUE_FOR_SECOND_SERVICE
services:
    first-service:
        ...
        env_file:
            - .env
            - first-service.env
        ...

    second-service:
        ...
        env_file:
            - .env
            - second-service.env
        ...

But now it always uses values specified in .env, no matter the order in env_file.

I guess such behaviour should be mentioned in the documentation.

UPD: Actually priority of .env higher then env_file option

@digaoddc
Copy link

I agree with @wbydc, the env files passed with the env_file options are now ignored. I'm having problems with this new version of docker-compose in our project.

We use it like this:

app:
  env_file:
     - .docker.env
     - .docker.env.local

Before this PR, first, the docker.env was loaded, then the file docker.env.local merge any settings defined in the first env file.

Now, these env files declared in the docker-compose.yml are completely ignored. I don't think this is the expected behavior.

@curry684
Copy link

curry684 commented Jul 6, 2022

They are not ignored, just processed in the wrong order as demonstrated in #9608 (comment)

Either way, this 'fix' is currently breaking production environments and should be rolled back or fixed asap.

@gisostallenberg
Copy link

@glours @laurazard
This clearly introduced a bug in env file handling to support something new, which was not used by anyone before. It also breaks production and workarounds are hard to create. Why isn't this rolled back asap and the 'new feature' added later?

@crzdg
Copy link

crzdg commented Jul 12, 2022

Do we really want to have the .env-file loaded automatically in any case. Regardless of having it loaded before or after the defined files in the docker-compose. If so, there should at least by a function to disable this feature. And with this, we're back to the current feature set where you have to define the file in the docker-compose and the corresponding env_file-key.

I really can't understand why we want to have this feature in any place? As far as I understand it should be no problem to achieve the requested behavior with features available <2.6.0. This breaks and changes a lot, not only conceptional but also concerning security, CI/CD, and dev-ops in general.

I would love to see to roll-back this and hardly think again about this "feature"

@curry684
Copy link

I agree that the special status of .env should be reconsidered, or at least configurable at the compose file root level, but it's not something easily changed (it would break as much as the current change).

Doing a quick round it seems at least Next.js, React, Symfony, Vite and Laravel switched to the .env.local convention, so I definitely think Compose v3 should reconsider their magic on .env anyway.

@ulyssessouza
Copy link
Collaborator Author

This precedence issues should be fixed with #9636.

About the .env file load. That can be avoided by setting --env-file /dev/null for example. That would make Docker Compose avoid the loading of .env

@gisostallenberg
Copy link

@ulyssessouza Before 2.6 it did load .env values, making it possible to use environment variables in the docker compose file, but it did also load env_file-files before starting the service. Providing --env-file /dev/null does not work for docker compose files using environment variables as substitutes.

@RuvendeGroote
Copy link

why did a new feature get merged with 0 tests added? and why did it break things without tests failing anywhere?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: DOCKER_BUILDKIT=1 in .env
10 participants