Fix env file read from include #582

jhrotko · 2024-02-19T13:52:55Z

When reading a custom env file from include, we were not passing the read enviroenmnt variables to the config Environments, thus the read vars were being "forgotten".
Also fixed the default directory for environment variables, which should be the same as the compose.yml that is invoking the include

withListeners change forgotten in previous PR

Fixes:
docker/compose#11509
https://docker.atlassian.net/browse/COMP-438

ndeloof · 2024-02-19T14:23:21Z

loader/include.go

+		} else {
+			envFile := []string{}
+			for _, f := range r.EnvFile {
+				f := filepath.Join(configDetails.WorkingDir, f)


need to guard this with filepath.IsAbs(r.Envfile) as env_file can also be set to an absolute path, or relative to user's home ~

ndeloof · 2024-02-19T14:24:24Z

loader/include.go

+			envFile := []string{}
+			for _, f := range r.EnvFile {
+				f := filepath.Join(configDetails.WorkingDir, f)
+				if s, err := os.Stat(f); err == nil && !s.IsDir() {


if explicitly selected env file is a Dir, then this is an error. We only ignore when selecting the default .env file.

ndeloof · 2024-02-19T14:28:48Z

loader/include.go

 		}

 		envFromFile, err := dotenv.GetEnvFromFile(configDetails.Environment, r.EnvFile)
 		if err != nil {
 			return err
 		}
+		configDetails.Environment.Merge(envFromFile)


Why this change ?
Need to clone before merge, otherwise env_file selected to parse the included model will still be present in configDetails and will have impacts on next include being processed

It took me some time to understand what is the error. Currently, the the only environment variables that are resolved DURING the parse is the Interpolation ($VAR) and in this case environment: - VAR will only be solved AFTER the load in - at the project level -

compose-go/loader/loader.go

Line 528 in 4c12ed6

project, err = project.WithServicesEnvironmentResolved(opts.discardEnvFiles)

That's why changing the Merge to also be applied to the higher level configDetails works. However, this is not correct, the environment variables of custom.env (from the test and the issue example) should be only within the context of the include. I propose to apply a resolveEnvironment at the end of the loadYamlModel to have this case into consideration, without changing the overall project.Environment as the first fix

ndeloof · 2024-02-21T10:41:28Z

loader/loader.go

@@ -787,3 +788,29 @@ func convertVolumePath(volume types.ServiceVolumeConfig) types.ServiceVolumeConf
 	volume.Source = convertedSource
 	return volume
 }
+
+func resolveEnvironment(dict map[string]any, opts *Options) {


I was confused reviewing this PR about this func name, which is close to Project.ResolveEnvironment but serve a very different scenario, but I don't have a better name for now 😅
Will create a follow-up PR if I find one

Ok I'm not alone 😅

ndeloof · 2024-02-21T10:45:24Z

loader/loader.go

+					if !ok {
+						continue
+					}
+					if found, ok := opts.Interpolate.LookupValue(env); ok {


Interpolate being involved here is a bit confusing. I understand the actual goal is to access environment map, which LookupValue captured in options. So this basically is a hack - We should better add an explicit Environment (or Variables ?) attribute to loader.Options so we make it explicit we rely on the ConfigDetails.Environment here (and maybe in other places in future)

glours

I think we should directly get the value from the map and avoid looping the whole dict

Edit: I didn't see the recursive call of the method, so I was wrong

glours · 2024-02-21T10:43:19Z

loader/loader.go

@@ -787,3 +788,29 @@ func convertVolumePath(volume types.ServiceVolumeConfig) types.ServiceVolumeConf
 	volume.Source = convertedSource
 	return volume
 }
+
+func resolveEnvironment(dict map[string]any, opts *Options) {


Ok I'm not alone 😅

ndeloof · 2024-02-21T10:51:44Z

loader/loader.go

+	for key, value := range dict {
+		if key == "environment" {
+			if environment, ok := value.([]any); ok {
+				serviceEnvironment := []any{}
+				for _, env := range environment {
+					env, ok := env.(string)
+					if !ok {
+						continue
+					}
+					if found, ok := opts.Interpolate.LookupValue(env); ok {
+						serviceEnvironment = append(serviceEnvironment, fmt.Sprintf("%s=%s", env, found))
+					} else {
+						serviceEnvironment = append(serviceEnvironment, env)
+					}
+
+				}
+				dict["environment"] = serviceEnvironment
+			}
+		}
+		if v, ok := value.(map[string]any); ok {
+			resolveEnvironment(v, opts)
+		}
+	}


can also do

switch environment := value.(type) { case []any: for _, env := range environment { ... } case maps[string]any: resolveEnvironment(v, opts)

ndeloof · 2024-02-21T10:51:44Z

loader/loader.go

+	for key, value := range dict {
+		if key == "environment" {
+			if environment, ok := value.([]any); ok {
+				serviceEnvironment := []any{}
+				for _, env := range environment {
+					env, ok := env.(string)
+					if !ok {
+						continue
+					}
+					if found, ok := opts.Interpolate.LookupValue(env); ok {
+						serviceEnvironment = append(serviceEnvironment, fmt.Sprintf("%s=%s", env, found))
+					} else {
+						serviceEnvironment = append(serviceEnvironment, env)
+					}
+
+				}
+				dict["environment"] = serviceEnvironment
+			}
+		}
+		if v, ok := value.(map[string]any); ok {
+			resolveEnvironment(v, opts)
+		}
+	}


can also do

switch environment := value.(type) { case []any: for _, env := range environment { ... } case maps[string]any: resolveEnvironment(v, opts)

Signed-off-by: jhrotko <joana.hrotko@docker.com>

glours

LGTM

jhrotko requested review from ndeloof, glours and milas February 19, 2024 13:53

jhrotko force-pushed the env-var-from-custom-env-file branch from e9735eb to 9b5ad02 Compare February 19, 2024 13:56

jhrotko marked this pull request as ready for review February 19, 2024 13:56

jhrotko self-assigned this Feb 19, 2024

jhrotko added the bug Something isn't working label Feb 19, 2024

ndeloof reviewed Feb 19, 2024

View reviewed changes

jhrotko requested review from ndeloof and laurazard February 19, 2024 15:41

jhrotko force-pushed the env-var-from-custom-env-file branch 6 times, most recently from 17844c5 to 3a3d39e Compare February 20, 2024 16:56

ndeloof approved these changes Feb 21, 2024

View reviewed changes

ndeloof reviewed Feb 21, 2024

View reviewed changes

glours reviewed Feb 21, 2024

View reviewed changes

ndeloof reviewed Feb 21, 2024

View reviewed changes

jhrotko force-pushed the env-var-from-custom-env-file branch 2 times, most recently from c2d05a7 to 15225af Compare February 21, 2024 10:59

ndeloof approved these changes Feb 21, 2024

View reviewed changes

glours approved these changes Feb 21, 2024

View reviewed changes

jhrotko force-pushed the env-var-from-custom-env-file branch 2 times, most recently from d3a0d73 to 11c5003 Compare February 21, 2024 15:21

Fix env file read from include

9ae13bd

Signed-off-by: jhrotko <joana.hrotko@docker.com>

jhrotko force-pushed the env-var-from-custom-env-file branch from 11c5003 to 9ae13bd Compare February 21, 2024 15:24

glours approved these changes Feb 21, 2024

View reviewed changes

jhrotko merged commit 6e8b7bf into compose-spec:main Feb 21, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix env file read from include #582

Fix env file read from include #582

jhrotko commented Feb 19, 2024 •

edited

Loading

ndeloof Feb 19, 2024

ndeloof Feb 19, 2024

ndeloof Feb 19, 2024

jhrotko Feb 20, 2024 •

edited

Loading

ndeloof Feb 21, 2024

glours Feb 21, 2024

ndeloof Feb 21, 2024

glours left a comment •

edited

Loading

glours Feb 21, 2024

ndeloof Feb 21, 2024

ndeloof Feb 21, 2024

glours left a comment

Fix env file read from include #582

Fix env file read from include #582

Conversation

jhrotko commented Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhrotko Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glours left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glours left a comment

Choose a reason for hiding this comment

jhrotko commented Feb 19, 2024 •

edited

Loading

jhrotko Feb 20, 2024 •

edited

Loading

glours left a comment •

edited

Loading