Conversation
8fc5249 to
e931d20
Compare
| 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 |
There was a problem hiding this comment.
Note that this points to compose-spec/compose-go#280
This PR shouldn't be merged before this dependency is merged and updated here.
e931d20 to
a5fbd64
Compare
milas
left a comment
There was a problem hiding this comment.
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)
8d19ecf to
4870c50
Compare
nicksieger
left a comment
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
pkg/e2e/compose_environment_test.go
Outdated
| 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") |
There was a problem hiding this comment.
Update to 3. Environment file <-- Result expected above?
There was a problem hiding this comment.
Also, these tests so far seem to indicate that 1. Compose file is not the highest priority. Am I reading this right?
There was a problem hiding this comment.
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
Signed-off-by: Ulysses Souza <ulyssessouza@gmail.com>
4870c50 to
69ad343
Compare
Signed-off-by: Ulysses Souza <ulyssessouza@gmail.com>
69ad343 to
e9c8cfc
Compare
|
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. 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 Is this a new expected behaviour or a bug? |
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:
docker compose run --env <KEY[=VAL]>https://docs.docker.com/engine/reference/commandline/compose_run/#options)service::environmentsection: https://docs.docker.com/compose/compose-file/#environment)service::env_filesection file: https://docs.docker.com/compose/compose-file/#env_file)ENVdirective (https://docs.docker.com/engine/reference/builder/#env)For the environment variables available for
docker compose's runtime:docker compose --env-file <FILE>(or.envin the root of the project by default)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
