Repository: compose-spec/compose-go
Pull request #257 (2022-05-03 03:53:07)

Resolve environments case-insensitively on Windows
Please have a look on https://github.com/docker/compose/issues/9431 for details.

compose-go extracts environment variables to `Environment map[string]string` and reference that later with simply `Environment[varname]`.
It works case-sensitively but environment variables should be treated case-insensitively on Windows.

This change handles environment variables case-insensitively on Windows, without changing data structures on compose-go.

This change also adds exposed `utils.EnvResolver` function. I plan to use that for additional changes in https://github.com/docker/compose/ , handling option args like `--build-arg` with `build` sub command and `-e` with `run` sub command.
(like this: https://github.com/ikedam/compose/pull/1/files)


ndeloof: Thanks for looking into this!
For legal reasons, all commits need to be signed-off. Please amend your commit and force push your branch:
`git commit --amend --signoff && git push --force ...`

ikedam: > Thanks for looking into this! For legal reasons, all commits need to be signed-off. Please amend your commit and force push your branch: `git commit --amend --signoff && git push --force ...`

I've added signed-off line as described in https://github.com/compose-spec/compose-go/blob/master/CONTRIBUTING.md :
```
PS C:\workspace\compose-go> git log -n 1
commit 90f9d92dab00e1272c050960baeaaa8d8b15aaad (HEAD -> feature/9431_CaseInsensitiveEnv, ikedam/feature/9431_CaseInsensitiveEnv)
Author: ikedam <devld@ikedam.jp>
Date:   Tue May 3 11:58:23 2022 +0900

    Resolve environments case-insensitively on Windows

    Signed-off-by: IKEDA Yasuyuki <devld@ikedam.jp>
PS C:\workspace\compose-go>
```
But something might be wrong as this is the first time I wrote signed-off line.

Should I use the real name also in author line? Or should I add GPG signature? (git commit -S)

ikedam: Oh, I got it. I have to pass DCO action. I'll try that.

ikedam: Thanks for the review. I fixed problems you pointed.


Patch:
diff --git a/loader/loader.go b/loader/loader.go
index 04198e1..6b349c8 100644
--- a/loader/loader.go
+++ b/loader/loader.go
@@ -24,6 +24,7 @@ import (
 	"path/filepath"
 	"reflect"
 	"regexp"
+	"sort"
 	"strconv"
 	"strings"
 	"time"
@@ -384,7 +385,7 @@ func createTransformHook(additionalTransformers ...Transformer) mapstructure.Dec
 		reflect.TypeOf(types.MappingWithEquals{}):                transformMappingOrListFunc("=", true),
 		reflect.TypeOf(types.Labels{}):                           transformMappingOrListFunc("=", false),
 		reflect.TypeOf(types.MappingWithColon{}):                 transformMappingOrListFunc(":", false),
-		reflect.TypeOf(types.HostsList{}):                        transformMappingOrListFunc(":", false),
+		reflect.TypeOf(types.HostsList{}):                        transformListOrMappingFunc(":", false),
 		reflect.TypeOf(types.ServiceVolumeConfig{}):              transformServiceVolumeConfig,
 		reflect.TypeOf(types.BuildConfig{}):                      transformBuildConfig,
 		reflect.TypeOf(types.Duration(0)):                        transformStringToDuration,
@@ -1059,6 +1060,22 @@ func transformMappingOrListFunc(sep string, allowNil bool) TransformerFunc {
 	}
 }
 
+func transformListOrMappingFunc(sep string, allowNil bool) TransformerFunc {
+	return func(data interface{}) (interface{}, error) {
+		return transformListOrMapping(data, sep, allowNil)
+	}
+}
+
+func transformListOrMapping(listOrMapping interface{}, sep string, allowNil bool) (interface{}, error) {
+	switch value := listOrMapping.(type) {
+	case map[string]interface{}:
+		return toStringList(value, sep, allowNil), nil
+	case []interface{}:
+		return listOrMapping, nil
+	}
+	return nil, errors.Errorf("expected a map or a list, got %T: %#v", listOrMapping, listOrMapping)
+}
+
 func transformMappingOrList(mappingOrList interface{}, sep string, allowNil bool) (interface{}, error) {
 	switch value := mappingOrList.(type) {
 	case map[string]interface{}:
@@ -1151,3 +1168,15 @@ func toString(value interface{}, allowNil bool) interface{} {
 		return ""
 	}
 }
+
+func toStringList(value map[string]interface{}, separator string, allowNil bool) []string {
+	var output []string
+	for key, value := range value {
+		if value == nil && !allowNil {
+			continue
+		}
+		output = append(output, fmt.Sprintf("%s%s%s", key, separator, value))
+	}
+	sort.Strings(output)
+	return output
+}
diff --git a/types/config.go b/types/config.go
index b395363..020e67d 100644
--- a/types/config.go
+++ b/types/config.go
@@ -18,10 +18,17 @@ package types
 
 import (
 	"encoding/json"
+	"runtime"
+	"strings"
 
 	"github.com/mitchellh/mapstructure"
 )
 
+var (
+	// isCaseInsensitiveEnvVars is true on platforms where environment variable names are treated case-insensitively.
+	isCaseInsensitiveEnvVars = (runtime.GOOS == "windows")
+)
+
 // ConfigDetails are the details about a group of ConfigFiles
 type ConfigDetails struct {
 	Version     string
@@ -33,7 +40,21 @@ type ConfigDetails struct {
 // LookupEnv provides a lookup function for environment variables
 func (cd ConfigDetails) LookupEnv(key string) (string, bool) {
 	v, ok := cd.Environment[key]
-	return v, ok
+	if !isCaseInsensitiveEnvVars || ok {
+		return v, ok
+	}
+	// variable names must be treated case-insensitively on some platforms (that is, Windows).
+	// Resolves in this way:
+	// * Return the value if its name matches with the passed name case-sensitively.
+	// * Otherwise, return the value if its lower-cased name matches lower-cased passed name.
+	//     * The value is indefinite if multiple variables match.
+	lowerKey := strings.ToLower(key)
+	for k, v := range cd.Environment {
+		if strings.ToLower(k) == lowerKey {
+			return v, true
+		}
+	}
+	return "", false
 }
 
 // ConfigFile is a filename and the contents of the file as a Dict
diff --git a/types/types.go b/types/types.go
index 597a3db..58369d4 100644
--- a/types/types.go
+++ b/types/types.go
@@ -468,16 +468,7 @@ func (s SSHKey) MarshalJSON() ([]byte, error) {
 type MappingWithColon map[string]string
 
 // HostsList is a list of colon-separated host-ip mappings
-type HostsList map[string]string
-
-// AsList return host-ip mappings as a list of colon-separated strings
-func (h HostsList) AsList() []string {
-	l := make([]string, 0, len(h))
-	for k, v := range h {
-		l = append(l, fmt.Sprintf("%s:%s", k, v))
-	}
-	return l
-}
+type HostsList []string
 
 // LoggingConfig the logging configuration for a service
 type LoggingConfig struct {


Test patch:
diff --git a/loader/full-struct_test.go b/loader/full-struct_test.go
index 0161518..36f9d6f 100644
--- a/loader/full-struct_test.go
+++ b/loader/full-struct_test.go
@@ -178,9 +178,9 @@ func services(workingDir, homeDir string) []types.ServiceConfig {
 				"project_db_1:mysql",
 				"project_db_1:postgresql",
 			},
-			ExtraHosts: types.HostsList{
-				"somehost":  "162.242.195.82",
-				"otherhost": "50.31.209.229",
+			ExtraHosts: []string{
+				"somehost:162.242.195.82",
+				"otherhost:50.31.209.229",
 			},
 			Extensions: map[string]interface{}{
 				"x-bar": "baz",
@@ -700,8 +700,8 @@ services:
     - project_db_1:mysql
     - project_db_1:postgresql
     extra_hosts:
-      otherhost: 50.31.209.229
-      somehost: 162.242.195.82
+    - somehost:162.242.195.82
+    - otherhost:50.31.209.229
     hostname: foo
     healthcheck:
       test:
@@ -1270,10 +1270,10 @@ func fullExampleJSON(workingDir, homeDir string) string {
         "project_db_1:mysql",
         "project_db_1:postgresql"
       ],
-      "extra_hosts": {
-        "otherhost": "50.31.209.229",
-        "somehost": "162.242.195.82"
-      },
+      "extra_hosts": [
+        "somehost:162.242.195.82",
+        "otherhost:50.31.209.229"
+      ],
       "hostname": "foo",
       "healthcheck": {
         "test": [
diff --git a/loader/loader_test.go b/loader/loader_test.go
index c3baec5..961baef 100644
--- a/loader/loader_test.go
+++ b/loader/loader_test.go
@@ -1232,8 +1232,8 @@ services:
 	assert.NilError(t, err)
 
 	expected := types.HostsList{
-		"alpha": "50.31.209.229",
-		"zulu":  "162.242.195.82",
+		"alpha:50.31.209.229",
+		"zulu:162.242.195.82",
 	}
 
 	assert.Assert(t, is.Len(config.Services, 1))
@@ -1246,14 +1246,16 @@ services:
   web:
     image: busybox
     extra_hosts:
+      - "zulu:162.242.195.82"
       - "alpha:50.31.209.229"
       - "zulu:ff02::1"
 `)
 	assert.NilError(t, err)
 
 	expected := types.HostsList{
-		"alpha": "50.31.209.229",
-		"zulu":  "ff02::1",
+		"zulu:162.242.195.82",
+		"alpha:50.31.209.229",
+		"zulu:ff02::1",
 	}
 
 	assert.Assert(t, is.Len(config.Services, 1))
diff --git a/loader/merge_test.go b/loader/merge_test.go
index 329b084..79a1241 100644
--- a/loader/merge_test.go
+++ b/loader/merge_test.go
@@ -1256,28 +1256,3 @@ func TestMergeEnvironments(t *testing.T) {
 	assert.Assert(t, *env["NAME"] == "DEV")
 	assert.Assert(t, env["VALUE"] == nil)
 }
-
-func TestMergeExtraHosts(t *testing.T) {
-	base := types.HostsList{
-		"kept":              "192.168.1.100",
-		"extra1.domain.org": "192.168.1.101",
-		"extra2.domain.org": "192.168.1.102",
-	}
-	override := types.HostsList{
-		"extra1.domain.org": "10.0.0.1",
-		"extra2.domain.org": "10.0.0.2",
-		"added":             "10.0.0.3",
-	}
-	err := mergo.Merge(&base, &override, mergo.WithOverride)
-	assert.NilError(t, err)
-	assert.DeepEqual(
-		t,
-		base,
-		types.HostsList{
-			"kept":              "192.168.1.100",
-			"extra1.domain.org": "10.0.0.1",
-			"extra2.domain.org": "10.0.0.2",
-			"added":             "10.0.0.3",
-		},
-	)
-}
diff --git a/types/config_test.go b/types/config_test.go
index 32ebc74..7743bf4 100644
--- a/types/config_test.go
+++ b/types/config_test.go
@@ -53,3 +53,102 @@ func Test_WithServices(t *testing.T) {
 	assert.NilError(t, err)
 	assert.DeepEqual(t, order, []string{"service_2", "service_3", "service_1"})
 }
+
+func Test_LookupEnv(t *testing.T) {
+	tests := []struct {
+		name            string
+		environment     map[string]string
+		caseInsensitive bool
+		search          string
+		expectedValue   string
+		expectedOk      bool
+	}{
+		{
+			name: "case sensitive/case match",
+			environment: map[string]string{
+				"Env1": "Value1",
+				"Env2": "Value2",
+			},
+			caseInsensitive: false,
+			search:          "Env1",
+			expectedValue:   "Value1",
+			expectedOk:      true,
+		},
+		{
+			name: "case sensitive/case unmatch",
+			environment: map[string]string{
+				"Env1": "Value1",
+				"Env2": "Value2",
+			},
+			caseInsensitive: false,
+			search:          "ENV1",
+			expectedValue:   "",
+			expectedOk:      false,
+		},
+		{
+			name:            "case sensitive/nil environment",
+			environment:     nil,
+			caseInsensitive: false,
+			search:          "Env1",
+			expectedValue:   "",
+			expectedOk:      false,
+		},
+		{
+			name: "case insensitive/case match",
+			environment: map[string]string{
+				"Env1": "Value1",
+				"Env2": "Value2",
+			},
+			caseInsensitive: true,
+			search:          "Env1",
+			expectedValue:   "Value1",
+			expectedOk:      true,
+		},
+		{
+			name: "case insensitive/case unmatch",
+			environment: map[string]string{
+				"Env1": "Value1",
+				"Env2": "Value2",
+			},
+			caseInsensitive: true,
+			search:          "ENV1",
+			expectedValue:   "Value1",
+			expectedOk:      true,
+		},
+		{
+			name: "case insensitive/unmatch",
+			environment: map[string]string{
+				"Env1": "Value1",
+				"Env2": "Value2",
+			},
+			caseInsensitive: true,
+			search:          "Env3",
+			expectedValue:   "",
+			expectedOk:      false,
+		},
+		{
+			name:            "case insensitive/nil environment",
+			environment:     nil,
+			caseInsensitive: true,
+			search:          "Env1",
+			expectedValue:   "",
+			expectedOk:      false,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			origIsCaseInsensitiveEnvVars := isCaseInsensitiveEnvVars
+			defer func() {
+				isCaseInsensitiveEnvVars = origIsCaseInsensitiveEnvVars
+			}()
+			isCaseInsensitiveEnvVars = test.caseInsensitive
+			cd := ConfigDetails{
+				Environment: test.environment,
+			}
+			v, ok := cd.LookupEnv(test.search)
+			assert.Equal(t, v, test.expectedValue)
+			assert.Equal(t, ok, test.expectedOk)
+		})
+	}
+}