fix: Trim white space from string slice retrieved from environment

util/env/env.go

@@ -126,9 +126,16 @@ func StringFromEnv(env string, defaultValue string) string {
 	return defaultValue
 }
 
+// StringsFromEnv parses given value from the environment as a list of strings,
+// using seperator as the delimeter, and returns them as a slice. The strings
+// in the returned slice will have leading and trailing white space removed.
 func StringsFromEnv(env string, defaultValue []string, separator string) []string {
 	if str := os.Getenv(env); str != "" {
-		return strings.Split(str, separator)
+		ss := strings.Split(str, separator)
+		for i, s := range ss {
+			ss[i] = strings.TrimSpace(s)
+		}
+		return ss
 	}
 	return defaultValue
 }

util/env/env_test.go

@@ -2,201 +2,211 @@ package env
 
 import (
 	"fmt"
-	"io"
 	"math"
-	"os"
 	"testing"
 	"time"
 
-	util "github.com/argoproj/argo-cd/v2/util/io"
-
 	"github.com/stretchr/testify/assert"
 )
 
-// nolint:unparam
-func setEnv(t *testing.T, env string, val string) io.Closer {
-	assert.NoError(t, os.Setenv(env, val))
-	return util.NewCloser(func() error {
-		assert.NoError(t, os.Setenv(env, ""))
-		return nil
-	})
-}
-
-func TestParseNumFromEnv_NoEnvVariable(t *testing.T) {
-	num := ParseNumFromEnv("test", 10, 0, 100)
-
-	assert.Equal(t, 10, num)
-}
-
-func TestParseNumFromEnv_CorrectValueSet(t *testing.T) {
-	closer := setEnv(t, "test", "15")
-	defer util.Close(closer)
-
-	num := ParseNumFromEnv("test", 10, 0, 100)
-
-	assert.Equal(t, 15, num)
-}
-
-func TestParseNumFromEnv_NonIntValueSet(t *testing.T) {
-	closer := setEnv(t, "test", "wrong")
-	defer util.Close(closer)
-
-	num := ParseNumFromEnv("test", 10, 0, 100)
-
-	assert.Equal(t, 10, num)
-}
-
-func TestParseNumFromEnv_NegativeValueSet(t *testing.T) {
-	closer := setEnv(t, "test", "-1")
-	defer util.Close(closer)
-
-	num := ParseNumFromEnv("test", 10, 0, 100)
-
-	assert.Equal(t, 10, num)
-}
-
-func TestParseNumFromEnv_OutOfRangeValueSet(t *testing.T) {
-	closer := setEnv(t, "test", "1000")
-	defer util.Close(closer)
-
-	num := ParseNumFromEnv("test", 10, 0, 100)
+func TestParseNumFromEnv(t *testing.T) {
+	const envKey = "SOMEKEY"
+	const min = math.MinInt + 1
+	const max = math.MaxInt - 1
+	const def = 10
+	testCases := []struct {
+		name     string
+		env      string
+		expected int
+	}{
+		{"Valid positive number", "200", 200},
+		{"Valid negative number", "-200", -200},
+		{"Invalid number", "abc", def},
+		{"Equals minimum", fmt.Sprintf("%d", math.MinInt+1), min},
+		{"Equals maximum", fmt.Sprintf("%d", math.MaxInt-1), max},
+		{"Less than minimum", fmt.Sprintf("%d", math.MinInt), def},
+		{"Greater than maximum", fmt.Sprintf("%d", math.MaxInt), def},
+		{"Variable not set", "", def},
+	}
 
-	assert.Equal(t, 10, num)
+	for _, tt := range testCases {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Setenv(envKey, tt.env)
+			n := ParseNumFromEnv(envKey, def, min, max)
+			assert.Equal(t, tt.expected, n)
+		})
+	}
 }
 
 func TestParseFloatFromEnv(t *testing.T) {
-	t.Run("Env not set", func(t *testing.T) {
-		closer := setEnv(t, "test", "")
-		defer util.Close(closer)
-		f := ParseFloatFromEnv("test", 1, 0, math.MaxFloat32)
-		assert.Equal(t, float32(1.0), f)
-	})
-	t.Run("Parse valid float", func(t *testing.T) {
-		closer := setEnv(t, "test", "2.5")
-		defer util.Close(closer)
-		f := ParseFloatFromEnv("test", 1, 0, math.MaxFloat32)
-		assert.Equal(t, float32(2.5), f)
-	})
-	t.Run("Parse valid integer as float", func(t *testing.T) {
-		closer := setEnv(t, "test", "2")
-		defer util.Close(closer)
-		f := ParseFloatFromEnv("test", 1, 0, math.MaxFloat32)
-		assert.Equal(t, float32(2.0), f)
-	})
-	t.Run("Parse invalid value", func(t *testing.T) {
-		closer := setEnv(t, "test", "foo")
-		defer util.Close(closer)
-		f := ParseFloatFromEnv("test", 1, 0, math.MaxFloat32)
-		assert.Equal(t, float32(1.0), f)
-	})
-	t.Run("Float lesser than allowed", func(t *testing.T) {
-		closer := setEnv(t, "test", "-2.0")
-		defer util.Close(closer)
-		f := ParseFloatFromEnv("test", 1, 0, math.MaxFloat32)
-		assert.Equal(t, float32(1.0), f)
-	})
-	t.Run("Float greater than allowed", func(t *testing.T) {
-		closer := setEnv(t, "test", "5.0")
-		defer util.Close(closer)
-		f := ParseFloatFromEnv("test", 1, 0, 4)
-		assert.Equal(t, float32(1.0), f)
-	})
-	t.Run("Check float overflow returning default value", func(t *testing.T) {
-		closer := setEnv(t, "test", fmt.Sprintf("%f", math.MaxFloat32*2))
-		defer util.Close(closer)
-		f := ParseFloatFromEnv("test", 1, 0, math.MaxFloat32)
-		assert.Equal(t, float32(1.0), f)
-	})
+	const envKey = "SOMEKEY"
+	var min float32 = -1000.5
+	var max float32 = 1000.5
+	const def float32 = 10.5
+	testCases := []struct {
+		name     string
+		env      string
+		expected float32
+	}{
+		{"Valid positive float", "2.0", 2.0},
+		{"Valid negative float", "-2.0", -2.0},
+		{"Valid integer as float", "2", 2.0},
+		{"Text as invalid float", "abc", def},
+		{"Equals maximum", fmt.Sprintf("%v", max), max},
+		{"Equals minimum", fmt.Sprintf("%v", min), min},
+		{"Greater than maximum", fmt.Sprintf("%f", max+1), def},
+		{"Lesser than minimum", fmt.Sprintf("%f", min-1), def},
+		{"Environment not set at", "", def},
+	}
+
+	for _, tt := range testCases {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Setenv(envKey, tt.env)
+			f := ParseFloatFromEnv(envKey, def, min, max)
+			assert.Equal(t, tt.expected, f)
+		})
+	}
 }
 
 func TestParseInt64FromEnv(t *testing.T) {
-	t.Run("Env not set", func(t *testing.T) {
-		closer := setEnv(t, "test", "")
-		defer util.Close(closer)
-		i := ParseInt64FromEnv("test", 1, 0, math.MaxInt64)
-		assert.Equal(t, int64(1), i)
-	})
-	t.Run("Parse valid int64", func(t *testing.T) {
-		closer := setEnv(t, "test", "3")
-		defer util.Close(closer)
-		i := ParseInt64FromEnv("test", 1, 0, math.MaxInt64)
-		assert.Equal(t, int64(3), i)
-	})
-	t.Run("Parse invalid value", func(t *testing.T) {
-		closer := setEnv(t, "test", "foo")
-		defer util.Close(closer)
-		i := ParseInt64FromEnv("test", 1, 0, math.MaxInt64)
-		assert.Equal(t, int64(1), i)
-	})
-	t.Run("Int64 lesser than allowed", func(t *testing.T) {
-		closer := setEnv(t, "test", "-2")
-		defer util.Close(closer)
-		i := ParseInt64FromEnv("test", 1, 0, math.MaxInt64)
-		assert.Equal(t, int64(1), i)
-	})
-	t.Run("Int64 greater than allowed", func(t *testing.T) {
-		closer := setEnv(t, "test", "5")
-		defer util.Close(closer)
-		i := ParseInt64FromEnv("test", 1, 0, 4)
-		assert.Equal(t, int64(1), i)
-	})
+	const envKey = "SOMEKEY"
+	const min int64 = 1
+	const max int64 = math.MaxInt64 - 1
+	const def int64 = 10
+	testCases := []struct {
+		name     string
+		env      string
+		expected int64
+	}{
+		{"Valid int64", "200", 200},
+		{"Text as invalid int64", "abc", def},
+		{"Equals maximum", fmt.Sprintf("%d", max), max},
+		{"Equals minimum", fmt.Sprintf("%d", min), min},
+		{"Greater than maximum", fmt.Sprintf("%d", max+1), def},
+		{"Less than minimum", fmt.Sprintf("%d", min-1), def},
+		{"Environment not set", "", def},
+	}
+
+	for _, tt := range testCases {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Setenv(envKey, tt.env)
+			n := ParseInt64FromEnv(envKey, def, min, max)
+			assert.Equal(t, tt.expected, n)
+		})
+	}
 }
 
 func TestParseDurationFromEnv(t *testing.T) {
-	testKey := "key"
-	defaultVal := 2 * time.Second
-	min := 1 * time.Second
-	max := 3 * time.Second
+	envKey := "SOMEKEY"
+	def := 3 * time.Second
+	min := 2 * time.Second
+	max := 5 * time.Second
 
 	testCases := []struct {
 		name     string
 		env      string
 		expected time.Duration
 	}{{
 		name:     "EnvNotSet",
-		expected: defaultVal,
+		expected: def,
 	}, {
 		name:     "ValidValueSet",
 		env:      "2s",
 		expected: time.Second * 2,
 	}, {
 		name:     "MoreThanMaxSet",
-		env:      "5s",
-		expected: defaultVal,
+		env:      "6s",
+		expected: def,
 	}, {
 		name:     "LessThanMinSet",
-		env:      "-1s",
-		expected: defaultVal,
+		env:      "1s",
+		expected: def,
 	}, {
 		name:     "InvalidSet",
 		env:      "hello",
-		expected: defaultVal,
+		expected: def,
 	}}
 
-	for i, tc := range testCases {
-		t.Run(testCases[i].name, func(t *testing.T) {
-			tc = testCases[i]
-			setEnv(t, testKey, tc.env)
-
-			val := ParseDurationFromEnv(testKey, defaultVal, min, max)
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Setenv(envKey, tc.env)
+			val := ParseDurationFromEnv(envKey, def, min, max)
 			assert.Equal(t, tc.expected, val)
 		})
 	}
 }
 
 func Test_ParseBoolFromEnv(t *testing.T) {
-	t.Run("Get 'true' value from existing env var", func(t *testing.T) {
-		_ = os.Setenv("TEST_BOOL_VAL", "true")
-		defer os.Setenv("TEST_BOOL_VAL", "")
-		assert.True(t, ParseBoolFromEnv("TEST_BOOL_VAL", false))
-	})
-	t.Run("Get 'false' value from existing env var", func(t *testing.T) {
-		_ = os.Setenv("TEST_BOOL_VAL", "false")
-		defer os.Setenv("TEST_BOOL_VAL", "")
-		assert.False(t, ParseBoolFromEnv("TEST_BOOL_VAL", true))
-	})
-	t.Run("Get default value from non-existing env var", func(t *testing.T) {
-		_ = os.Setenv("TEST_BOOL_VAL", "")
-		assert.True(t, ParseBoolFromEnv("TEST_BOOL_VAL", true))
-	})
+	envKey := "SOMEKEY"
+
+	testCases := []struct {
+		name     string
+		env      string
+		expected bool
+		def      bool
+	}{
+		{"True value", "true", true, false},
+		{"False value", "false", false, true},
+		{"Invalid value with true default", "somevalue", true, true},
+		{"Invalid value with false default", "somevalue", false, false},
+		{"Env not set", "", false, false},
+	}
+
+	for _, tt := range testCases {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Setenv(envKey, tt.env)
+			b := ParseBoolFromEnv(envKey, tt.def)
+			assert.Equal(t, tt.expected, b)
+		})
+	}
+}
+
+func TestStringFromEnv(t *testing.T) {
+	envKey := "SOMEKEY"
+	def := "somestring"
+
+	testCases := []struct {
+		name     string
+		env      string
+		expected string
+		def      string
+	}{
+		{"Some string", "true", "true", def},
+		{"Empty string with default", "", def, def},
+		{"Empty string without default", "", "", ""},
+	}
+
+	for _, tt := range testCases {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Setenv(envKey, tt.env)
+			b := StringFromEnv(envKey, tt.def)
+			assert.Equal(t, tt.expected, b)
+		})
+	}
+}
+
+func TestStringsFromEnv(t *testing.T) {
+	envKey := "SOMEKEY"
+	def := []string{"one", "two"}
+
+	testCases := []struct {
+		name     string
+		env      string
+		expected []string
+		def      []string
+		sep      string
+	}{
+		{"List of strings", "one,two,three", []string{"one", "two", "three"}, def, ","},
+		{"Comma separated with other delimeter", "one,two,three", []string{"one,two,three"}, def, ";"},
+		{"With trimmed white space", "one, two   ,    three", []string{"one", "two", "three"}, def, ","},
+		{"Env not set", "", def, def, ","},
+	}
+
+	for _, tt := range testCases {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Setenv(envKey, tt.env)
+			ss := StringsFromEnv(envKey, tt.def, tt.sep)
+			assert.Equal(t, tt.expected, ss)
+		})
+	}
 }