config: use semi-colon instead of comma to separate ATHENS_GO_BINARY_… (#1531)

* config: use semi-colon instead of comma to separate ATHENS_GO_BINARY_ENV_VARS

* document behavior

config.dev.toml

@@ -35,6 +35,22 @@ GoProxy = "direct"
 # Although you can pass any key=value to the Go command here, you can see
 # the list of possible env vars by running `go env`.
 # Env override: ATHENS_GO_BINARY_ENV_VARS
+# 
+# IMPORTANT note about using the env var to override this config:
+#
+# You must use a semi-colon (;) to separate multiple env vars
+# within ATHENS_GO_BINARY_ENV_VARS. For example:
+# ATHENS_GO_BINARY_ENV_VARS='GOPROXY=proxy.golang.org,direct; GOPRIVATE=github.com/gomods/*'
+# The semi-colon is here used instead of the comma (,) because the comma is a valid value to 
+# separate arguments in certain go env vars such as GOPROXY and GOPRIVATE
+#
+# NOTE that if you use the env var, then whatever you have in this config file will be overridden 
+# and NOT appended/merged. In other words, if the config file value is 
+# GoBinaryEnvVars = ["GOPROXY=direct"]
+# And you pass the following env var: 
+# ATHENS_GO_BINARY_ENV_VARS='GODEBUG=true'
+# Then the final value that the Go binary will receive is [GOBINARY=true] and NOT ["GOPROXY=direct", "GOBINARY=true"]
+# Therefore, whether you use the config file or the env var, make sure you have all the values you need there.
 GoBinaryEnvVars = ["GOPROXY=direct"]
 
 # GoGetWorkers specifies how many times you can concurrently

pkg/config/config.go

@@ -80,6 +80,31 @@ func (el *EnvList) Add(key, value string) {
 	*el = append(*el, key+"="+value)
 }
 
+// Decode implements envconfig.Decoder. Please see the below link for more information on
+// that interface:
+//
+// https://github.com/kelseyhightower/envconfig#custom-decoders
+//
+// We are doing this to allow for very long lists of assignments to be set inside of
+// a single environment variable. For example:
+//
+//	ATHENS_GO_BINARY_ENV_VARS="GOPRIVATE=*.corp.example.com,rsc.io/private; GOPROXY=direct"
+//
+// See the below link for more information:
+// https://github.com/gomods/athens/issues/1404
+func (el *EnvList) Decode(value string) error {
+	const op errors.Op = "envList.Decode"
+	if value == "" {
+		return nil
+	}
+	*el = EnvList{} // env vars must override config file
+	assignments := strings.Split(value, ";")
+	for _, assignment := range assignments {
+		*el = append(*el, strings.TrimSpace(assignment))
+	}
+	return el.Validate()
+}
+
 // Validate validates that all strings inside the
 // list are of the key=value format
 func (el EnvList) Validate() error {

pkg/config/config_test.go

@@ -11,6 +11,8 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
+	"github.com/kelseyhightower/envconfig"
+	"github.com/stretchr/testify/require"
 )
 
 func testConfigFile(t *testing.T) (testConfigFile string) {
@@ -512,3 +514,120 @@ func TestEnvList(t *testing.T) {
 		t.Fatalf("expected err to be nil but got %v", err)
 	}
 }
+
+type decodeTestCase struct {
+	name     string
+	pre      EnvList
+	given    string
+	valid    bool
+	expected EnvList
+}
+
+var envListDecodeTests = []decodeTestCase{
+	{
+		name:     "empty",
+		pre:      EnvList{},
+		given:    "",
+		valid:    true,
+		expected: EnvList{},
+	},
+	{
+		name:     "unchanged",
+		pre:      EnvList{"GOPROXY=direct"},
+		given:    "",
+		valid:    true,
+		expected: EnvList{"GOPROXY=direct"},
+	},
+	{
+		name:     "must not merge",
+		pre:      EnvList{"GOPROXY=direct"},
+		given:    "GOPRIVATE=github.com/gomods/*",
+		valid:    true,
+		expected: EnvList{"GOPRIVATE=github.com/gomods/*"},
+	},
+	{
+		name:     "must override",
+		pre:      EnvList{"GOPROXY=direct"},
+		given:    "GOPROXY=https://proxy.golang.org",
+		valid:    true,
+		expected: EnvList{"GOPROXY=https://proxy.golang.org"},
+	},
+	{
+		name:  "semi colon separator",
+		pre:   EnvList{"GOPROXY=direct", "GOPRIVATE="},
+		given: "GOPROXY=off; GOPRIVATE=marwan.io/*;GONUTS=lol;GODEBUG=dns=true",
+		valid: true,
+		expected: EnvList{
+			"GOPROXY=off",
+			"GOPRIVATE=marwan.io/*",
+			"GONUTS=lol",
+			"GODEBUG=dns=true",
+		},
+	},
+	{
+		name:  "with commas",
+		pre:   EnvList{"GOPROXY=direct", "GOPRIVATE="},
+		given: "GOPROXY=proxy.golang.org,direct;GOPRIVATE=marwan.io/*;GONUTS=lol;GODEBUG=dns=true",
+		valid: true,
+		expected: EnvList{
+			"GOPROXY=proxy.golang.org,direct",
+			"GOPRIVATE=marwan.io/*",
+			"GONUTS=lol",
+			"GODEBUG=dns=true",
+		},
+	},
+	{
+		name:  "invalid",
+		pre:   EnvList{},
+		given: "GOPROXY=direct; INVALID",
+		valid: false,
+	},
+	{
+		name:     "accept empty value",
+		pre:      EnvList{"GOPROXY=direct"},
+		given:    "GOPROXY=; GOPRIVATE=github.com/*",
+		valid:    true,
+		expected: EnvList{"GOPROXY=", "GOPRIVATE=github.com/*"},
+	},
+}
+
+func TestEnvListDecode(t *testing.T) {
+	for _, tc := range envListDecodeTests {
+		t.Run(tc.name, func(t *testing.T) {
+			testDecode(t, tc)
+		})
+	}
+	cfg := &Config{
+		GoBinaryEnvVars: EnvList{"GOPROXY=direct"},
+	}
+	err := cfg.GoBinaryEnvVars.Decode("GOPROXY=https://proxy.golang.org; GOPRIVATE=github.com/gomods/*")
+	if err != nil {
+		t.Fatal(err)
+	}
+	cfg.GoBinaryEnvVars.Validate()
+}
+
+func testDecode(t *testing.T, tc decodeTestCase) {
+	const envKey = "ATHENS_LIST_TEST"
+
+	os.Setenv(envKey, tc.given)
+	defer func() {
+		require.NoError(t, os.Unsetenv(envKey))
+	}()
+
+	var config struct {
+		GoBinaryEnvVars EnvList `envconfig:"ATHENS_LIST_TEST"`
+	}
+	config.GoBinaryEnvVars = tc.pre
+	err := envconfig.Process("", &config)
+	if tc.valid && err != nil {
+		t.Fatal(err)
+	}
+	if !tc.valid {
+		if err == nil {
+			t.Fatal("expected an error but got nil")
+		}
+		return
+	}
+	require.Equal(t, tc.expected, config.GoBinaryEnvVars)
+}