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.

Sign up for GitHub

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

Jump to bottom

Fix lookup precedence #280

Merged
merged 1 commit into from Jul 28, 2022
+73 −15
Merged

Fix lookup precedence #280

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 0 additions & 4 deletions cli/options.go
View file
Open in desktop
Expand Up @@ -242,7 +242,6 @@ func GetEnvFromFile(currentEnv map[string]string, workingDir string, filename st
env, err := dotenv.ParseWithLookup(file, func(k string) (string, bool) {
v, ok := currentEnv[k]
if !ok {

return "", false
}
return v, true
Expand All @@ -251,9 +250,6 @@ func GetEnvFromFile(currentEnv map[string]string, workingDir string, filename st
return envMap, err
}
for k, v := range env {
if _, set := currentEnv[k]; set {
continue
}
Comment on lines -254 to -256
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 •
edited

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 😅

envMap[k] = v
}

Expand Down
5 changes: 5 additions & 0 deletions dotenv/fixtures/substitutions-default.env
View file
Open in desktop
@@ -0,0 +1,5 @@
OPTION_A=${OPTION_A:-1}
OPTION_B=${OPTION_A}
OPTION_C=$OPTION_B
OPTION_D=${OPTION_A}_${OPTION_B}
OPTION_E=${OPTION_NOT_DEFINED}
2 changes: 1 addition & 1 deletion dotenv/fixtures/substitutions.env
View file
Open in desktop
@@ -1,5 +1,5 @@
OPTION_A=1
OPTION_B=${OPTION_A}
OPTION_C=$OPTION_B
OPTION_D=${OPTION_A}${OPTION_B}
OPTION_D=${OPTION_A}_${OPTION_B}
OPTION_E=${OPTION_NOT_DEFINED}
64 changes: 61 additions & 3 deletions dotenv/godotenv_test.go
View file
Open in desktop
Expand Up @@ -218,7 +218,7 @@ func TestSubstitutions(t *testing.T) {
"OPTION_A": "1",
"OPTION_B": "1",
"OPTION_C": "1",
"OPTION_D": "11",
"OPTION_D": "1_1",
"OPTION_E": "",
}

Expand Down Expand Up @@ -614,19 +614,77 @@ func TestExpendingEnvironmentWithLookup(t *testing.T) {
}
}

func TestSubstitutionsWithShellEnvPrecedence(t *testing.T) {
func TestSubstitutionsWithEnvFilePrecedence(t *testing.T) {
os.Clearenv()
const envKey = "OPTION_A"
const envVal = "5"
os.Setenv(envKey, envVal)
defer os.Unsetenv(envKey)

envFileName := "fixtures/substitutions.env"
expectedValues := map[string]string{
"OPTION_A": "1",
milas marked this conversation as resolved.
Show resolved Hide resolved
"OPTION_B": "1",
"OPTION_C": "1",
"OPTION_D": "1_1",
"OPTION_E": "",
}

envMap, err := ReadWithLookup(os.LookupEnv, envFileName)
if err != nil {
t.Error("Error reading file")
}
if len(envMap) != len(expectedValues) {
t.Error("Didn't get the right size map back")
}

for key, value := range expectedValues {
if envMap[key] != value {
t.Errorf("Read got one of the keys wrong, [%q]->%q", key, envMap[key])
}
}
}

func TestSubstitutionsWithEnvFileDefaultValuePrecedence(t *testing.T) {
os.Clearenv()
const envKey = "OPTION_A"
const envVal = "5"
os.Setenv(envKey, envVal)
defer os.Unsetenv(envKey)

envFileName := "fixtures/substitutions-default.env"
expectedValues := map[string]string{
"OPTION_A": "5",
"OPTION_B": "5",
"OPTION_C": "5",
"OPTION_D": "55",
"OPTION_D": "5_5",
"OPTION_E": "",
}

envMap, err := ReadWithLookup(os.LookupEnv, envFileName)
if err != nil {
t.Error("Error reading file")
}
if len(envMap) != len(expectedValues) {
t.Error("Didn't get the right size map back")
}

for key, value := range expectedValues {
if envMap[key] != value {
t.Errorf("Read got one of the keys wrong, [%q]->%q", key, envMap[key])
}
}
}

func TestSubstitutionsWithUnsetVarEnvFileDefaultValuePrecedence(t *testing.T) {
os.Clearenv()

envFileName := "fixtures/substitutions-default.env"
expectedValues := map[string]string{
"OPTION_A": "1",
"OPTION_B": "1",
"OPTION_C": "1",
"OPTION_D": "1_1",
"OPTION_E": "",
}

Expand Down
4 changes: 0 additions & 4 deletions dotenv/parser.go
View file
Open in desktop
Expand Up @@ -37,7 +37,6 @@ func parseBytes(src []byte, out map[string]string, lookupFn LookupFn) error {
}

if inherited {

value, ok := lookupFn(key)
if ok {
out[key] = value
Expand All @@ -50,9 +49,6 @@ func parseBytes(src []byte, out map[string]string, lookupFn LookupFn) error {
if err != nil {
return err
}
if lookUpValue, ok := lookupFn(key); ok {
value = lookUpValue
}

out[key] = value
cutset = left
Expand Down
6 changes: 3 additions & 3 deletions loader/full-struct_test.go
View file
Open in desktop
Expand Up @@ -165,7 +165,7 @@ func services(workingDir, homeDir string) []types.ServiceConfig {
Entrypoint: []string{"/code/entrypoint.sh", "-p", "3000"},
Environment: map[string]*string{
"FOO": strPtr("foo_from_env_file"),
"BAR": strPtr("this is a secret"),
"BAR": strPtr("bar_from_env_file_2"),
"BAZ": strPtr("baz_from_service_def"),
"QUX": strPtr("qux_from_environment"),
"ENV.WITH.DOT": strPtr("ok"),
Expand Down Expand Up @@ -692,7 +692,7 @@ services:
- -p
- "3000"
environment:
BAR: this is a secret
BAR: bar_from_env_file_2
BAZ: baz_from_service_def
ENV.WITH.DOT: ok
ENV_WITH_UNDERSCORE: ok
Expand Down Expand Up @@ -1265,7 +1265,7 @@ func fullExampleJSON(workingDir, homeDir string) string {
"3000"
],
"environment": {
"BAR": "this is a secret",
"BAR": "bar_from_env_file_2",
"BAZ": "baz_from_service_def",
"ENV.WITH.DOT": "ok",
"ENV_WITH_UNDERSCORE": "ok",
Expand Down
3 changes: 3 additions & 0 deletions loader/loader.go
View file
Open in desktop
Expand Up @@ -624,6 +624,9 @@ func resolveEnvironment(serviceConfig *types.ServiceConfig, workingDir string, l
environment := types.MappingWithEquals{}

if len(serviceConfig.EnvFile) > 0 {
if serviceConfig.Environment == nil {
serviceConfig.Environment = types.MappingWithEquals{}
}
for _, envFile := range serviceConfig.EnvFile {
filePath := absPath(workingDir, envFile)
file, err := os.Open(filePath)
Expand Down