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

Environment variables priority #9636

Merged
merged 2 commits into from
Jul 29, 2022
Merged

Conversation

ulyssessouza
Copy link
Collaborator

@ulyssessouza ulyssessouza commented Jul 8, 2022

What I did
This PR is meant to define the precedence of the environment variables evaluation.

For the environment variables to be available in the container:

  1. Command Line (docker compose run --env <KEY[=VAL]> https://docs.docker.com/engine/reference/commandline/compose_run/#options)
  2. Compose File (service::environment section: https://docs.docker.com/compose/compose-file/#environment)
  3. Compose File (service::env_file section file: https://docs.docker.com/compose/compose-file/#env_file)
  4. Container Image ENV directive (https://docs.docker.com/engine/reference/builder/#env)

For the environment variables available for docker compose's runtime:

  1. Command Line parent command option docker compose --env-file <FILE> (or .env in the root of the project by default)
  2. OS Environment Variables

Obs: Note that the variables available for runtime will not be available in the container unless it's imported by one of the methods on the list above

Related issue
Resolves #9521
Resolves #9638
Resolves #9608
Resolves #9578
Resolves #9468
Resolves #9683

Depends on compose-spec/compose-go#280

Documentation update PR

(not mandatory) A picture of a cute animal, if possible in relation with what you did
beautiful-goat

@ulyssessouza ulyssessouza requested a review from glours July 8, 2022 12:13
@ulyssessouza ulyssessouza force-pushed the dotenvfile-priority branch 3 times, most recently from 8fc5249 to e931d20 Compare July 12, 2022 17:24
@ulyssessouza ulyssessouza marked this pull request as ready for review July 12, 2022 18:44
@@ -151,3 +151,5 @@ replace (
k8s.io/apimachinery => k8s.io/apimachinery v0.22.4
k8s.io/client-go => k8s.io/client-go v0.22.4
)

replace github.com/compose-spec/compose-go => github.com/compose-spec/compose-go v1.2.9-0.20220712021117-dcfe0889b601 // TODO Remove and bump require section when PR is merged
Copy link
Collaborator Author

@ulyssessouza ulyssessouza Jul 12, 2022

Choose a reason for hiding this comment

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

Note that this points to compose-spec/compose-go#280
This PR shouldn't be merged before this dependency is merged and updated here.

Copy link
Contributor

@milas milas left a comment

Choose a reason for hiding this comment

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

Wahoo! Looks good for approval once the compose-go side of the house is merged + tagged 🥳

(Sorry for nits - as you're well aware, the env code is pretty complex, so I was trying to identify places I got confused and had to dig in more)

pkg/compose/run.go Outdated Show resolved Hide resolved
pkg/compose/run.go Outdated Show resolved Hide resolved
pkg/e2e/compose_environment_test.go Outdated Show resolved Hide resolved
cmd/compose/compose.go Outdated Show resolved Hide resolved
@ulyssessouza ulyssessouza force-pushed the dotenvfile-priority branch 3 times, most recently from 8d19ecf to 4870c50 Compare July 14, 2022 23:53
Copy link
Member

@nicksieger nicksieger left a comment

Choose a reason for hiding this comment

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

Overall this looks good, just have some questions about the tests and what the new priority order is.

t.Run("compose file priority", func(t *testing.T) {
cmd := c.NewDockerComposeCmd(t, "-f", "./fixtures/environment/env-priority/compose-with-env.yaml",
"--env-file", "./fixtures/environment/env-priority/.env.override",
"run", "--rm", "-e", "WHEREAMI", "env-compose-priority")
Copy link
Member

Choose a reason for hiding this comment

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

So there's a difference between -e WHEREAMI with the var defined, and -e WHEREAMI=shell? In this case shouldn't 3. be the expected result?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the first, the variable value will be inherited from the environment.
The second one will have the explicit value passed (shell in this case).

"--rm", "-e", "WHEREAMI", "env-compose-priority")
cmd.Env = append(cmd.Env, "WHEREAMI=shell")
res := icmd.RunCmd(cmd)
assert.Equal(t, strings.TrimSpace(res.Stdout()), "Compose File")
assert.Equal(t, strings.TrimSpace(res.Stdout()), "override")
Copy link
Member

Choose a reason for hiding this comment

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

Update to 3. Environment file <-- Result expected above?

Copy link
Member

Choose a reason for hiding this comment

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

Also, these tests so far seem to indicate that 1. Compose file is not the highest priority. Am I reading this right?

Copy link
Collaborator Author

@ulyssessouza ulyssessouza Jul 28, 2022

Choose a reason for hiding this comment

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

I just refactored the comments to fix this and reflect the order of this change.
BTW, the new doc on priority is in the oven docker/docs#15161

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

@K0Te
Copy link

K0Te commented Aug 10, 2022

Hi,

Sorry if this is not the correct place to ask a question, I've checked this PR source code and documentation update and am still not sure what the expected behavior is.
I've used .env file to override COMPOSE_FILE for local development: COMPOSE_FILE=docker/docker-compose.yml:docker/docker-compose.override.yml. This allowed to have all docker stuff in a separate folder and provide some secrets in docker-compose.override.yml.
And on CI I've just used COMPOSE_FILE=docker/docker-compose.yml as an env variable in GH Actions.

This worked fine as of docker compose 2.7.0, but with docker compose 2.9.0 this workflow doesn't work anymore. Value from .env started to take precedence over the environment variable.

Is this a new expected behaviour or a bug?

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