From cfca092640fc9f51b993d46c5f09ec156174bc83 Mon Sep 17 00:00:00 2001
From: Andrew Mains <amains@uber.com>
Date: Thu, 31 Oct 2019 18:41:44 -0400
Subject: [PATCH] Make expand work by default

---
 src/x/config/config.go      | 22 ++++++++--
 src/x/config/config_test.go | 81 +++++++++++++++++++++++++------------
 2 files changed, 74 insertions(+), 29 deletions(-)

diff --git a/src/x/config/config.go b/src/x/config/config.go
index 1c5b671c4a..8da57a7a51 100644
--- a/src/x/config/config.go
+++ b/src/x/config/config.go
@@ -23,6 +23,7 @@ package config
 
 import (
 	"errors"
+	"os"
 	"reflect"
 	"strings"
 
@@ -35,13 +36,23 @@ const (
 	deprecatedPrefix = "Deprecated"
 )
 
-var errNoFilesToLoad = errors.New("attempt to load config with no files")
+var (
+	errNoFilesToLoad = errors.New("attempt to load config with no files")
+
+	// osLookupEnv allows simple mocking of os.LookupEnv for the test of that
+	// code path. Use Expand option
+	// for most test cases instead.
+	osLookupEnv = os.LookupEnv
+)
 
 // Options is an options set used when parsing config.
 type Options struct {
 	DisableUnmarshalStrict bool
 	DisableValidate        bool
-	Expand                 config.LookupFunc
+
+	// Expand provides values for templated strings of the form ${KEY}.
+	// By default, we extract these values from the environment.
+	Expand config.LookupFunc
 }
 
 // LoadFile loads a config from a file.
@@ -66,10 +77,13 @@ func LoadFiles(dst interface{}, files []string, opts Options) error {
 		yamlOpts = append(yamlOpts, config.Permissive())
 	}
 
-	if opts.Expand != nil {
-		yamlOpts = append(yamlOpts, config.Expand(opts.Expand))
+	expand := opts.Expand
+	if expand == nil {
+		expand = osLookupEnv
 	}
 
+	yamlOpts = append(yamlOpts, config.Expand(expand))
+
 	provider, err := config.NewYAML(yamlOpts...)
 	if err != nil {
 		return err
diff --git a/src/x/config/config_test.go b/src/x/config/config_test.go
index 234898b25b..ed5f7e4c47 100644
--- a/src/x/config/config_test.go
+++ b/src/x/config/config_test.go
@@ -317,17 +317,25 @@ func TestLoadFilesEnvExpansion(t *testing.T) {
 		}
 	}
 
-	cases := []struct {
+	newMapLookupOptions := func(m map[string]string) Options {
+		return Options{
+			Expand: mapLookup(m),
+		}
+	}
+
+	type testCase struct {
 		Name        string
-		Input       map[string]string
+		Options     Options
 		Expected    configuration
 		ExpectedErr error
-	}{{
+	}
+
+	cases := []testCase{{
 		Name: "all_provided",
-		Input: map[string]string{
+		Options: newMapLookupOptions(map[string]string{
 			"PORT":         "9090",
 			"BUFFER_SPACE": "256",
-		},
+		}),
 		Expected: configuration{
 			ListenAddress: "localhost:9090",
 			BufferSpace:   256,
@@ -335,16 +343,16 @@ func TestLoadFilesEnvExpansion(t *testing.T) {
 		},
 	}, {
 		Name: "missing_no_default",
-		Input: map[string]string{
+		Options: newMapLookupOptions(map[string]string{
 			"PORT": "9090",
 			// missing BUFFER_SPACE,
-		},
+		}),
 		ExpectedErr: errors.New("couldn't expand environment: default is empty for \"BUFFER_SPACE\" (use \"\" for empty string)"),
 	}, {
 		Name: "missing_with_default",
-		Input: map[string]string{
+		Options: newMapLookupOptions(map[string]string{
 			"BUFFER_SPACE": "256",
-		},
+		}),
 		Expected: configuration{
 			ListenAddress: "localhost:8080",
 			BufferSpace:   256,
@@ -352,26 +360,49 @@ func TestLoadFilesEnvExpansion(t *testing.T) {
 		},
 	}}
 
+	doTest := func(t *testing.T, tc testCase) {
+		fname1 := writeFile(t, withEnv)
+		defer func() {
+			require.NoError(t, os.Remove(fname1))
+		}()
+		var cfg configuration
+
+		err := LoadFile(&cfg, fname1, tc.Options)
+		if tc.ExpectedErr != nil {
+			require.EqualError(t, err, tc.ExpectedErr.Error())
+			return
+		}
+
+		require.NoError(t, err)
+		assert.Equal(t, tc.Expected, cfg)
+	}
+
 	for _, tc := range cases {
 		t.Run(tc.Name, func(t *testing.T) {
-			fname1 := writeFile(t, withEnv)
-			defer func() {
-				require.NoError(t, os.Remove(fname1))
-			}()
-			var cfg configuration
-
-			err := LoadFile(&cfg, fname1, Options{
-				Expand: mapLookup(tc.Input),
-			})
-			if tc.ExpectedErr != nil {
-				require.EqualError(t, err, tc.ExpectedErr.Error())
-				return
-			}
-
-			require.NoError(t, err)
-			assert.Equal(t, tc.Expected, cfg)
+			doTest(t, tc)
 		})
 	}
+
+	t.Run("uses os.LookupEnv by default", func(t *testing.T) {
+		curOSLookupEnv := osLookupEnv
+		osLookupEnv = mapLookup(map[string]string{
+			"PORT":         "9090",
+			"BUFFER_SPACE": "256",
+		})
+		defer func() {
+			osLookupEnv = curOSLookupEnv
+		}()
+
+		doTest(t, testCase{
+			// use defaults
+			Options: Options{},
+			Expected: configuration{
+				ListenAddress: "localhost:9090",
+				BufferSpace:   256,
+				Servers:       []string{"server2:8010"},
+			},
+		})
+	})
 }
 
 func TestDeprecationCheck(t *testing.T) {