Merge pull request #280 from compose-spec/fix-env-vars

Fix lookup precedence
compose-spec · Jul 28, 2022 · cc56ae5 · cc56ae5
2 parents a317b17 + 748d531
commit cc56ae5
Show file tree

Hide file tree

Showing 7 changed files with 73 additions and 15 deletions.
diff --git a/cli/options.go b/cli/options.go
@@ -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
@@ -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
-		}
 		envMap[k] = v
 	}
 

diff --git a/dotenv/fixtures/substitutions-default.env b/dotenv/fixtures/substitutions-default.env
@@ -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}
diff --git a/dotenv/fixtures/substitutions.env b/dotenv/fixtures/substitutions.env
@@ -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}
diff --git a/dotenv/godotenv_test.go b/dotenv/godotenv_test.go
@@ -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": "",
 	}
 
@@ -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",
+		"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": "",
 	}
 

diff --git a/dotenv/parser.go b/dotenv/parser.go
@@ -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
@@ -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

diff --git a/loader/full-struct_test.go b/loader/full-struct_test.go
@@ -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"),
@@ -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
@@ -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",

diff --git a/loader/loader.go b/loader/loader.go
@@ -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)