Repository: argoproj/argo-cd
Pull request #10275 (2022-08-11 10:39:13)

fix: Trim white space from string slice retrieved from environment
See https://github.com/argoproj/argo-cd/pull/10270#discussion_r943025970

Also, while at this module, I took a shot at the unit tests for `util/env`, homogenizing them and add tests for `StringFromEnv()` and `StringsFromEnv()` functions.

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the *Details* link next to the DCO action for instructions on how to resolve this.

Checklist:

* [ ] Either (a) I've created an [enhancement proposal](https://github.com/argoproj/argo-cd/issues/new/choose) and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
* [ ] The title of the PR states what changed and the related issues number (used for the release note).
* [ ] I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
* [ ] I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
* [ ] Does this PR require documentation updates?
* [ ] I've updated documentation as required by this PR.
* [ ] Optional. My organization is added to USERS.md.
* [ ] I have signed off all my commits as required by [DCO](https://github.com/argoproj/argoproj/tree/master/community#contributing-to-argo)
* [ ] I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
* [ ] My build is green ([troubleshooting builds](https://argo-cd.readthedocs.io/en/latest/developer-guide/ci/)). 



codecov[bot]: # [Codecov](https://codecov.io/gh/argoproj/argo-cd/pull/10275?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=argoproj) Report
> Merging [#10275](https://codecov.io/gh/argoproj/argo-cd/pull/10275?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=argoproj) (31bf007) into [master](https://codecov.io/gh/argoproj/argo-cd/commit/5515cde64b7978246b0e9cbf44cfc66b68b6b89c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=argoproj) (5515cde) will **increase** coverage by `0.03%`.
> The diff coverage is `100.00%`.

```diff
@@            Coverage Diff             @@
##           master   #10275      +/-   ##
==========================================
+ Coverage   46.10%   46.13%   +0.03%     
==========================================
  Files         228      228              
  Lines       27958    27961       +3     
==========================================
+ Hits        12889    12901      +12     
+ Misses      13322    13314       -8     
+ Partials     1747     1746       -1     
```


| [Impacted Files](https://codecov.io/gh/argoproj/argo-cd/pull/10275?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=argoproj) | Coverage Δ | |
|---|---|---|
| [util/env/env.go](https://codecov.io/gh/argoproj/argo-cd/pull/10275/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=argoproj#diff-dXRpbC9lbnYvZW52Lmdv) | `100.00% <100.00%> (+13.04%)` | :arrow_up: |
| [util/settings/settings.go](https://codecov.io/gh/argoproj/argo-cd/pull/10275/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=argoproj#diff-dXRpbC9zZXR0aW5ncy9zZXR0aW5ncy5nbw==) | `51.36% <0.00%> (ø)` | |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=argoproj). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=argoproj)


Patch:
diff --git a/USERS.md b/USERS.md
index fce63f217..c72406eac 100644
--- a/USERS.md
+++ b/USERS.md
@@ -220,3 +220,4 @@ Currently, the following organizations are **officially** using Argo CD:
 1. [South China Morning Post (SCMP)](https://www.scmp.com/)
 1. [Lian Chu Securities](https://lczq.com)
 1. [gloat](https://gloat.com/)
+1. [Freshop, Inc](https://www.freshop.com/)
diff --git a/util/env/env.go b/util/env/env.go
index 910c4525f..dc1549082 100644
--- a/util/env/env.go
+++ b/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
 }


Test patch:
diff --git a/util/env/env_test.go b/util/env/env_test.go
index 10145dca6..691d23580 100644
--- a/util/env/env_test.go
+++ b/util/env/env_test.go
@@ -2,151 +2,105 @@ 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
@@ -154,49 +108,105 @@ func TestParseDurationFromEnv(t *testing.T) {
 		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)
+		})
+	}
 }