PS-1530: Viper -> Kong, rework controller entrypoint

Author: zhming0

cmd/controller/controller.go

@@ -1,13 +1,14 @@
 package controller_test
 
 import (
+	"os"
 	"testing"
 	"time"
 
 	"github.com/buildkite/agent-stack-k8s/v2/cmd/controller"
 	"github.com/buildkite/agent-stack-k8s/v2/internal/controller/config"
 	"github.com/google/go-cmp/cmp"
-	"github.com/spf13/cobra"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
@@ -164,22 +165,9 @@ func TestReadAndParseConfig(t *testing.T) {
 		},
 	}
 
-	// These need to be unset to as it is set in CI which pollutes the test environment
-	t.Setenv("INTEGRATION_TEST_BUILDKITE_TOKEN", "")
-	t.Setenv("IMAGE", "")
-	t.Setenv("NAMESPACE", "")
-	t.Setenv("AGENT_TOKEN_SECRET", "")
+	cleanTestEnv(t)
 
-	cmd := &cobra.Command{}
-	controller.AddConfigFlags(cmd)
-	v, err := controller.ReadConfigFromFileArgsAndEnv(cmd, []string{})
-	require.NoError(t, err)
-
-	// We need to read the config file from the test
-	v.SetConfigFile("../../examples/config.yaml")
-	require.NoError(t, v.ReadInConfig())
-
-	actual, err := controller.ParseAndValidateConfig(v)
+	actual, err := controller.BuildConfigFromArgs([]string{"--config=../../examples/config.yaml"})
 	require.NoError(t, err)
 
 	if diff := cmp.Diff(*actual, expected); diff != "" {
@@ -189,55 +177,257 @@ func TestReadAndParseConfig(t *testing.T) {
 
 func TestParseAndValidateConfig_DefaultResourceClassValidation(t *testing.T) {
 	tests := []struct {
-		name    string
-		config  map[string]any
-		wantErr string
+		name       string
+		configYAML string
+		wantErr    string
 	}{
 		{
 			name: "default references non-existent resource class",
-			config: map[string]any{
-				"agent-token-secret":          "test",
-				"image":                       "test:latest",
-				"job-active-deadline-seconds": 3600,
-				"namespace":                   "default",
-				"default-resource-class-name": "nonexistent",
-				"resource-classes": map[string]any{
-					"small": map[string]any{
-						"resource": map[string]any{
-							"requests": map[string]any{"cpu": "100m"},
-						},
-					},
-				},
-			},
+			configYAML: `
+default-resource-class-name: nonexistent
+resource-classes:
+  small:
+    resource:
+      requests:
+        cpu: "100m"
+`,
 			wantErr: `default-resource-class-name "nonexistent" not found in resource-classes`,
 		},
 		{
 			name: "default specified but no resource classes defined",
-			config: map[string]any{
-				"agent-token-secret":          "test",
-				"image":                       "test:latest",
-				"job-active-deadline-seconds": 3600,
-				"namespace":                   "default",
-				"default-resource-class-name": "small",
-			},
+			configYAML: `
+default-resource-class-name: small
+`,
 			wantErr: `default-resource-class-name "small" specified but no resource-classes defined`,
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			cmd := &cobra.Command{}
-			controller.AddConfigFlags(cmd)
-			v, err := controller.ReadConfigFromFileArgsAndEnv(cmd, []string{})
-			require.NoError(t, err)
+			cleanTestEnv(t)
+			configFile := createTempConfigFile(t, tt.configYAML)
 
-			for k, val := range tt.config {
-				v.Set(k, val)
-			}
-
-			_, err = controller.ParseAndValidateConfig(v)
+			_, err := controller.BuildConfigFromArgs([]string{"--config=" + configFile})
 			require.Error(t, err)
 			require.Contains(t, err.Error(), tt.wantErr)
 		})
 	}
 }
+
+func TestConfigPrecedence(t *testing.T) {
+	// Helper to build config with optional config file
+	buildConfig := func(t *testing.T, args []string, configFile string) (*config.Config, error) {
+		t.Helper()
+		if configFile != "" {
+			args = append([]string{"--config=" + configFile}, args...)
+		}
+		return controller.BuildConfigFromArgs(args)
+	}
+
+	t.Run("defaults applied when nothing set", func(t *testing.T) {
+		cleanTestEnv(t)
+
+		cfg, err := buildConfig(t, []string{}, "")
+		require.NoError(t, err)
+
+		assert.Equal(t, 25, cfg.MaxInFlight)
+		assert.Equal(t, "default", cfg.Namespace)
+		assert.Equal(t, 10*time.Minute, cfg.JobTTL)
+		assert.False(t, cfg.Debug)
+	})
+
+	t.Run("config file overrides defaults", func(t *testing.T) {
+		cleanTestEnv(t)
+		configFile := createTempConfigFile(t, `
+max-in-flight: 100
+namespace: from-file
+debug: true
+`)
+
+		cfg, err := buildConfig(t, []string{}, configFile)
+		require.NoError(t, err)
+
+		assert.Equal(t, 100, cfg.MaxInFlight)
+		assert.Equal(t, "from-file", cfg.Namespace)
+		assert.True(t, cfg.Debug)
+	})
+
+	t.Run("config file can set zero value", func(t *testing.T) {
+		cleanTestEnv(t)
+		configFile := createTempConfigFile(t, `max-in-flight: 0`)
+
+		cfg, err := buildConfig(t, []string{}, configFile)
+		require.NoError(t, err)
+
+		assert.Equal(t, 0, cfg.MaxInFlight)
+	})
+
+	t.Run("config file can set false", func(t *testing.T) {
+		cleanTestEnv(t)
+		configFile := createTempConfigFile(t, `debug: false`)
+
+		cfg, err := buildConfig(t, []string{}, configFile)
+		require.NoError(t, err)
+
+		assert.False(t, cfg.Debug)
+	})
+
+	t.Run("env var overrides config file", func(t *testing.T) {
+		cleanTestEnv(t)
+		configFile := createTempConfigFile(t, `
+max-in-flight: 100
+namespace: from-file
+`)
+		t.Setenv("MAX_IN_FLIGHT", "50")
+		t.Setenv("NAMESPACE", "from-env")
+
+		cfg, err := buildConfig(t, []string{}, configFile)
+		require.NoError(t, err)
+
+		assert.Equal(t, 50, cfg.MaxInFlight)
+		assert.Equal(t, "from-env", cfg.Namespace)
+	})
+
+	t.Run("env var can override config file with zero", func(t *testing.T) {
+		cleanTestEnv(t)
+		configFile := createTempConfigFile(t, `max-in-flight: 100`)
+		t.Setenv("MAX_IN_FLIGHT", "0")
+
+		cfg, err := buildConfig(t, []string{}, configFile)
+		require.NoError(t, err)
+
+		assert.Equal(t, 0, cfg.MaxInFlight)
+	})
+
+	t.Run("env var can override config file with false", func(t *testing.T) {
+		cleanTestEnv(t)
+		configFile := createTempConfigFile(t, `debug: true`)
+		t.Setenv("DEBUG", "false")
+
+		cfg, err := buildConfig(t, []string{}, configFile)
+		require.NoError(t, err)
+
+		assert.False(t, cfg.Debug)
+	})
+
+	t.Run("CLI overrides env var", func(t *testing.T) {
+		cleanTestEnv(t)
+		t.Setenv("MAX_IN_FLIGHT", "50")
+		t.Setenv("NAMESPACE", "from-env")
+
+		cfg, err := buildConfig(t, []string{
+			"--max-in-flight=200",
+			"--namespace=from-cli",
+		}, "")
+		require.NoError(t, err)
+
+		assert.Equal(t, 200, cfg.MaxInFlight)
+		assert.Equal(t, "from-cli", cfg.Namespace)
+	})
+
+	t.Run("CLI can override env var with zero", func(t *testing.T) {
+		cleanTestEnv(t)
+		t.Setenv("MAX_IN_FLIGHT", "50")
+
+		cfg, err := buildConfig(t, []string{"--max-in-flight=0"}, "")
+		require.NoError(t, err)
+
+		assert.Equal(t, 0, cfg.MaxInFlight)
+	})
+
+	t.Run("CLI can override env var with false", func(t *testing.T) {
+		cleanTestEnv(t)
+		t.Setenv("DEBUG", "true")
+
+		cfg, err := buildConfig(t, []string{"--debug=false"}, "")
+		require.NoError(t, err)
+
+		assert.False(t, cfg.Debug)
+	})
+
+	t.Run("full precedence chain", func(t *testing.T) {
+		cleanTestEnv(t)
+		configFile := createTempConfigFile(t, `
+max-in-flight: 100
+namespace: from-file
+job-ttl: 5m
+`)
+		t.Setenv("NAMESPACE", "from-env")
+		t.Setenv("JOB_TTL", "15m")
+
+		cfg, err := buildConfig(t, []string{"--namespace=from-cli"}, configFile)
+		require.NoError(t, err)
+
+		// max-in-flight: only in file
+		assert.Equal(t, 100, cfg.MaxInFlight)
+		// namespace: file -> env -> cli (cli wins)
+		assert.Equal(t, "from-cli", cfg.Namespace)
+		// job-ttl: file -> env (env wins)
+		assert.Equal(t, 15*time.Minute, cfg.JobTTL)
+	})
+
+	t.Run("duration fields work correctly", func(t *testing.T) {
+		cleanTestEnv(t)
+		configFile := createTempConfigFile(t, `
+job-ttl: 30m
+poll-interval: 5s
+`)
+
+		cfg, err := buildConfig(t, []string{}, configFile)
+		require.NoError(t, err)
+
+		assert.Equal(t, 30*time.Minute, cfg.JobTTL)
+		assert.Equal(t, 5*time.Second, cfg.PollInterval)
+	})
+
+	t.Run("config file with unknown field is rejected", func(t *testing.T) {
+		cleanTestEnv(t)
+		configFile := createTempConfigFile(t, `
+namespace: my-namespace
+unknown-field: some-value
+`)
+
+		_, err := buildConfig(t, []string{}, configFile)
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "unknown-field")
+	})
+}
+
+// cleanTestEnv unsets environment variables that might be set in CI or .envrc
+// which could pollute the test environment.
+func cleanTestEnv(t *testing.T) {
+	t.Helper()
+	for _, env := range []string{
+		"BUILDKITE_TOKEN",
+		"INTEGRATION_TEST_BUILDKITE_TOKEN",
+		"IMAGE",
+		"NAMESPACE",
+		"AGENT_TOKEN_SECRET",
+		"IMAGE_PULL_BACKOFF_GRACE_PERIOD",
+		"CONFIG",
+		"MAX_IN_FLIGHT",
+		"DEBUG",
+		"JOB_TTL",
+	} {
+		t.Setenv(env, "")
+		os.Unsetenv(env)
+	}
+}
+
+// createTempConfigFile creates a temporary config file with the given content
+// and returns its path. The file is automatically cleaned up after the test.
+func createTempConfigFile(t *testing.T, content string) string {
+	t.Helper()
+	f, err := os.CreateTemp("", "config-*.yaml")
+	if err != nil {
+		t.Fatalf("failed to create temp config file: %v", err)
+	}
+	if _, err := f.WriteString(content); err != nil {
+		t.Fatalf("failed to write temp config file: %v", err)
+	}
+	if err := f.Close(); err != nil {
+		t.Fatalf("failed to close temp config file: %v", err)
+	}
+	t.Cleanup(func() { os.Remove(f.Name()) })
+	return f.Name()
+}