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

Fix lookup precedence #280

Merged
merged 1 commit into from Jul 28, 2022
Merged

Fix lookup precedence #280

merged 1 commit into from Jul 28, 2022

Conversation

ulyssessouza
Copy link
Contributor

No description provided.

@ulyssessouza ulyssessouza marked this pull request as ready for review Jul 5, 2022
@ulyssessouza ulyssessouza requested a review from ndeloof as a code owner Jul 5, 2022
@ulyssessouza ulyssessouza requested a review from glours Jul 5, 2022
@ulyssessouza ulyssessouza marked this pull request as draft Jul 5, 2022
@ulyssessouza ulyssessouza marked this pull request as ready for review Jul 8, 2022
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.

LGTM

Copy link
Member

@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.

This seems to be changing the precedence to make env file higher than OS/shell, which I didn't think we wanted

if _, set := currentEnv[k]; set {
continue
}
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be making env file HIGHER precedence than shell - isn't that the opposite of what we want?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. That's exactly the intention.
If the users want to choose the OS one they can use something like this in the .env file:

MYVAR=${MYVAR:-DefaultValue}

This would use the OS variable when present and use DefaultValue when unset or empty.

That's why I had to fix the default values first.

Copy link
Member

@milas milas Jul 26, 2022

Choose a reason for hiding this comment

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

Ahhh okay I just did some sleuthing, and v1 does behave in the same way that you are making the changes here: env has higher precedence than shell, so interpolation/default syntax is required for any values you want to "fall through" to shell.

HOWEVER, my confusion came from the fact that the Compose docs, even historically going back to v1, actually incorrectly state the opposite behavior 🙈 (e.g. here's the page in 2019 -> https://github.com/docker/docker.github.io/blob/f2093d4e2a5f950d1fd8e6dc61441a1d90cecdb4/compose/environment-variables.md?plain=1#L115-L117). NOTE: the docs are now technically correct, as current v2 behavior matches what they say rather than what v1 actually does 🙃

So, I think for v1 compat, this makes sense! But we'll definitely need to take care to find any remaining errant references to the order being the other way around, and given that v2 is now flip-flopping its behavior, should make sure we call that out explicitly in release notes, along with the "workaround" (use MYVAR=${MYVAR:-DefaultValue} syntax in .env file), and explanation that it should now behave the same between v1+v2 😅

dotenv/godotenv_test.go Show resolved Hide resolved
@ulyssessouza ulyssessouza force-pushed the fix-env-vars branch 2 times, most recently from 396ec14 to 81ed786 Compare Jul 14, 2022
Signed-off-by: Ulysses Souza <ulyssessouza@gmail.com>
glours
glours approved these changes Jul 22, 2022
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
Waiting for @milas approval before merging

milas
milas approved these changes Jul 26, 2022
@milas
Copy link
Member

milas commented Jul 26, 2022

@ulyssessouza @glours I found a case where (after these changes) v1 and v2 behave slightly differently: the in-file interpolation in v1 DID take shell as higher precedence than env.

Arguably, that's a bug/inconsistency in v1, but noting here so that we can mention it in docs/release notes somehow and to make sure there's not some reason that behavior was desirable???

$ cat .env
VAR=dotenv
$ cat docker-compose.yaml
services:
  echo:
    image: alpine
    command: [
      '/bin/ash',
      '-c',
      'echo "Compose config: ${VAR} | Container: $${VAR}"'
    ]
    env_file: .env
Version Command Result
v1 docker-compose up Compose config: dotenv | Container: dotenv
v1 VAR=shell docker-compose up Compose config: shell | Container: dotenv
v2 (with fixes) docker compose up Compose config: dotenv | Container: dotenv
v2 (with fixes) VAR=shell docker compose up Compose config: dotenv | Container: dotenv

@ulyssessouza
Copy link
Contributor Author

ulyssessouza commented Jul 28, 2022

@milas Hmmm! It's because in V1 the precedence of .env and the OS Env are switched in this case. So the OS is not overwritten by the .env.

If you change the name of the env file (also in docker-compose.yml) you should see the same results.

@milas
Copy link
Member

milas commented Jul 28, 2022

in V1 the precedence of .env and the OS Env are switched in this case

Yeah, I'd argue this is a bug / inconsistency / undesirable behavior from v1, so I don't think it makes sense to carry over.

I think the behavior of v2 (with your changes from this PR + others) is now good/correct! 💪

@ulyssessouza ulyssessouza merged commit cc56ae5 into master Jul 28, 2022
9 checks passed
@ulyssessouza ulyssessouza deleted the fix-env-vars branch Jul 28, 2022
@glours
Copy link
Contributor

glours commented Jul 28, 2022

🎉

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.

None yet

4 participants