From e99dad5a9f37632efb89b40fb889498fb1c36796 Mon Sep 17 00:00:00 2001 From: Max Gagnon <82063767+maxwellgagnon@users.noreply.github.com> Date: Thu, 9 Oct 2025 15:34:35 -0700 Subject: [PATCH 01/14] Update tests for example, parser, and types; Add cpu and memory normalization --- internal/config/example_test.go | 82 +--- internal/config/parser.go | 493 ++++++++++++++++++++ internal/config/parser_test.go | 405 ++++++++++------ internal/config/types.go | 69 +-- internal/config/types_test.go | 793 ++++++++++++++++++++++---------- internal/config/validation.go | 50 -- 6 files changed, 1361 insertions(+), 531 deletions(-) diff --git a/internal/config/example_test.go b/internal/config/example_test.go index ecbc0e8..96ec178 100644 --- a/internal/config/example_test.go +++ b/internal/config/example_test.go @@ -25,91 +25,15 @@ func ExampleLoadDeveloperConfig() { // Access SSH keys sshKeys, err := cfg.GetSSHKeys() if err != nil { - log.Fatal(err) + fmt.Println("error:", err) + return } fmt.Printf("SSH Keys: %d configured\n", len(sshKeys)) // Output: // Developer: testuser - // CPU: 4 + // CPU: 4000m // Memory: 16Gi // User ID: 2000 // SSH Keys: 1 configured } - -// ExampleDevEnvConfig_CPU demonstrates flexible CPU resource handling -// with different input types and default values. -func ExampleDevEnvConfig_CPU() { - // Example with string CPU value - cfg1 := &config.DevEnvConfig{ - Name: "alice", - BaseConfig: config.BaseConfig{ - Resources: config.ResourceConfig{ - CPU: "8", // String value - }, - }, - } - fmt.Printf("String CPU: %s\n", cfg1.CPU()) - - // Example with integer CPU value - cfg2 := &config.DevEnvConfig{ - Name: "bob", - BaseConfig: config.BaseConfig{ - Resources: config.ResourceConfig{ - CPU: 4, // Integer value - }, - }, - } - fmt.Printf("Integer CPU: %s\n", cfg2.CPU()) - - // Example with no CPU specified (uses default) - cfg3 := &config.DevEnvConfig{ - Name: "charlie", - // No Resources specified - } - fmt.Printf("Default CPU: %s\n", cfg3.CPU()) - - // Output: - // String CPU: 8 - // Integer CPU: 4 - // Default CPU: 0 -} - -// ExampleDevEnvConfig_GetSSHKeys demonstrates SSH key handling with -// both single string and multiple string array formats. -func ExampleDevEnvConfig_GetSSHKeys() { - // Single SSH key as string - cfg1 := &config.DevEnvConfig{ - Name: "alice", - BaseConfig: config.BaseConfig{ - SSHPublicKey: "ssh-rsa AAAAB3NzaC1yc2EAAAADAQAB alice@example.com", - }, - } - - keys1, err := cfg1.GetSSHKeys() - if err != nil { - log.Fatal(err) - } - fmt.Printf("Single key: %d SSH key(s)\n", len(keys1)) - - // Multiple SSH keys as slice - cfg2 := &config.DevEnvConfig{ - Name: "bob", - BaseConfig: config.BaseConfig{ - SSHPublicKey: []interface{}{ - "ssh-rsa AAAAB3NzaC1yc2EAAAADAQAB bob@work.com", - "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAI bob@home.com", - }, - }, - } - - keys2, err := cfg2.GetSSHKeys() - if err != nil { - log.Fatal(err) - } - fmt.Printf("Multiple keys: %d SSH key(s)\n", len(keys2)) - - // Output: - // Single key: 1 SSH key(s) - // Multiple keys: 2 SSH key(s) -} diff --git a/internal/config/parser.go b/internal/config/parser.go index 4e9d347..c048039 100644 --- a/internal/config/parser.go +++ b/internal/config/parser.go @@ -2,8 +2,11 @@ package config import ( "fmt" + "math" "os" "path/filepath" + "strconv" + "strings" "gopkg.in/yaml.v3" ) @@ -33,6 +36,13 @@ func LoadGlobalConfig(configDir string) (*BaseConfig, error) { return nil, fmt.Errorf("failed to parse YAML in global config %s: %w", globalConfigPath, err) } + if err := normalizeCPU(&globalConfig.Resources); err != nil { + return nil, fmt.Errorf("normalize cpu: %w", err) + } + + if err := normalizeMemory(&globalConfig.Resources); err != nil { + return nil, fmt.Errorf("normalize memory: %w", err) + } return &globalConfig, nil } @@ -68,6 +78,14 @@ func LoadDeveloperConfig(configDir, developerName string) (*DevEnvConfig, error) config.DeveloperDir = developerDir + if err := normalizeCPU(&config.Resources); err != nil { + return nil, fmt.Errorf("normalize cpu: %w", err) + } + + if err := normalizeMemory(&config.Resources); err != nil { + return nil, fmt.Errorf("normalize memory: %w", err) + } + // Basic validation if err := config.Validate(); err != nil { return nil, fmt.Errorf("invalid configuration in %s: %w", configPath, err) @@ -113,6 +131,15 @@ func LoadDeveloperConfigWithBaseConfig(configDir, developerName string, baseConf // Step 6: Set developer directory and validate userConfig.DeveloperDir = developerDir + // Step 7: Normalize flexible/raw fields → canonical representation + if err := normalizeCPU(&userConfig.Resources); err != nil { + return nil, fmt.Errorf("normalize cpu: %w", err) + } + + if err := normalizeMemory(&userConfig.Resources); err != nil { + return nil, fmt.Errorf("normalize memory: %w", err) + } + if err := userConfig.Validate(); err != nil { return nil, fmt.Errorf("invalid configuration in %s: %w", configPath, err) } @@ -216,3 +243,469 @@ func mergeVolumes(global, user []VolumeMount) []VolumeMount { return result } + +// normalizeSSHKeys converts the flexible SSH key field to a string slice +// Handles both single string and string array formats from YAML +func normalizeSSHKeys(sshKeyField any) ([]string, error) { + if sshKeyField == nil { + return []string{}, nil + } + + switch keys := sshKeyField.(type) { + case string: + s := strings.TrimSpace(keys) + // Single SSH key + if s == "" { + return []string{}, fmt.Errorf("SSH key cannot be empty string") + } + return []string{s}, nil + + case []string: + // Direct string slice + if len(keys) == 0 { + return nil, fmt.Errorf("SSH key array cannot be empty") + } + out := make([]string, len(keys)) + for i, k := range keys { + s := strings.TrimSpace(k) + if s == "" { + return nil, fmt.Errorf("SSH key at index %d cannot be empty", i) + } + out[i] = s + } + return out, nil + + case []any: // alias of []interface{} + if len(keys) == 0 { + return nil, fmt.Errorf("SSH key array cannot be empty") + } + out := make([]string, len(keys)) + // Array of SSH keys (from YAML) + for i, e := range keys { + s, ok := e.(string) + if !ok { + return nil, fmt.Errorf("SSH key at index %d is not a string", i) + } + s = strings.TrimSpace(s) + if s == "" { + return nil, fmt.Errorf("SSH key at index %d cannot be empty", i) + } + out[i] = s + } + return out, nil + + default: + return nil, fmt.Errorf("SSH key field must be string or array of strings, got %T", sshKeyField) + } +} + +// normalizeCPU reads r.CPURaw and sets r.CPU (millicores). +// With strict policy, invalid or non-positive inputs propagate as zero with an error, or +// you can choose to return ok=false and let the loader decide. Here I return an error. +func normalizeCPU(r *ResourceConfig) error { + if r.CPURaw == nil { + return nil // no user value; keep default already in r.CPU + } + m, ok := toMillicores(r.CPURaw) + if !ok { + // make policy explicit: treat invalid as an error + return fmt.Errorf("invalid cpu value: %v", r.CPURaw) + } + r.CPU = m + return nil +} + +// toMillicores converts a flexible CPU value to millicores. +// Policy: syntactically valid but non-positive (<=0) values are INVALID → ok=false. +func toMillicores(v any) (int64, bool) { + switch x := v.(type) { + case nil: + return 0, false + + case string: + s := strings.TrimSpace(x) + if s == "" { + return 0, false + } + // Already in millicores: "500m" + if strings.HasSuffix(s, "m") { + d := strings.TrimSpace(strings.TrimSuffix(s, "m")) + if !allDigits(d) { + return 0, false + } + n, err := strconv.ParseInt(d, 10, 64) + if err != nil || n <= 0 { + return 0, false + } + return n, true + } + // Plain number: "2", "2.5" + f, err := strconv.ParseFloat(s, 64) + if err != nil || f <= 0 { + return 0, false + } + return int64(math.Round(f * 1000.0)), true + + // Signed ints: cores + case int: + if x <= 0 { + return 0, false + } + return int64(x) * 1000, true + case int8: + if x <= 0 { + return 0, false + } + return int64(x) * 1000, true + case int16: + if x <= 0 { + return 0, false + } + return int64(x) * 1000, true + case int32: + if x <= 0 { + return 0, false + } + return int64(x) * 1000, true + case int64: + if x <= 0 { + return 0, false + } + return x * 1000, true + + // Unsigned ints: cores (cannot be negative) + case uint: + if x == 0 { + return 0, false + } + return int64(x) * 1000, true + case uint8: + if x == 0 { + return 0, false + } + return int64(x) * 1000, true + case uint16: + if x == 0 { + return 0, false + } + return int64(x) * 1000, true + case uint32: + if x == 0 { + return 0, false + } + return int64(x) * 1000, true + case uint64: + if x == 0 || x > math.MaxInt64/1000 { + return 0, false + } + return int64(x) * 1000, true + + // Floats (cores) + case float32: + f := float64(x) + if f <= 0 || math.IsNaN(f) || math.IsInf(f, 0) { + return 0, false + } + return int64(math.Round(f * 1000.0)), true + case float64: + if x <= 0 || math.IsNaN(x) || math.IsInf(x, 0) { + return 0, false + } + return int64(math.Round(x * 1000.0)), true + + default: + return 0, false + } +} + +func allDigits(s string) bool { + if s == "" { + return false + } + for _, r := range s { + if r < '0' || r > '9' { + return false + } + } + return true +} + +// normalizeMemory reads r.MemoryRaw (e.g., "16Gi", "512Mi", "500M", "1.5", 2, …) +// and sets r.Memory to Mi (mebibytes). Invalid or non-positive → 0. +func normalizeMemory(r *ResourceConfig) error { + if r.MemoryRaw == nil { + return nil // no user value; keep default already in r.Memory + } + mi, ok := toMi(r.MemoryRaw) + if !ok || mi <= 0 { + r.Memory = 0 + return nil + } + r.Memory = mi + return nil +} + +// toMi converts a flexible memory value into canonical Mi (mebibytes). +// Strict policy: syntactically valid but non-positive (<= 0) values are INVALID (ok=false). +func toMi(v any) (int64, bool) { + switch x := v.(type) { + case nil: + return 0, false + + case string: + s := strings.TrimSpace(x) + if s == "" { + return 0, false + } + return parseStringToMi(s) + + // Signed ints: interpret as Gi + case int: + if x <= 0 { + return 0, false + } + if int64(x) > math.MaxInt64/1024 { + return 0, false + } + return int64(x) * 1024, true + case int8: + if x <= 0 { + return 0, false + } + return int64(x) * 1024, true + case int16: + if x <= 0 { + return 0, false + } + return int64(x) * 1024, true + case int32: + if x <= 0 { + return 0, false + } + return int64(x) * 1024, true + case int64: + if x <= 0 || x > math.MaxInt64/1024 { + return 0, false + } + return x * 1024, true + + // Unsigned ints: interpret as Gi (guard overflow and zero) + case uint: + if x == 0 || uint64(x) > uint64(math.MaxInt64/1024) { + return 0, false + } + return int64(x) * 1024, true + case uint8: + if x == 0 { + return 0, false + } + return int64(x) * 1024, true + case uint16: + if x == 0 { + return 0, false + } + return int64(x) * 1024, true + case uint32: + if x == 0 { + return 0, false + } + return int64(x) * 1024, true + case uint64: + if x == 0 || x > uint64(math.MaxInt64/1024) { + return 0, false + } + return int64(x) * 1024, true + + // Floats: interpret as Gi + case float32: + return roundGiToMi(float64(x)) + case float64: + return roundGiToMi(x) + + default: + return 0, false + } +} + +// roundGiToMi converts Gi (float) to Mi with rounding. +// Returns (0,false) if gi <= 0, NaN/Inf, or rounding yields 0 Mi. +func roundGiToMi(gi float64) (int64, bool) { + if gi <= 0 || math.IsNaN(gi) || math.IsInf(gi, 0) { + return 0, false + } + miFloat := gi * 1024.0 + mi := int64(math.Round(miFloat)) + if mi <= 0 { + return 0, false + } + return mi, true +} + +// parseStringToMi handles binary units (Ki/Mi/Gi/Ti/Pi/Ei), +// decimal SI (k/M/G/T/P/E), and bare numeric strings (Gi). +// Returns (0,false) if value <= 0 or on invalid input. +func parseStringToMi(s string) (int64, bool) { + // ---- Binary SI (powers of 1024) ---- + if hasSuffixFold(s, "Ki") { + n, ok := parseNumber(strings.TrimSpace(s[:len(s)-2])) + if !ok || n <= 0 { + return 0, false + } + // KiB → MiB + miFloat := n / 1024.0 + mi := int64(math.Round(miFloat)) + if mi <= 0 { + return 0, false + } + return mi, true + } + if hasSuffixFold(s, "Mi") { + n, ok := parseNumber(strings.TrimSpace(s[:len(s)-2])) + if !ok || n <= 0 { + return 0, false + } + mi := int64(math.Round(n)) + if mi <= 0 { + return 0, false + } + return mi, true + } + if hasSuffixFold(s, "Gi") { + n, ok := parseNumber(strings.TrimSpace(s[:len(s)-2])) + if !ok || n <= 0 { + return 0, false + } + return roundGiToMi(n) + } + if hasSuffixFold(s, "Ti") { + n, ok := parseNumber(strings.TrimSpace(s[:len(s)-2])) + if !ok || n <= 0 { + return 0, false + } + miFloat := n * 1024.0 * 1024.0 + if miFloat > float64(math.MaxInt64) { + return 0, false + } + mi := int64(math.Round(miFloat)) + if mi <= 0 { + return 0, false + } + return mi, true + } + if hasSuffixFold(s, "Pi") { + n, ok := parseNumber(strings.TrimSpace(s[:len(s)-2])) + if !ok || n <= 0 { + return 0, false + } + miFloat := n * 1024.0 * 1024.0 * 1024.0 + if miFloat > float64(math.MaxInt64) { + return 0, false + } + mi := int64(math.Round(miFloat)) + if mi <= 0 { + return 0, false + } + return mi, true + } + if hasSuffixFold(s, "Ei") { + n, ok := parseNumber(strings.TrimSpace(s[:len(s)-2])) + if !ok || n <= 0 { + return 0, false + } + // Ei → Mi (giant); guard overflow + miFloat := n * 1024.0 * 1024.0 * 1024.0 * 1024.0 + if miFloat > float64(math.MaxInt64) { + return 0, false + } + mi := int64(math.Round(miFloat)) + if mi <= 0 { + return 0, false + } + return mi, true + } + + // ---- Decimal SI (powers of 1000) ---- + if hasSuffixFold(s, "k") { + n, ok := parseNumber(strings.TrimSpace(s[:len(s)-1])) + if !ok || n <= 0 { + return 0, false + } + return bytesToMi(n * 1e3) + } + if hasSuffixFold(s, "M") { + n, ok := parseNumber(strings.TrimSpace(s[:len(s)-1])) + if !ok || n <= 0 { + return 0, false + } + return bytesToMi(n * 1e6) + } + if hasSuffixFold(s, "G") { + n, ok := parseNumber(strings.TrimSpace(s[:len(s)-1])) + if !ok || n <= 0 { + return 0, false + } + return bytesToMi(n * 1e9) + } + if hasSuffixFold(s, "T") { + n, ok := parseNumber(strings.TrimSpace(s[:len(s)-1])) + if !ok || n <= 0 { + return 0, false + } + return bytesToMi(n * 1e12) + } + if hasSuffixFold(s, "P") { + n, ok := parseNumber(strings.TrimSpace(s[:len(s)-1])) + if !ok || n <= 0 { + return 0, false + } + return bytesToMi(n * 1e15) + } + if hasSuffixFold(s, "E") { + n, ok := parseNumber(strings.TrimSpace(s[:len(s)-1])) + if !ok || n <= 0 { + return 0, false + } + return bytesToMi(n * 1e18) + } + + // ---- Bare number: interpret as Gi (not bytes) ---- + if n, ok := parseNumber(s); ok && n > 0 { + return roundGiToMi(n) + } + return 0, false +} + +// bytesToMi converts decimal bytes to Mi with rounding. +// Returns (0,false) if bytes <= 0, NaN/Inf, or if the result would overflow or round to 0. +func bytesToMi(bytes float64) (int64, bool) { + if bytes <= 0 || math.IsNaN(bytes) || math.IsInf(bytes, 0) { + return 0, false + } + miFloat := bytes / (1024.0 * 1024.0) + if miFloat > float64(math.MaxInt64) { + return 0, false + } + mi := int64(math.Round(miFloat)) + if mi <= 0 { + return 0, false + } + return mi, true +} + +// parseNumber accepts integers/decimals/scientific notation (e.g., "1e6"). +func parseNumber(s string) (float64, bool) { + s = strings.TrimSpace(s) + if s == "" { + return 0, false + } + f, err := strconv.ParseFloat(s, 64) + if err != nil { + return 0, false + } + return f, true +} + +// Case-insensitive suffix check. +func hasSuffixFold(s, suf string) bool { + return len(s) >= len(suf) && strings.EqualFold(s[len(s)-len(suf):], suf) +} diff --git a/internal/config/parser_test.go b/internal/config/parser_test.go index 2151689..971bfaf 100644 --- a/internal/config/parser_test.go +++ b/internal/config/parser_test.go @@ -3,6 +3,7 @@ package config import ( "os" "path/filepath" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -24,63 +25,78 @@ resources: cpu: 4 memory: "16Gi" ` - err := os.WriteFile(globalConfigPath, []byte(globalConfigYAML), 0644) - require.NoError(t, err) + require.NoError(t, os.WriteFile(globalConfigPath, []byte(globalConfigYAML), 0o644)) // Load global config - config, err := LoadGlobalConfig(tempDir) + cfg, err := LoadGlobalConfig(tempDir) require.NoError(t, err) - // Test that YAML values override defaults - assert.Equal(t, "custom:latest", config.Image) - assert.False(t, config.InstallHomebrew) // Override default true - assert.Equal(t, 4, config.Resources.CPU) - assert.Equal(t, "16Gi", config.Resources.Memory) - assert.Equal(t, []string{"curl", "git"}, config.Packages.APT) - assert.Equal(t, []string{"requests"}, config.Packages.Python) - - // Test that unspecified values keep defaults - assert.True(t, config.ClearLocalPackages == false) // Default - assert.True(t, config.ClearVSCodeCache == false) // Default - assert.Equal(t, "/opt/venv/bin", config.PythonBinPath) // Default - assert.Equal(t, 1000, config.UID) // Default + // YAML values override defaults + assert.Equal(t, "custom:latest", cfg.Image) + assert.False(t, cfg.InstallHomebrew) // override default true + + // Canonical resource units: CPU in millicores, Memory in Mi + assert.Equal(t, int64(4000), cfg.Resources.CPU) // 4 cores -> 4000m + assert.Equal(t, int64(16*1024), cfg.Resources.Memory) // 16Gi -> 16384 Mi + + // Also verify formatted getters via DevEnvConfig wrapper + dev := &DevEnvConfig{BaseConfig: *cfg} + assert.Equal(t, "4000m", dev.CPU()) + assert.Equal(t, "16Gi", dev.Memory()) + + // Packages merged from YAML + assert.Equal(t, []string{"curl", "git"}, cfg.Packages.APT) + assert.Equal(t, []string{"requests"}, cfg.Packages.Python) + + // Unspecified fields keep defaults + assert.False(t, cfg.ClearLocalPackages) + assert.False(t, cfg.ClearVSCodeCache) + assert.Equal(t, "/opt/venv/bin", cfg.PythonBinPath) + assert.Equal(t, 1000, cfg.UID) + assert.Equal(t, "20Gi", cfg.Resources.Storage) // default storage unchanged + assert.Equal(t, 0, cfg.Resources.GPU) // default GPU unchanged }) - t.Run("global config file does not exist", func(t *testing.T) { - // Create empty temp directory + t.Run("global config file does not exist -> system defaults", func(t *testing.T) { tempDir := t.TempDir() - // Load global config from non-existent file - config, err := LoadGlobalConfig(tempDir) + cfg, err := LoadGlobalConfig(tempDir) require.NoError(t, err) - // Should return all defaults - assert.Equal(t, "ubuntu:22.04", config.Image) - assert.True(t, config.InstallHomebrew) - assert.False(t, config.ClearLocalPackages) - assert.False(t, config.ClearVSCodeCache) - assert.Equal(t, "/opt/venv/bin", config.PythonBinPath) - assert.Equal(t, 1000, config.UID) - assert.Equal(t, 2, config.Resources.CPU) - assert.Equal(t, "8Gi", config.Resources.Memory) - assert.Equal(t, []string{}, config.Packages.APT) - assert.Equal(t, []string{}, config.Packages.Python) + // Top-level defaults + assert.Equal(t, "ubuntu:22.04", cfg.Image) + assert.True(t, cfg.InstallHomebrew) + assert.False(t, cfg.ClearLocalPackages) + assert.False(t, cfg.ClearVSCodeCache) + assert.Equal(t, "/opt/venv/bin", cfg.PythonBinPath) + assert.Equal(t, 1000, cfg.UID) + + // Canonical resource defaults (CPU millicores, Memory Mi) + assert.Equal(t, int64(2000), cfg.Resources.CPU) // 2 cores + assert.Equal(t, int64(8*1024), cfg.Resources.Memory) // 8Gi + assert.Equal(t, "20Gi", cfg.Resources.Storage) + assert.Equal(t, 0, cfg.Resources.GPU) + + // Slices are non-nil and empty + assert.NotNil(t, cfg.Packages.APT) + assert.Len(t, cfg.Packages.APT, 0) + assert.NotNil(t, cfg.Packages.Python) + assert.Len(t, cfg.Packages.Python, 0) + assert.NotNil(t, cfg.Volumes) + assert.Len(t, cfg.Volumes, 0) }) - t.Run("invalid YAML in global config", func(t *testing.T) { + t.Run("invalid YAML in global config -> error", func(t *testing.T) { tempDir := t.TempDir() globalConfigPath := filepath.Join(tempDir, "devenv.yaml") - // Write invalid YAML - invalidYAML := `image: "test -installHomebrew: [invalid` - err := os.WriteFile(globalConfigPath, []byte(invalidYAML), 0644) - require.NoError(t, err) + invalidYAML := "image: \"test\ninstallHomebrew: [invalid" + require.NoError(t, os.WriteFile(globalConfigPath, []byte(invalidYAML), 0o644)) - // Should return error - _, err = LoadGlobalConfig(tempDir) - assert.Error(t, err) - assert.Contains(t, err.Error(), "failed to parse YAML") + _, err := LoadGlobalConfig(tempDir) + require.Error(t, err) + // Keep the substring check loose to avoid overfitting exact wording + assert.Contains(t, strings.ToLower(err.Error()), "parse") }) } @@ -88,13 +104,13 @@ func TestLoadDeveloperConfig(t *testing.T) { t.Run("valid developer config", func(t *testing.T) { tempDir := t.TempDir() developerDir := filepath.Join(tempDir, "alice") - err := os.MkdirAll(developerDir, 0755) - require.NoError(t, err) + require.NoError(t, os.MkdirAll(developerDir, 0o755)) configPath := filepath.Join(developerDir, "devenv-config.yaml") + // Include resources to exercise normalization to canonical units. configYAML := `name: alice sshPublicKey: - - "ssh-rsa AAAAB3NzaC1yc2E alice@example.com" + - "ssh-rsa AAAAB3NzaC1yc2EAAAADAQAB alice@example.com" sshPort: 30022 isAdmin: true git: @@ -103,70 +119,102 @@ git: packages: python: ["numpy", "pandas"] apt: ["vim"] +resources: + cpu: 4 + memory: "16Gi" ` - err = os.WriteFile(configPath, []byte(configYAML), 0644) - require.NoError(t, err) + require.NoError(t, os.WriteFile(configPath, []byte(configYAML), 0o644)) // Load developer config - config, err := LoadDeveloperConfig(tempDir, "alice") + cfg, err := LoadDeveloperConfig(tempDir, "alice") require.NoError(t, err) - // Test basic fields - assert.Equal(t, "alice", config.Name) - assert.Equal(t, 30022, config.SSHPort) - assert.True(t, config.IsAdmin) - assert.Equal(t, "Alice Smith", config.Git.Name) - assert.Equal(t, "alice@example.com", config.Git.Email) - assert.Equal(t, []string{"numpy", "pandas"}, config.Packages.Python) - assert.Equal(t, []string{"vim"}, config.Packages.APT) - - // Test SSH keys - sshKeys, err := config.GetSSHKeys() + // Basic fields + assert.Equal(t, "alice", cfg.Name) + assert.Equal(t, 30022, cfg.SSHPort) + assert.True(t, cfg.IsAdmin) + assert.Equal(t, "Alice Smith", cfg.Git.Name) + assert.Equal(t, "alice@example.com", cfg.Git.Email) + assert.Equal(t, []string{"numpy", "pandas"}, cfg.Packages.Python) + assert.Equal(t, []string{"vim"}, cfg.Packages.APT) + + // Canonical resources: CPU millicores, Memory Mi + assert.Equal(t, int64(4000), cfg.Resources.CPU) // 4 cores → 4000m + assert.Equal(t, int64(16*1024), cfg.Resources.Memory) // 16Gi → 16384 Mi + + // Getter formatting (K8s quantities) + assert.Equal(t, "4000m", cfg.CPU()) + assert.Equal(t, "16Gi", cfg.Memory()) + + // SSH keys (strict accessor) + keys, err := cfg.GetSSHKeys() require.NoError(t, err) - assert.Equal(t, []string{"ssh-rsa AAAAB3NzaC1yc2E alice@example.com"}, sshKeys) + assert.Equal(t, []string{"ssh-rsa AAAAB3NzaC1yc2EAAAADAQAB alice@example.com"}, keys) - // Test developer directory is set - assert.Equal(t, developerDir, config.DeveloperDir) + // DeveloperDir set + assert.Equal(t, developerDir, cfg.DeveloperDir) }) t.Run("config file not found", func(t *testing.T) { tempDir := t.TempDir() _, err := LoadDeveloperConfig(tempDir, "nonexistent") - assert.Error(t, err) - assert.Contains(t, err.Error(), "configuration file not found") + require.Error(t, err) + assert.Contains(t, strings.ToLower(err.Error()), "configuration file not found") }) - t.Run("invalid config - missing name", func(t *testing.T) { + t.Run("invalid config - missing SSH key", func(t *testing.T) { tempDir := t.TempDir() developerDir := filepath.Join(tempDir, "alice") - err := os.MkdirAll(developerDir, 0755) - require.NoError(t, err) + require.NoError(t, os.MkdirAll(developerDir, 0o755)) configPath := filepath.Join(developerDir, "devenv-config.yaml") - configYAML := `sshPublicKey: "ssh-rsa AAAAB3NzaC1yc2E alice@example.com"` - err = os.WriteFile(configPath, []byte(configYAML), 0644) - require.NoError(t, err) + configYAML := `name: alice` + require.NoError(t, os.WriteFile(configPath, []byte(configYAML), 0o644)) - _, err = LoadDeveloperConfig(tempDir, "alice") - assert.Error(t, err) - assert.Contains(t, err.Error(), "'Name' is required") + _, err := LoadDeveloperConfig(tempDir, "alice") + require.Error(t, err) + // Validation layer currently reports: "at least one SSH public key is required" + assert.Contains(t, strings.ToLower(err.Error()), "ssh public key") + assert.Contains(t, strings.ToLower(err.Error()), "required") }) - t.Run("invalid config - missing SSH key", func(t *testing.T) { + t.Run("invalid config - malformed SSH key", func(t *testing.T) { tempDir := t.TempDir() developerDir := filepath.Join(tempDir, "alice") - err := os.MkdirAll(developerDir, 0755) - require.NoError(t, err) + require.NoError(t, os.MkdirAll(developerDir, 0o755)) configPath := filepath.Join(developerDir, "devenv-config.yaml") - configYAML := `name: alice` - err = os.WriteFile(configPath, []byte(configYAML), 0644) - require.NoError(t, err) + configYAML := `name: alice +sshPublicKey: "ssh-rsa not-base64 user" +` + require.NoError(t, os.WriteFile(configPath, []byte(configYAML), 0o644)) - _, err = LoadDeveloperConfig(tempDir, "alice") - assert.Error(t, err) - assert.Contains(t, err.Error(), "SSH public key is required") + _, err := LoadDeveloperConfig(tempDir, "alice") + require.Error(t, err) + // Error may flow from ssh_keys validator or its wrapper message + assert.Contains(t, strings.ToLower(err.Error()), "ssh") + assert.Contains(t, strings.ToLower(err.Error()), "invalid") + }) + + t.Run("invalid config - bad CPU value", func(t *testing.T) { + tempDir := t.TempDir() + developerDir := filepath.Join(tempDir, "alice") + require.NoError(t, os.MkdirAll(developerDir, 0o755)) + + configPath := filepath.Join(developerDir, "devenv-config.yaml") + configYAML := `name: alice +sshPublicKey: "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAI alice@example.com" +resources: + cpu: "abc" # invalid + memory: "8Gi" +` + require.NoError(t, os.WriteFile(configPath, []byte(configYAML), 0o644)) + + _, err := LoadDeveloperConfig(tempDir, "alice") + require.Error(t, err) + // Depending on where it fails, message may indicate cpu invalid/parse/validation + assert.Contains(t, strings.ToLower(err.Error()), "cpu") }) } @@ -174,7 +222,7 @@ func TestLoadDeveloperConfigWithGlobalDefaults(t *testing.T) { t.Run("complete integration - global and user config", func(t *testing.T) { tempDir := t.TempDir() - // Create global config + // Global config (provides defaults + base packages + base SSH key) globalConfigYAML := `image: "global:latest" installHomebrew: true clearLocalPackages: true @@ -186,14 +234,11 @@ resources: memory: "16Gi" sshPublicKey: "ssh-rsa AAAAB3NzaC1yc2E admin@company.com" ` - globalConfigPath := filepath.Join(tempDir, "devenv.yaml") - err := os.WriteFile(globalConfigPath, []byte(globalConfigYAML), 0644) - require.NoError(t, err) + require.NoError(t, os.WriteFile(filepath.Join(tempDir, "devenv.yaml"), []byte(globalConfigYAML), 0o644)) - // Create user config + // User config (overrides and additive lists) developerDir := filepath.Join(tempDir, "alice") - err = os.MkdirAll(developerDir, 0755) - require.NoError(t, err) + require.NoError(t, os.MkdirAll(developerDir, 0o755)) userConfigYAML := `name: alice sshPublicKey: "ssh-rsa AAAAB3NzaC1yc2E alice@example.com" @@ -205,80 +250,83 @@ git: name: "Alice Smith" email: "alice@example.com" ` - configPath := filepath.Join(developerDir, "devenv-config.yaml") - err = os.WriteFile(configPath, []byte(userConfigYAML), 0644) - require.NoError(t, err) + require.NoError(t, os.WriteFile(filepath.Join(developerDir, "devenv-config.yaml"), []byte(userConfigYAML), 0o644)) - // Load global config - globalConfig, err := LoadGlobalConfig(tempDir) + // Load global (should normalize canonical CPU/Mem) + globalCfg, err := LoadGlobalConfig(tempDir) require.NoError(t, err) - // Load with global defaults - config, err := LoadDeveloperConfigWithBaseConfig(tempDir, "alice", globalConfig) + // Load user with global defaults as base (merge + normalize + validate) + cfg, err := LoadDeveloperConfigWithBaseConfig(tempDir, "alice", globalCfg) require.NoError(t, err) - // Test user-specific fields - assert.Equal(t, "alice", config.Name) - assert.Equal(t, "Alice Smith", config.Git.Name) - assert.Equal(t, "alice@example.com", config.Git.Email) + // User-specific fields + assert.Equal(t, "alice", cfg.Name) + assert.Equal(t, "Alice Smith", cfg.Git.Name) + assert.Equal(t, "alice@example.com", cfg.Git.Email) - // Test override fields (user overrides global) - assert.Equal(t, "global:latest", config.Image) // User didn't specify, uses global - assert.False(t, config.InstallHomebrew) // User overrides global true with false - assert.True(t, config.ClearLocalPackages) // User didn't specify, uses global - assert.Equal(t, 4, config.Resources.CPU) // User didn't specify, uses global - assert.Equal(t, "16Gi", config.Resources.Memory) // User didn't specify, uses global + // Overrides and inherited values + assert.Equal(t, "global:latest", cfg.Image) // user didn't specify; inherited from global + assert.False(t, cfg.InstallHomebrew) // user overrides global=true → false + assert.True(t, cfg.ClearLocalPackages) // inherited from global - // Test additive fields (global + user) - expectedAPT := []string{"curl", "git", "vim"} // Global + user packages - assert.Equal(t, expectedAPT, config.Packages.APT) + // Canonical resource units (CPU millicores, Memory MiB) + assert.Equal(t, int64(4000), cfg.Resources.CPU) // 4 cores → 4000m + assert.Equal(t, int64(16*1024), cfg.Resources.Memory) // 16Gi → 16384Mi + assert.Equal(t, "4000m", cfg.CPU()) // formatted getter + assert.Equal(t, "16Gi", cfg.Memory()) // formatted getter - expectedPython := []string{"requests", "pandas"} // Global + user packages - assert.Equal(t, expectedPython, config.Packages.Python) + // Additive list merging (global first, then user) + assert.Equal(t, []string{"curl", "git", "vim"}, cfg.Packages.APT) + assert.Equal(t, []string{"requests", "pandas"}, cfg.Packages.Python) - // Test SSH key merging (global + user) - sshKeys, err := config.GetSSHKeys() + // SSH keys merge (global + user). Order depends on your mergeListFields; this expects global first. + keys, err := cfg.GetSSHKeys() require.NoError(t, err) - expectedSSHKeys := []string{ - "ssh-rsa AAAAB3NzaC1yc2E admin@company.com", // Global - "ssh-rsa AAAAB3NzaC1yc2E alice@example.com", // User - } - assert.Equal(t, expectedSSHKeys, sshKeys) + assert.Equal(t, + []string{ + "ssh-rsa AAAAB3NzaC1yc2E admin@company.com", + "ssh-rsa AAAAB3NzaC1yc2E alice@example.com", + }, + keys, + ) - // Test developer directory is set - assert.Equal(t, developerDir, config.DeveloperDir) + // Developer directory set + assert.Equal(t, developerDir, cfg.DeveloperDir) }) t.Run("user config with no global config", func(t *testing.T) { tempDir := t.TempDir() - // Create only user config (no global config file) + // Only user config (no devenv.yaml) developerDir := filepath.Join(tempDir, "alice") - err := os.MkdirAll(developerDir, 0755) - require.NoError(t, err) + require.NoError(t, os.MkdirAll(developerDir, 0o755)) userConfigYAML := `name: alice sshPublicKey: "ssh-rsa AAAAB3NzaC1yc2E alice@example.com" installHomebrew: false ` - configPath := filepath.Join(developerDir, "devenv-config.yaml") - err = os.WriteFile(configPath, []byte(userConfigYAML), 0644) - require.NoError(t, err) + require.NoError(t, os.WriteFile(filepath.Join(developerDir, "devenv-config.yaml"), []byte(userConfigYAML), 0o644)) - // Load global config (should be all defaults) - globalConfig, err := LoadGlobalConfig(tempDir) + // Global = system defaults (no file present) + globalCfg, err := LoadGlobalConfig(tempDir) require.NoError(t, err) - // Load with global defaults - config, err := LoadDeveloperConfigWithBaseConfig(tempDir, "alice", globalConfig) + cfg, err := LoadDeveloperConfigWithBaseConfig(tempDir, "alice", globalCfg) require.NoError(t, err) - // Should get system defaults + user overrides - assert.Equal(t, "alice", config.Name) - assert.Equal(t, "ubuntu:22.04", config.Image) // System default - assert.False(t, config.InstallHomebrew) // User override - assert.False(t, config.ClearLocalPackages) // System default - assert.Equal(t, "/opt/venv/bin", config.PythonBinPath) // System default + // Defaults + user overrides + assert.Equal(t, "alice", cfg.Name) + assert.Equal(t, "ubuntu:22.04", cfg.Image) // system default + assert.False(t, cfg.InstallHomebrew) // user override + assert.False(t, cfg.ClearLocalPackages) // system default + assert.Equal(t, "/opt/venv/bin", cfg.PythonBinPath) // system default + + // Canonical resource defaults and formatted getters + assert.Equal(t, int64(2000), cfg.Resources.CPU) // default 2 cores + assert.Equal(t, int64(8*1024), cfg.Resources.Memory) // default 8Gi + assert.Equal(t, "2000m", cfg.CPU()) + assert.Equal(t, "8Gi", cfg.Memory()) }) } @@ -557,3 +605,90 @@ func TestNormalizeSSHKeys(t *testing.T) { }) } } + +func Test_toMi_NormalizesInputs(t *testing.T) { + tests := []struct { + name string + in any + want int64 // Mi + ok bool + }{ + // --- Binary units (powers of 2) --- + {"Gi exact", "16Gi", 16 * 1024, true}, + {"Mi exact", "512Mi", 512, true}, + {"Ki to Mi", "1024Ki", 1, true}, + {"Gi decimal", "2.5Gi", 2560, true}, + {"trim + casefold", " 2gi ", 2 * 1024, true}, // if you allow case-insensitive units + + // --- Decimal SI (powers of 10) --- + {"500M", "500M", 477, true}, // 500e6 / 2^20 ≈ 476.84 → round to 477 + {"1G", "1G", 954, true}, + + // --- Bare numeric strings (policy: treat as Gi) --- + {"bare int string", "15", 15 * 1024, true}, + {"bare float string", "1.5", 1536, true}, + + // --- Non-string numerics (policy: Gi) --- + {"int means Gi", 2, 2 * 1024, true}, + {"float means Gi", 1.5, 1536, true}, + {"uint means Gi", uint(4), 4 * 1024, true}, + + // --- Zero/negative are invalid --- + {"zero Gi invalid", "0Gi", 0, false}, + {"zero Mi invalid", "0Mi", 0, false}, + {"bare zero invalid", "0", 0, false}, + {"zero int invalid", 0, 0, false}, + {"negative Gi invalid", "-1Gi", 0, false}, + {"negative int invalid", -1, 0, false}, + + // --- Invalid shapes --- + {"invalid unit", "12GB", 0, false}, // unsupported 'GB' + {"nonnumeric", "abc", 0, false}, + {"nil", nil, 0, false}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got, ok := toMi(tc.in) + assert.Equal(t, tc.ok, ok) + assert.Equal(t, tc.want, got) + }) + } +} + +func TestNormalizeCPU_ToMillicores(t *testing.T) { + tests := []struct { + name string + raw any + want int64 + ok bool + }{ + {"int", 3, 3000, true}, + {"float", 1.25, 1250, true}, + {"string int", "4", 4000, true}, + {"string decimal", "2.5", 2500, true}, + {"millicores", "500m", 500, true}, + + {"zero string invalid", "0", 0, false}, + {"zero millicores invalid", "0m", 0, false}, + {"zero int invalid", 0, 0, false}, + {"negative int invalid", -1, 0, false}, + {"negative string decimal invalid", "-2.5", 0, false}, + {"negative millicores invalid", "-250m", 0, false}, + + {"spaces", " 3 ", 3000, true}, + {"leading zeros", "0003", 3000, true}, + + {"invalid", "abc", 0, false}, + {"invalid suffix", "5M", 0, false}, // only lowercase 'm' for millicores + {"nil", nil, 0, false}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got, ok := toMillicores(tc.raw) + assert.Equal(t, tc.ok, ok) + assert.Equal(t, tc.want, got) + }) + } +} diff --git a/internal/config/types.go b/internal/config/types.go index 66572b1..f6fa06d 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -73,10 +73,12 @@ type PackageConfig struct { // ResourceConfig represents resource allocation type ResourceConfig struct { - CPU any `yaml:"cpu,omitempty" validate:"omitempty,k8s_cpu"` // Can be string or int - Memory string `yaml:"memory,omitempty" validate:"omitempty,k8s_memory"` - Storage string `yaml:"storage,omitempty" validate:"omitempty,k8s_memory"` - GPU int `yaml:"gpu,omitempty" validate:"omitempty,min=0,max=8"` // Number of GPUs requested + CPURaw any `yaml:"cpu,omitempty" validate:"omitempty,k8s_cpu"` // Can be string or int + MemoryRaw any `yaml:"memory,omitempty" validate:"omitempty,k8s_memory"` + CPU int64 `yaml:"-"` // canonical + Memory int64 `yaml:"-"` // canonical + Storage string `yaml:"storage,omitempty" validate:"omitempty,k8s_memory"` + GPU int `yaml:"gpu,omitempty" validate:"omitempty,min=0,max=8"` // Number of GPUs requested } // VolumeMount represents a volume mount configuration @@ -104,10 +106,10 @@ func NewBaseConfigWithDefaults() BaseConfig { ClearVSCodeCache: false, PythonBinPath: "/opt/venv/bin", Resources: ResourceConfig{ - CPU: 2, // Default CPU - Memory: "8Gi", // Default Memory - Storage: "20Gi", // Default Storage - GPU: 0, // Default GPU + CPU: 2000, // Default CPU (2 cores) + Memory: 8 * 1024, // Default Memory (8Gi -> 8192Mi) + Storage: "20Gi", // Default Storage + GPU: 0, // Default GPU }, Packages: PackageConfig{ Python: []string{}, // Empty slice - no default packages @@ -148,31 +150,35 @@ func (c *DevEnvConfig) GetUserID() string { // GPU returns the number of GPU resources requested for the developer environment. // Returns 0 if no GPU allocation is specified in the configuration. func (c *DevEnvConfig) GPU() int { + if c.Resources.GPU < 0 { + return 0 + } return c.Resources.GPU } -// CPU returns the CPU resource allocation as a string suitable for Kubernetes manifests. -// It handles flexible input types from YAML (string, int, float64) and converts them -// to a consistent string format. +// CPU returns the canonical CPU quantity formatted for Kubernetes (e.g., "2500m"). +// Assumes Resources.CPU has already been normalized to millicores. +// Returns "0" if CPU <= 0 so callers can omit the field or treat as no request. func (c *DevEnvConfig) CPU() string { - if c.Resources.CPU == nil { - return "0" // This shouldn't happen with proper config loading - } - switch v := c.Resources.CPU.(type) { - case string: - return v - case int: - return fmt.Sprintf("%d", v) - case float64: - return fmt.Sprintf("%.0f", v) - default: + if c.Resources.CPU <= 0 { return "0" } + return fmt.Sprintf("%dm", c.Resources.CPU) } -// Memory returns the memory resource allocation as a string suitable for Kubernetes manifests. +// Memory returns the canonical memory quantity formatted for Kubernetes. +// Assumes Resources.Memory has been normalized to Mi (mebibytes). +// Returns "" if Memory <= 0 so callers can omit the field. +// Uses "Gi" when an exact Gi multiple, otherwise "Mi". func (c *DevEnvConfig) Memory() string { - return c.Resources.Memory + mi := c.Resources.Memory + if mi <= 0 { + return "" + } + if mi%1024 == 0 { + return fmt.Sprintf("%dGi", mi/1024) + } + return fmt.Sprintf("%dMi", mi) } // CPURequest returns the CPU resource request as a string suitable for Kubernetes manifests. @@ -200,7 +206,12 @@ func (c *DevEnvConfig) NodePort() int { // Returns the slice of VolumeMount configurations for binding local directories // into the developer environment container. func (c *DevEnvConfig) VolumeMounts() []VolumeMount { - return c.Volumes + if c.Volumes == nil { + return nil + } + out := make([]VolumeMount, len(c.Volumes)) + copy(out, c.Volumes) // copies the slice AND each struct element by value + return out } // GetSSHKeysSlice returns SSH keys as a string slice for use in Go templates. @@ -209,10 +220,12 @@ func (c *DevEnvConfig) VolumeMounts() []VolumeMount { // is not possible. func (c *DevEnvConfig) GetSSHKeysSlice() []string { keys, err := c.GetSSHKeys() - if err != nil { - return []string{} // Return empty slice on error + if err != nil || len(keys) == 0 { + return []string{} } - return keys + cp := make([]string, len(keys)) + copy(cp, keys) + return cp } // GetSSHKeysString returns all SSH keys as a single newline-separated string diff --git a/internal/config/types_test.go b/internal/config/types_test.go index 6d2da87..3e35180 100644 --- a/internal/config/types_test.go +++ b/internal/config/types_test.go @@ -5,33 +5,60 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) +// TestNewBaseConfigWithDefaults verifies that the factory defaults produce a +// ready-to-use config with canonical resource units and non-nil empty slices. +// This test documents the contract of NewBaseConfigWithDefaults so future changes +// (e.g., image tag, default resources, or slice initialization) don’t regress silently. func TestNewBaseConfigWithDefaults(t *testing.T) { - config := NewBaseConfigWithDefaults() - - // Test basic defaults - assert.Equal(t, "ubuntu:22.04", config.Image) - assert.Equal(t, 1000, config.UID) - assert.Equal(t, "/opt/venv/bin", config.PythonBinPath) - - // Test container setup defaults - assert.True(t, config.InstallHomebrew) - assert.False(t, config.ClearLocalPackages) - assert.False(t, config.ClearVSCodeCache) - - // Test resource defaults - assert.Equal(t, 2, config.Resources.CPU) - assert.Equal(t, "8Gi", config.Resources.Memory) - assert.Equal(t, "20Gi", config.Resources.Storage) - assert.Equal(t, 0, config.Resources.GPU) - - // Test empty slice defaults - assert.Equal(t, []string{}, config.Packages.Python) - assert.Equal(t, []string{}, config.Packages.APT) - assert.Equal(t, []VolumeMount{}, config.Volumes) + cfg := NewBaseConfigWithDefaults() + + // --- Basic defaults (scalar fields) --- + assert.Equal(t, "ubuntu:22.04", cfg.Image) + assert.Equal(t, 1000, cfg.UID) + assert.Equal(t, "/opt/venv/bin", cfg.PythonBinPath) + + // --- Container setup toggles --- + assert.True(t, cfg.InstallHomebrew) + assert.False(t, cfg.ClearLocalPackages) + assert.False(t, cfg.ClearVSCodeCache) + + // --- Resource defaults (canonical units) --- + // Canonical CPU is millicores; default should be 2 cores = 2000m. + assert.Equal(t, int64(2000), cfg.Resources.CPU) + // Canonical Memory is Mi; default should be 8 Gi = 8192 Mi. + assert.Equal(t, int64(8192), cfg.Resources.Memory) + assert.Equal(t, "20Gi", cfg.Resources.Storage) + assert.Equal(t, 0, cfg.Resources.GPU) + + // Also assert getters render the canonical values as expected (future-proofing). + assert.Equal(t, "2000m", (&DevEnvConfig{BaseConfig: cfg}).CPU()) + assert.Equal(t, "8Gi", (&DevEnvConfig{BaseConfig: cfg}).Memory()) + + // Raw fields should be empty in defaults (we set canonical values directly). + assert.Nil(t, cfg.Resources.CPURaw) + assert.Nil(t, cfg.Resources.MemoryRaw) + + // --- Empty-but-non-nil slices (ergonomic contract) --- + // Callers can append without nil checks; order is preserved. + assert.NotNil(t, cfg.Packages.Python) + assert.NotNil(t, cfg.Packages.APT) + assert.NotNil(t, cfg.Volumes) + assert.Len(t, cfg.Packages.Python, 0) + assert.Len(t, cfg.Packages.APT, 0) + assert.Len(t, cfg.Volumes, 0) + + // Appending to default-initialized slices should not panic. + assert.NotPanics(t, func() { cfg.Packages.Python = append(cfg.Packages.Python, "numpy") }) + assert.NotPanics(t, func() { cfg.Packages.APT = append(cfg.Packages.APT, "curl") }) + assert.NotPanics(t, func() { cfg.Volumes = append(cfg.Volumes, VolumeMount{Name: "work"}) }) } +// TestBaseConfig_GetSSHKeys verifies that SSH keys are normalized from flexible +// YAML shapes (string, []string, []any) into a clean []string, with trimming, +// order preserved, and clear failures for invalid/empty inputs. func TestBaseConfig_GetSSHKeys(t *testing.T) { tests := []struct { name string @@ -46,37 +73,56 @@ func TestBaseConfig_GetSSHKeys(t *testing.T) { expectError: false, }, { - name: "multiple string keys", + name: "single string trimmed", + sshKeyField: " ssh-ed25519 AAAAC3... user ", + expected: []string{"ssh-ed25519 AAAAC3... user"}, + expectError: false, + }, + { + name: "multiple string keys preserves order", sshKeyField: []string{"ssh-rsa AAAAB3... user1", "ssh-ed25519 AAAAC3... user2"}, expected: []string{"ssh-rsa AAAAB3... user1", "ssh-ed25519 AAAAC3... user2"}, expectError: false, }, { name: "interface slice from YAML", - sshKeyField: []interface{}{"ssh-rsa AAAAB3... user1", "ssh-ed25519 AAAAC3... user2"}, + sshKeyField: []any{"ssh-rsa AAAAB3... user1", "ssh-ed25519 AAAAC3... user2"}, expected: []string{"ssh-rsa AAAAB3... user1", "ssh-ed25519 AAAAC3... user2"}, expectError: false, }, { - name: "nil field", + name: "nil field yields empty slice (safe default)", sshKeyField: nil, expected: []string{}, expectError: false, }, { - name: "empty string", + name: "empty string key is invalid", sshKeyField: "", expected: []string{}, expectError: true, }, + // TODO: Return to this { - name: "empty array", + name: "empty slice is invalid", sshKeyField: []string{}, + expected: nil, // ignored when expectError=true + expectError: true, + }, + { + name: "slice containing blank entry is invalid", + sshKeyField: []string{"ssh-rsa AAAAB3... user", " "}, expected: nil, expectError: true, }, { - name: "invalid type", + name: "mixed-type slice is invalid", + sshKeyField: []any{"ssh-rsa AAAAB3... user", 42}, + expected: nil, + expectError: true, + }, + { + name: "invalid type (int) is invalid", sshKeyField: 123, expected: nil, expectError: true, @@ -85,252 +131,482 @@ func TestBaseConfig_GetSSHKeys(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - config := &BaseConfig{SSHPublicKey: tt.sshKeyField} - result, err := config.GetSSHKeys() + cfg := &BaseConfig{SSHPublicKey: tt.sshKeyField} + got, err := cfg.GetSSHKeys() if tt.expectError { assert.Error(t, err) - } else { - assert.NoError(t, err) - assert.Equal(t, tt.expected, result) + return } + assert.NoError(t, err) + assert.Equal(t, tt.expected, got) }) } } +// TestDevEnvConfig_GetUserID verifies that UID is formatted as a string with no +// implicit defaulting or validation at this layer (pure accessor behavior). func TestDevEnvConfig_GetUserID(t *testing.T) { tests := []struct { name string uid int expected string }{ - { - name: "custom UID", - uid: 2000, - expected: "2000", - }, - { - name: "zero UID should still return zero (not default)", - uid: 0, - expected: "0", - }, + {name: "custom UID", uid: 2000, expected: "2000"}, + {name: "zero UID returns '0'", uid: 0, expected: "0"}, + {name: "negative UID is formatted as-is", uid: -7, expected: "-7"}, // validation happens elsewhere } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - config := &DevEnvConfig{ - BaseConfig: BaseConfig{UID: tt.uid}, - } - result := config.GetUserID() - assert.Equal(t, tt.expected, result) + cfg := &DevEnvConfig{BaseConfig: BaseConfig{UID: tt.uid}} + assert.Equal(t, tt.expected, cfg.GetUserID()) }) } } -func TestDevEnvConfig_CPU(t *testing.T) { +// TestDevEnvConfig_CPU_Format verifies that CPU() is a pure formatter over the +// canonical millicores field (Resources.CPU). It does not parse raw inputs or +// apply defaults—those happen during normalization. +func TestDevEnvConfig_CPU_Format(t *testing.T) { tests := []struct { - name string - cpuValue any - expected string + name string + milli int64 + want string }{ - { - name: "integer CPU", - cpuValue: 4, - expected: "4", - }, - { - name: "string CPU", - cpuValue: "2.5", - expected: "2.5", - }, - { - name: "float CPU", - cpuValue: 3.5, - expected: "4", // float64 formatted as integer - }, - { - name: "nil CPU returns 0", - cpuValue: nil, - expected: "0", - }, + {name: "zero -> 0", milli: 0, want: "0"}, + {name: "positive -> Xm", milli: 2500, want: "2500m"}, + {name: "large -> Xm", milli: 123456, want: "123456m"}, + {name: "negative clamps", milli: -1, want: "0"}, } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - config := &DevEnvConfig{ + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + cfg := &DevEnvConfig{ BaseConfig: BaseConfig{ - Resources: ResourceConfig{ - CPU: tt.cpuValue, - }, + Resources: ResourceConfig{CPU: tc.milli}, }, } - result := config.CPU() - assert.Equal(t, tt.expected, result) + assert.Equal(t, tc.want, cfg.CPU()) }) } } -func TestDevEnvConfig_Memory(t *testing.T) { +// CPU() must format canonical Resources.CPU and ignore CPURaw if canonical isn't set. +func TestDevEnvConfig_CPU_IgnoresRawWithoutNormalization(t *testing.T) { + // Raw provided, canonical zero => CPU() should not look at CPURaw. + cfg := &DevEnvConfig{ + BaseConfig: BaseConfig{ + Resources: ResourceConfig{CPURaw: "2.5", CPU: 0}, + }, + } + require.Equal(t, "0", cfg.CPU()) + + // Canonical set => CPU() formats it regardless of CPURaw. + cfg2 := &DevEnvConfig{ + BaseConfig: BaseConfig{ + Resources: ResourceConfig{CPURaw: "bad", CPU: 2500}, + }, + } + require.Equal(t, "2500m", cfg2.CPU()) +} + +// TestDevEnvConfig_Memory_Format verifies that Memory() is a pure formatter over the +// canonical Mi field (Resources.Memory). It does not parse raw inputs; normalization +// must have already populated Memory in MiB. Exact Gi multiples render as "Gi", otherwise "Mi". +func TestDevEnvConfig_Memory_Format(t *testing.T) { tests := []struct { - name string - memory string - expected string + name string + memMi int64 + want string }{ - { - name: "custom memory", - memory: "16Gi", - expected: "16Gi", + {name: "zero -> empty", memMi: 0, want: ""}, + {name: "exact Gi prints Gi", memMi: 16 * 1024, want: "16Gi"}, + {name: "non-Gi prints Mi", memMi: 1536, want: "1536Mi"}, // 1.5 Gi + {name: "small Mi prints Mi", memMi: 477, want: "477Mi"}, + {name: "negative -> empty", memMi: -1, want: ""}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + cfg := &DevEnvConfig{ + BaseConfig: BaseConfig{ + Resources: ResourceConfig{Memory: tc.memMi}, + }, + } + assert.Equal(t, tc.want, cfg.Memory()) + }) + } +} + +// Memory() must format canonical Resources.Memory and ignore MemoryRaw if canonical isn't set. +func TestDevEnvConfig_Memory_IgnoresRawWithoutNormalization(t *testing.T) { + cfg := &DevEnvConfig{ + BaseConfig: BaseConfig{ + Resources: ResourceConfig{MemoryRaw: "1.5Gi", Memory: 0}, }, - { - name: "empty memory returns as-is", - memory: "", - expected: "", + } + require.Equal(t, "", cfg.Memory()) + + cfg2 := &DevEnvConfig{ + BaseConfig: BaseConfig{ + Resources: ResourceConfig{MemoryRaw: "bad", Memory: 1536}, }, } + require.Equal(t, "1536Mi", cfg2.Memory()) +} + +// TestDevEnvConfig_Memory_FormatFromCanonical verifies that Memory() is a pure +// formatter over the canonical Mi field (Resources.Memory). Exact Gi multiples +// render with "Gi"; all other positive values render with "Mi"; non-positive +// values render as an empty string. +func TestDevEnvConfig_Memory_FormatFromCanonical(t *testing.T) { + tests := []struct { + name string + memory int64 // canonical Mi + want string + }{ + {name: "zero -> empty", memory: 0, want: ""}, + {name: "negative -> empty", memory: -1, want: ""}, + {name: "exact Gi", memory: 16 * 1024, want: "16Gi"}, + {name: "non-Gi (1.5Gi)", memory: 1536, want: "1536Mi"}, + {name: "non-Gi (small Mi)", memory: 477, want: "477Mi"}, + } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - config := &DevEnvConfig{ + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + cfg := &DevEnvConfig{ BaseConfig: BaseConfig{ - Resources: ResourceConfig{ - Memory: tt.memory, - }, + Resources: ResourceConfig{Memory: tc.memory}, }, } - result := config.Memory() - assert.Equal(t, tt.expected, result) + got := cfg.Memory() + assert.Equal(t, tc.want, got) }) } } +// TestDevEnvConfig_GPU documents the contract of GPU(): it returns a non-negative +// GPU count by clamping negatives to zero, and defaults to zero when unset. func TestDevEnvConfig_GPU(t *testing.T) { - config := &DevEnvConfig{ - BaseConfig: BaseConfig{ - Resources: ResourceConfig{ - GPU: 2, - }, - }, + tests := []struct { + name string + gpu int + expected int + }{ + {name: "positive", gpu: 2, expected: 2}, + {name: "zero", gpu: 0, expected: 0}, + {name: "negative clamped to zero", gpu: -1, expected: 0}, + {name: "large value preserved", gpu: 8, expected: 8}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + cfg := &DevEnvConfig{ + BaseConfig: BaseConfig{ + Resources: ResourceConfig{GPU: tc.gpu}, + }, + } + got := cfg.GPU() + assert.Equal(t, tc.expected, got) + }) } - assert.Equal(t, 2, config.GPU()) + + t.Run("unset defaults to zero", func(t *testing.T) { + // Do not set GPU at all; rely on zero-values. + cfg := &DevEnvConfig{} // BaseConfig.Resources.GPU == 0 by default + assert.Equal(t, 0, cfg.GPU()) + }) } func TestDevEnvConfig_NodePort(t *testing.T) { - config := &DevEnvConfig{ - SSHPort: 30022, + tests := []struct { + name string + sshPort int + expected int + }{ + {name: "typical value", sshPort: 30022, expected: 30022}, + {name: "lower bound", sshPort: 30000, expected: 30000}, + {name: "upper bound", sshPort: 32767, expected: 32767}, + + // Out-of-range values still pass through here. + // Range enforcement is tested in validation (ports.go) tests. + {name: "below range", sshPort: 29999, expected: 29999}, + {name: "above range", sshPort: 32768, expected: 32768}, + + // Degenerate cases + {name: "zero", sshPort: 0, expected: 0}, + {name: "negative", sshPort: -1, expected: -1}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + cfg := &DevEnvConfig{SSHPort: tc.sshPort} + got := cfg.NodePort() + assert.Equal(t, tc.expected, got) + }) } - assert.Equal(t, 30022, config.NodePort()) } +// TestDevEnvConfig_VolumeMounts verifies that VolumeMounts() returns a stable, +// template-friendly view of the configured mounts: +// - order is preserved +// - empty vs. nil are preserved +// - it returns a defensive copy (mutations to the returned slice do not affect config) func TestDevEnvConfig_VolumeMounts(t *testing.T) { - volumes := []VolumeMount{ - {Name: "data", LocalPath: "/local/data", ContainerPath: "/data"}, - {Name: "logs", LocalPath: "/local/logs", ContainerPath: "/logs"}, + tests := []struct { + name string + vols []VolumeMount + expected []VolumeMount + }{ + { + name: "multiple entries preserve order", + vols: []VolumeMount{ + {Name: "data", LocalPath: "/local/data", ContainerPath: "/data"}, + {Name: "logs", LocalPath: "/local/logs", ContainerPath: "/logs"}, + }, + expected: []VolumeMount{ + {Name: "data", LocalPath: "/local/data", ContainerPath: "/data"}, + {Name: "logs", LocalPath: "/local/logs", ContainerPath: "/logs"}, + }, + }, + { + name: "empty slice returns empty (non-nil)", + vols: []VolumeMount{}, + expected: []VolumeMount{}, + }, + { + name: "nil slice stays nil", + vols: nil, + expected: nil, + }, + { + name: "single entry", + vols: []VolumeMount{{Name: "workspace", LocalPath: "/src", ContainerPath: "/work"}}, + expected: []VolumeMount{{Name: "workspace", LocalPath: "/src", ContainerPath: "/work"}}, + }, } - config := &DevEnvConfig{ - BaseConfig: BaseConfig{Volumes: volumes}, + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + cfg := &DevEnvConfig{BaseConfig: BaseConfig{Volumes: tc.vols}} + + // Basic equality and empty/nil shape + got := cfg.VolumeMounts() + assert.Equal(t, tc.expected, got) + + // Immutability: mutate the returned slice; original must not change. + before := cfg.BaseConfig.Volumes + if got != nil { + // append to returned slice + got = append(got, VolumeMount{Name: "tmp", LocalPath: "/tmp", ContainerPath: "/tmp"}) + // modify element content + if len(got) > 0 { + got[0].Name = "mutated" + } + } + // Config’s stored volumes must remain identical to the original input + assert.Equal(t, before, cfg.BaseConfig.Volumes) + }) } - - result := config.VolumeMounts() - assert.Equal(t, volumes, result) } +// TestDevEnvConfig_GetSSHKeysSlice verifies the template-friendly accessor: +// - It normalizes flexible shapes into []string (string, []string, []any of string). +// - On any normalization error, it returns an empty slice (never nil). +// - It returns a defensive copy: mutating the result doesn't affect subsequent calls. func TestDevEnvConfig_GetSSHKeysSlice(t *testing.T) { - t.Run("valid SSH keys", func(t *testing.T) { - config := &DevEnvConfig{ - BaseConfig: BaseConfig{ - SSHPublicKey: []string{"ssh-rsa AAAAB3... user1", "ssh-ed25519 AAAAC3... user2"}, - }, - } - result := config.GetSSHKeysSlice() - expected := []string{"ssh-rsa AAAAB3... user1", "ssh-ed25519 AAAAC3... user2"} - assert.Equal(t, expected, result) - }) + tests := []struct { + name string + input any + expected []string + }{ + { + name: "valid slice preserves order", + input: []string{"ssh-rsa AAAA user1", "ssh-ed25519 BBBB user2"}, + expected: []string{"ssh-rsa AAAA user1", "ssh-ed25519 BBBB user2"}, + }, + { + name: "single string normalizes to slice", + input: "ssh-ed25519 BBBB user", + expected: []string{"ssh-ed25519 BBBB user"}, + }, + { + name: "single string trimmed", + input: " ssh-rsa AAAA user ", + expected: []string{"ssh-rsa AAAA user"}, + }, + { + name: "interface slice (YAML) preserves order", + input: []any{"ssh-rsa AAAA user1", "ssh-ed25519 BBBB user2"}, + expected: []string{"ssh-rsa AAAA user1", "ssh-ed25519 BBBB user2"}, + }, + { + name: "nil yields empty slice", + input: nil, + expected: []string{}, + }, + { + // empty slice is an error in normalizeSSHKeys; accessor suppresses to empty + name: "empty slice yields empty slice", + input: []string{}, + expected: []string{}, + }, + { + name: "invalid type suppressed to empty", + input: 123, // wrong type + expected: []string{}, + }, + { + name: "mixed-type interface slice suppressed to empty", + input: []any{"ssh-rsa AAAA user", 42}, + expected: []string{}, + }, + } - t.Run("invalid SSH keys returns empty slice", func(t *testing.T) { - config := &DevEnvConfig{ - BaseConfig: BaseConfig{ - SSHPublicKey: 123, // Invalid type - }, - } - result := config.GetSSHKeysSlice() - assert.Equal(t, []string{}, result) - }) + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + cfg := &DevEnvConfig{BaseConfig: BaseConfig{SSHPublicKey: tc.input}} + + got1 := cfg.GetSSHKeysSlice() + assert.Equal(t, tc.expected, got1) + assert.NotNil(t, got1) // accessor always returns a slice, never nil + + // Immutability / defensive copy: mutate the returned slice, call again; result should be unchanged. + if len(got1) > 0 { + got1[0] = "MUTATED" + } + got2 := cfg.GetSSHKeysSlice() + assert.Equal(t, tc.expected, got2) // must not reflect caller mutations + }) + } } -func TestDevEnvConfig_GetSSHKeysString(t *testing.T) { - t.Run("valid SSH keys", func(t *testing.T) { - config := &DevEnvConfig{ - BaseConfig: BaseConfig{ - SSHPublicKey: []string{"ssh-rsa AAAAB3... user1", "ssh-ed25519 AAAAC3... user2"}, - }, - } - result := config.GetSSHKeysString() - expected := "ssh-rsa AAAAB3... user1\nssh-ed25519 AAAAC3... user2\n" - assert.Equal(t, expected, result) - }) +// TestDevEnvConfig_GetDeveloperDir verifies that GetDeveloperDir() is a pure accessor: +// it returns exactly what is stored (no cleaning, normalization, or validation). +func TestDevEnvConfig_GetDeveloperDir(t *testing.T) { + tests := []struct { + name string + path string + want string + }{ + {name: "absolute path", path: "/path/to/developers/alice", want: "/path/to/developers/alice"}, + {name: "relative path", path: "developers/alice", want: "developers/alice"}, + {name: "empty string", path: "", want: ""}, + {name: "trailing slash preserved", path: "/devs/alice/", want: "/devs/alice/"}, + {name: "unicode / spaces preserved", path: "/devs/álïçë projects", want: "/devs/álïçë projects"}, + + // No normalization: leading dot or parent segments are returned as-is. + {name: "dot-segment retained", path: "./devs/alice", want: "./devs/alice"}, + {name: "parent-segment retained", path: "../devs/alice", want: "../devs/alice"}, + } - t.Run("no SSH keys returns empty string", func(t *testing.T) { - config := &DevEnvConfig{ - BaseConfig: BaseConfig{ - SSHPublicKey: nil, - }, - } - result := config.GetSSHKeysString() - assert.Equal(t, "", result) + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + cfg := &DevEnvConfig{DeveloperDir: tc.path} + got := cfg.GetDeveloperDir() + assert.Equal(t, tc.want, got) + }) + } + + t.Run("unset (zero-value struct) -> empty", func(t *testing.T) { + var cfg DevEnvConfig // DeveloperDir is zero value (empty string) + assert.Equal(t, "", cfg.GetDeveloperDir()) }) +} - t.Run("invalid SSH keys returns empty string", func(t *testing.T) { - config := &DevEnvConfig{ +// TestDevEnvConfig_CPURequest_AliasOfCPU documents that CPURequest() returns +// exactly what CPU() returns for canonical millicores. This test does not +// exercise parsing/normalization; it only covers the formatting layer. +func TestDevEnvConfig_CPURequest_AliasOfCPU(t *testing.T) { + tests := []struct { + name string + milli int64 // canonical millicores + }{ + {name: "zero", milli: 0}, + {name: "positive", milli: 2500}, + {name: "large", milli: 123456}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + cfg := &DevEnvConfig{ + BaseConfig: BaseConfig{ + Resources: ResourceConfig{CPU: tc.milli}, + }, + } + gotCPU := cfg.CPU() + gotReq := cfg.CPURequest() + assert.Equal(t, gotCPU, gotReq) + }) + } + + // Guard: CPURaw alone (without normalization) has no effect on getters. + t.Run("raw value ignored without normalization", func(t *testing.T) { + cfg := &DevEnvConfig{ BaseConfig: BaseConfig{ - SSHPublicKey: 123, // Invalid type + Resources: ResourceConfig{CPURaw: "2.5", CPU: 0}, }, } - result := config.GetSSHKeysString() - assert.Equal(t, "", result) + assert.Equal(t, "0", cfg.CPU()) + assert.Equal(t, cfg.CPU(), cfg.CPURequest()) }) } -func TestDevEnvConfig_GetDeveloperDir(t *testing.T) { - config := &DevEnvConfig{ - DeveloperDir: "/path/to/developers/alice", +// TestDevEnvConfig_MemoryRequest_AliasOfMemory documents that MemoryRequest() +// returns exactly what Memory() returns for canonical mebibytes (Mi). This +// exercises the formatting layer only; parsing/normalization is tested elsewhere. +func TestDevEnvConfig_MemoryRequest_AliasOfMemory(t *testing.T) { + tests := []struct { + name string + memMi int64 // canonical Mi + }{ + {name: "zero -> empty", memMi: 0}, + {name: "exact Gi formats Gi", memMi: 16 * 1024}, // "16Gi" + {name: "non-Gi (1.5Gi) -> Mi", memMi: 1536}, // "1536Mi" + {name: "non-Gi (small Mi)", memMi: 477}, // "477Mi" + // Under strict normalization, canonical should not be negative; this is defensive. + {name: "negative -> empty (defensive)", memMi: -1}, } - result := config.GetDeveloperDir() - assert.Equal(t, "/path/to/developers/alice", result) -} -func TestDevEnvConfig_CPURequest(t *testing.T) { - config := &DevEnvConfig{ - BaseConfig: BaseConfig{ - Resources: ResourceConfig{ - CPU: "4", - }, - }, + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + cfg := &DevEnvConfig{ + BaseConfig: BaseConfig{ + Resources: ResourceConfig{Memory: tc.memMi}, + }, + } + gotMem := cfg.Memory() + gotReq := cfg.MemoryRequest() + assert.Equal(t, gotMem, gotReq) + }) } - // CPURequest should be an alias for CPU - assert.Equal(t, config.CPU(), config.CPURequest()) -} -func TestDevEnvConfig_MemoryRequest(t *testing.T) { - config := &DevEnvConfig{ - BaseConfig: BaseConfig{ - Resources: ResourceConfig{ - Memory: "16Gi", + // Guard: MemoryRaw alone (without normalization) has no effect on getters. + t.Run("raw value ignored without normalization", func(t *testing.T) { + cfg := &DevEnvConfig{ + BaseConfig: BaseConfig{ + Resources: ResourceConfig{MemoryRaw: "1.5Gi", Memory: 0}, }, - }, - } - // MemoryRequest should be an alias for Memory - assert.Equal(t, config.Memory(), config.MemoryRequest()) + } + assert.Equal(t, "", cfg.Memory()) + assert.Equal(t, cfg.Memory(), cfg.MemoryRequest()) + }) } +// TestDevEnvConfig_Embedding verifies Go struct embedding behavior for this API: +// - BaseConfig fields are promoted and readable directly on DevEnvConfig. +// - Methods defined on BaseConfig are promoted to DevEnvConfig. +// - DevEnvConfig-owned fields remain accessible. +// - The embedded BaseConfig is not copied (promoted fields reflect the same storage). func TestDevEnvConfig_Embedding(t *testing.T) { - // Test that BaseConfig fields are promoted properly - config := &DevEnvConfig{ + cfg := &DevEnvConfig{ BaseConfig: BaseConfig{ Image: "custom:latest", InstallHomebrew: false, ClearLocalPackages: true, PythonBinPath: "/custom/python/bin", + UID: 2000, }, Name: "alice", Git: GitConfig{ @@ -339,65 +615,104 @@ func TestDevEnvConfig_Embedding(t *testing.T) { }, } - // Test direct access to embedded fields - assert.Equal(t, "custom:latest", config.Image) - assert.False(t, config.InstallHomebrew) - assert.True(t, config.ClearLocalPackages) - assert.Equal(t, "/custom/python/bin", config.PythonBinPath) + // 1) Field promotion: embedded fields readable directly + assert.Equal(t, "custom:latest", cfg.Image) + assert.False(t, cfg.InstallHomebrew) + assert.True(t, cfg.ClearLocalPackages) + assert.Equal(t, "/custom/python/bin", cfg.PythonBinPath) + assert.Equal(t, 2000, cfg.UID) + + // 2) Method promotion: methods on BaseConfig are callable on DevEnvConfig + assert.Equal(t, "2000", cfg.GetUserID()) + + // 3) Owned fields: user-specific fields + assert.Equal(t, "alice", cfg.Name) + assert.Equal(t, "Alice Smith", cfg.Git.Name) + assert.Equal(t, "alice@example.com", cfg.Git.Email) - // Test user-specific fields - assert.Equal(t, "alice", config.Name) - assert.Equal(t, "Alice Smith", config.Git.Name) - assert.Equal(t, "alice@example.com", config.Git.Email) + // 4) Same storage: mutate via promoted field and check embedded struct mirrors it + cfg.Image = "changed:tag" + assert.Equal(t, "changed:tag", cfg.BaseConfig.Image) + + // Also mutate via embedded and check promoted view updates + cfg.BaseConfig.InstallHomebrew = true + assert.True(t, cfg.InstallHomebrew) } -func TestNewBaseConfigWithDefaults_AllFieldsSet(t *testing.T) { - config := NewBaseConfigWithDefaults() - - // Ensure no fields are left at zero values that shouldn't be - assert.NotEmpty(t, config.Image) - assert.NotZero(t, config.UID) - assert.NotEmpty(t, config.PythonBinPath) - assert.NotNil(t, config.Resources.CPU) - assert.NotEmpty(t, config.Resources.Memory) - assert.NotEmpty(t, config.Resources.Storage) - - // These should be initialized as empty slices, not nil - assert.NotNil(t, config.Packages.Python) - assert.NotNil(t, config.Packages.APT) - assert.NotNil(t, config.Volumes) +// Verifies NewBaseConfigWithDefaults returns canonical resource units: +// CPU in millicores (2000 = 2 cores) and Memory in Mi (8192 = 8Gi). +func TestNewBaseConfigWithDefaults_ExactValues(t *testing.T) { + cfg := NewBaseConfigWithDefaults() + + // Top-level fields + require.Equal(t, "ubuntu:22.04", cfg.Image) + require.Equal(t, 1000, cfg.UID) + require.True(t, cfg.InstallHomebrew) + require.False(t, cfg.ClearLocalPackages) + require.False(t, cfg.ClearVSCodeCache) + require.Equal(t, "/opt/venv/bin", cfg.PythonBinPath) + + // Resources (canonical) + require.Equal(t, int64(2000), cfg.Resources.CPU) // 2 cores + require.Equal(t, int64(8192), cfg.Resources.Memory) // 8Gi + require.Equal(t, "20Gi", cfg.Resources.Storage) + require.Equal(t, 0, cfg.Resources.GPU) + + // Raw fields should be empty when canonical defaults are used + require.Nil(t, cfg.Resources.CPURaw) + require.Nil(t, cfg.Resources.MemoryRaw) + + // Packages: non-nil, length 0 + require.NotNil(t, cfg.Packages.Python) + require.Len(t, cfg.Packages.Python, 0) + require.NotNil(t, cfg.Packages.APT) + require.Len(t, cfg.Packages.APT, 0) + + // Volumes: non-nil, length 0 + require.NotNil(t, cfg.Volumes) + require.Len(t, cfg.Volumes, 0) + + // Optional: also assert formatter getters (future-proof) + dev := &DevEnvConfig{BaseConfig: cfg} + require.Equal(t, "2000m", dev.CPU()) + require.Equal(t, "8Gi", dev.Memory()) } -func TestResourceConfig_FlexibleCPU(t *testing.T) { - // Test that ResourceConfig can handle different CPU types - tests := []struct { - name string - cpuValue any - valid bool - }{ - {"string CPU", "2.5", true}, - {"int CPU", 4, true}, - {"float64 CPU", 3.5, true}, - {"nil CPU", nil, true}, - {"bool CPU", true, false}, // Would be handled by CPU() method returning "0" - } +func TestNewBaseConfigWithDefaults_DeterministicAndIndependent(t *testing.T) { + a := NewBaseConfigWithDefaults() + b := NewBaseConfigWithDefaults() - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - config := &DevEnvConfig{ - BaseConfig: BaseConfig{ - Resources: ResourceConfig{ - CPU: tt.cpuValue, - }, - }, - } + // Deterministic: two fresh values are deeply equal. + require.Equal(t, a, b) - // The CPU() method should handle all these cases gracefully - result := config.CPU() - assert.IsType(t, "", result) // Should always return string - assert.NotEmpty(t, result) // Should never be empty - }) + // --- Independence for slices: modifying 'a' does not affect 'b' --- + + // Packages.Python + a.Packages.Python = append(a.Packages.Python, "numpy") + require.NotEqual(t, a.Packages.Python, b.Packages.Python) + + // Packages.APT + a.Packages.APT = append(a.Packages.APT, "curl") + require.NotEqual(t, a.Packages.APT, b.Packages.APT) + + // Volumes: append and also mutate an element to prove deep independence + a.Volumes = append(a.Volumes, VolumeMount{Name: "data", LocalPath: "/data", ContainerPath: "/mnt/data"}) + require.NotEqual(t, a.Volumes, b.Volumes) + + // Mutate an existing element if present + if len(a.Volumes) > 0 { + a.Volumes[0].Name = "mutated" + require.NotEqual(t, a.Volumes, b.Volumes) } + + // --- Scalars diverge independently as well --- + // CPU is canonical millicores; change a to 4000m (4 cores) + a.Resources.CPU = 4000 + require.NotEqual(t, a.Resources.CPU, b.Resources.CPU) + + // Memory is canonical Mi; change a to 16384Mi (16Gi) + a.Resources.Memory = 16 * 1024 + require.NotEqual(t, a.Resources.Memory, b.Resources.Memory) } // Command-line flag for updating golden files diff --git a/internal/config/validation.go b/internal/config/validation.go index e8104c8..94cca4e 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -43,56 +43,6 @@ func validateSSHKeys(fl validator.FieldLevel) bool { return true } -// normalizeSSHKeys converts the flexible SSH key field to a string slice -// Handles both single string and string array formats from YAML -func normalizeSSHKeys(sshKeyField any) ([]string, error) { - if sshKeyField == nil { - return []string{}, nil - } - - switch keys := sshKeyField.(type) { - case string: - // Single SSH key - if keys == "" { - return []string{}, fmt.Errorf("SSH key cannot be empty string") - } - return []string{keys}, nil - - case []interface{}: - // Array of SSH keys (from YAML) - var result []string - for i, key := range keys { - keyStr, ok := key.(string) - if !ok { - return nil, fmt.Errorf("SSH key at index %d is not a string", i) - } - if keyStr == "" { - return nil, fmt.Errorf("SSH key at index %d cannot be empty", i) - } - result = append(result, keyStr) - } - if len(result) == 0 { - return nil, fmt.Errorf("SSH key array cannot be empty") - } - return result, nil - - case []string: - // Direct string slice - if len(keys) == 0 { - return nil, fmt.Errorf("SSH key array cannot be empty") - } - for i, key := range keys { - if key == "" { - return nil, fmt.Errorf("SSH key at index %d cannot be empty", i) - } - } - return keys, nil - - default: - return nil, fmt.Errorf("SSH key field must be string or array of strings, got %T", sshKeyField) - } -} - func validateKubernetesCPU(fl validator.FieldLevel) bool { cpuField := fl.Field().Interface() switch cpu := cpuField.(type) { From 306151dbb8e635ce4b083845261b9045e6ba87c7 Mon Sep 17 00:00:00 2001 From: Max Gagnon <82063767+maxwellgagnon@users.noreply.github.com> Date: Thu, 9 Oct 2025 16:52:48 -0700 Subject: [PATCH 02/14] Update validation.go, add validation_test.go --- internal/config/validation.go | 204 +++++++++++++++++++----- internal/config/validation_test.go | 243 +++++++++++++++++++++++++++++ 2 files changed, 410 insertions(+), 37 deletions(-) create mode 100644 internal/config/validation_test.go diff --git a/internal/config/validation.go b/internal/config/validation.go index 94cca4e..645b946 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -9,19 +9,58 @@ import ( "github.com/go-playground/validator/v10" ) +// Package-level validator used by ValidateBaseConfig / ValidateDevEnvConfig. var validate *validator.Validate +// sshKeyRE matches common OpenSSH public key formats: +// +// - ssh-ed25519 +// - ssh-rsa +// - ecdsa-sha2-nistp256 / nistp384 / nistp521 +// - sk-ecdsa-sha2-nistp256@openssh.com (FIDO) +// +// Pattern: [optional comment] +// Base64 is matched loosely with 0–2 '=' padding to accommodate real-world keys. +var sshKeyRegex = regexp.MustCompile( + `^(?:(?:ssh-(?:ed25519|rsa))|(?:ecdsa-sha2-nistp(?:256|384|521))|(?:sk-ecdsa-sha2-nistp256@openssh\.com)) [A-Za-z0-9+/]+={0,2}(?: .+)?$`, +) + +// numberRe matches a non-negative decimal number (integer or fractional). +// Examples: "0", "2", "2.5", " 3 ". +var numberRe = regexp.MustCompile(`^\s*[0-9]+(?:\.[0-9]+)?\s*$`) + +// cpuMillicoresRe matches a non-negative decimal number with an 'm' suffix (millicores). +// Examples: "500m", "0m", " 12.5m ". +var cpuMillicoresRe = regexp.MustCompile(`^\s*[0-9]+(?:\.[0-9]+)?m\s*$`) + +// memoryRe matches Kubernetes-like memory quantities as strings, case-insensitive. +// Accepts: +// - Binary: Ki, Mi, Gi, Ti, Pi, Ei +// - Decimal SI: k, M, G, T, P, E +// - Optional unit (bare numbers allowed — your parser treats these as Gi later) +// +// Examples: "512Mi", "16Gi", "500M", "1G", "1536", " 2.5Gi ". +var memoryRe = regexp.MustCompile(`(?i)^\s*[0-9]+(?:\.[0-9]+)?(?:ki|mi|gi|ti|pi|ei|k|m|g|t|p|e)?\s*$`) + func init() { - // Opt-in to v11+ behavior + // Enable "required on structs" semantics and register custom validators. validate = validator.New(validator.WithRequiredStructEnabled()) - // Register custom validators - validate.RegisterValidation("ssh_keys", validateSSHKeys) - validate.RegisterValidation("k8s_cpu", validateKubernetesCPU) - validate.RegisterValidation("k8s_memory", validateKubernetesMemory) + if err := validate.RegisterValidation("ssh_keys", validateSSHKeys); err != nil { + panic(fmt.Errorf("register validator ssh_keys: %w", err)) + } + if err := validate.RegisterValidation("k8s_cpu", validateKubernetesCPU); err != nil { + panic(fmt.Errorf("register validator k8s_cpu: %w", err)) + } + if err := validate.RegisterValidation("k8s_memory", validateKubernetesMemory); err != nil { + panic(fmt.Errorf("register validator k8s_memory: %w", err)) + } } -// validateSSHKeys validates SSH public key format +// validateSSHKeys implements the "ssh_keys" tag. +// It normalizes the flexible field (nil | string | []string | []any of string) to []string, +// trims each entry, and validates format via sshKeyRE. It returns true iff all present +// entries are valid. Presence (≥1) is enforced separately in ValidateDevEnvConfig. func validateSSHKeys(fl validator.FieldLevel) bool { sshKeyField := fl.Field().Interface() @@ -30,10 +69,6 @@ func validateSSHKeys(fl validator.FieldLevel) bool { if err != nil { return false } - - // Validate each SSH key format - sshKeyRegex := regexp.MustCompile(`^ssh-(rsa|ed25519|ecdsa) [A-Za-z0-9+/]+=*( .+)?$`) - for _, key := range sshKeys { key = strings.TrimSpace(key) if key == "" || !sshKeyRegex.MatchString(key) { @@ -43,65 +78,144 @@ func validateSSHKeys(fl validator.FieldLevel) bool { return true } +// validateKubernetesCPU implements the "k8s_cpu" tag for *raw* CPU fields. +// Accepts: +// - Strings: "", "unlimited", plain number ("2", "2.5"), or millicores ("500m") +// - Numbers (int/uint/float): non-negative +// +// Negatives and malformed strings are rejected. +// NOTE: canonicalization (→ millicores) happens in normalizeCPU during loading. func validateKubernetesCPU(fl validator.FieldLevel) bool { cpuField := fl.Field().Interface() - switch cpu := cpuField.(type) { + switch v := cpuField.(type) { case string: - if cpu == "" || cpu == "unlimited" { - return true // Valid special values + s := strings.TrimSpace(v) + if s == "" || strings.EqualFold(s, "unlimited") { + return true } // Check if it's a valid number or decimal - if _, err := strconv.ParseFloat(cpu, 64); err != nil { - // Check for 'm' suffix (millicores) - if strings.HasSuffix(cpu, "m") { - cpuValue := strings.TrimSuffix(cpu, "m") - _, err := strconv.ParseFloat(cpuValue, 64) - return err == nil + if numberRe.MatchString(s) || cpuMillicoresRe.MatchString(s) { + // Strip optional 'm' and parse number to ensure it's a valid float ≥ 0. + if strings.HasSuffix(strings.ToLower(s), "m") { + s = strings.TrimSpace(strings.TrimSuffix(s, "m")) } - return false + f, err := strconv.ParseFloat(s, 64) + return err == nil && f >= 0 } - return true + return false + case int: - return cpu >= 0 // Non-negative integer + return v >= 0 + case int8: + return v >= 0 + case int16: + return v >= 0 + case int32: + return v >= 0 + case int64: + return v >= 0 + + case uint, uint8, uint16, uint32, uint64: + // Unsigned are non-negative by definition. + return true + + case float32: + return !float32IsNaNOrInf(v) && v >= 0 case float64: - return cpu >= 0 // Non-negative float + return !float64IsNaNOrInf(v) && v >= 0 + default: - return false // Invalid type + return false } } +// validateKubernetesMemory implements the "k8s_memory" tag for *raw* memory fields. +// Accepts: +// - Strings: "", "unlimited", or a non-negative decimal + optional unit among +// Ki/Mi/Gi/Ti/Pi/Ei (binary) or k/M/G/T/P/E (decimal SI), case-insensitive. +// Bare numbers are allowed (your parser interprets them as Gi). +// - Numbers (int/uint/float): non-negative +// +// Negatives and malformed strings are rejected. +// NOTE: canonicalization (→ MiB) happens in normalizeMemory during loading. func validateKubernetesMemory(fl validator.FieldLevel) bool { - // get lowercase of string value - memory := fl.Field().String() - if memory == "" || strings.ToLower(memory) == "unlimited" { - return true // Valid special values - } + switch v := fl.Field().Interface().(type) { + case string: + s := strings.TrimSpace(v) + if s == "" || strings.EqualFold(s, "unlimited") { + return true + } + if !memoryRe.MatchString(s) { + return false + } + // Strip known unit suffix (if any) before parsing the number. + ls := strings.ToLower(s) + for _, suf := range []string{"ki", "mi", "gi", "ti", "pi", "ei", "k", "m", "g", "t", "p", "e"} { + if strings.HasSuffix(ls, suf) { + s = s[:len(s)-len(suf)] + break + } + } + f, err := strconv.ParseFloat(strings.TrimSpace(s), 64) + return err == nil && f >= 0 + + case int: + return v >= 0 + case int8: + return v >= 0 + case int16: + return v >= 0 + case int32: + return v >= 0 + case int64: + return v >= 0 - // Kubernetes memory/straoge format: number + unit (Ki, Mi, Gi, Ti, Pi, Ei) - memoryRegex := regexp.MustCompile(`^[0-9]+(\.[0-9]+)?(Ki|Mi|Gi|Ti|Pi|Ei)?$`) - return memoryRegex.MatchString(memory) + case uint, uint8, uint16, uint32, uint64: + return true + + case float32: + return !float32IsNaNOrInf(v) && v >= 0 + case float64: + return !float64IsNaNOrInf(v) && v >= 0 + + default: + return false + } } -// ValidateDevEnvConfig validates a DevEnvConfig using the validator +// ValidateDevEnvConfig runs tag-based validation and then applies +// additional semantic checks that are easier to express in code. func ValidateDevEnvConfig(config *DevEnvConfig) error { if err := validate.Struct(config); err != nil { return formatValidationError(err) } - // Additional validation for BaseConfig fields that needs special handling + // Require ≥1 SSH public key with valid format. sshKeys, err := config.GetSSHKeys() if err != nil { - return fmt.Errorf("failed to get SSH keys: %w", err) + return fmt.Errorf("invalid SSH public key(s): %w", err) } if len(sshKeys) == 0 { return fmt.Errorf("at least one SSH public key is required") } + // Canonical resources (already normalized) must be non-negative. + if config.Resources.CPU < 0 { + return fmt.Errorf("cpu must be >= 0") + } + if config.Resources.Memory < 0 { + return fmt.Errorf("memory must be >= 0") + } + if config.Resources.GPU < 0 { + return fmt.Errorf("gpu must be >= 0") + } + return nil } -// ValidateBaseConfig validates a BaseConfig using the validator +// ValidateBaseConfig validates only the BaseConfig portion; useful for +// validating global defaults or partial configs before embedding. func ValidateBaseConfig(config *BaseConfig) error { if err := validate.Struct(config); err != nil { return formatValidationError(err) @@ -109,7 +223,7 @@ func ValidateBaseConfig(config *BaseConfig) error { return nil } -// formatValidationError converts validator errors to user-friendly messages +// formatValidationError renders go-playground/validator errors as concise, user-facing text. func formatValidationError(err error) error { var errorMessages []string @@ -141,13 +255,29 @@ func formatFieldError(fieldError validator.FieldError) string { return fmt.Sprintf("'%s' must be at most %s characters/value, got '%v'", fieldName, param, value) case "hostname": return fmt.Sprintf("'%s' must be a valid hostname format, got '%v'", fieldName, value) + case "url": + return fmt.Sprintf("'%s' must be a valid URL, got '%v'", fieldName, value) + case "filepath": + return fmt.Sprintf("'%s' must be a valid file path, got '%v'", fieldName, value) + case "cron": + return fmt.Sprintf("'%s' must be a valid cron expression, got '%v'", fieldName, value) + case "ssh_keys": return fmt.Sprintf("'%s' contains invalid SSH key format", fieldName) case "k8s_cpu": return fmt.Sprintf("'%s' must be a valid Kubernetes CPU format (e.g., '2', '1.5', '500m'), got '%v'", fieldName, value) case "k8s_memory": return fmt.Sprintf("'%s' must be a valid Kubernetes memory format (e.g., '1Gi', '512Mi'), got '%v'", fieldName, value) + default: return fmt.Sprintf("'%s' failed validation '%s', got '%v'", fieldName, tag, value) } } + +// Small float guards for validator predicates (avoid NaN/Inf). +func float32IsNaNOrInf(f float32) bool { + return f != f || f > 1e38 || f < -1e38 +} +func float64IsNaNOrInf(f float64) bool { + return f != f || f > 1e308 || f < -1e308 +} diff --git a/internal/config/validation_test.go b/internal/config/validation_test.go new file mode 100644 index 0000000..489e903 --- /dev/null +++ b/internal/config/validation_test.go @@ -0,0 +1,243 @@ +package config + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// +// --- ssh_keys predicate ------------------------------------------------------ +// + +func TestValidator_SSHKeys(t *testing.T) { + type S struct { + Keys any `validate:"ssh_keys"` + } + cases := []struct { + name string + val any + ok bool + }{ + // Accept: single string + {"single string", "ssh-ed25519 AAAAB3NzaC1lZDI1NTE5AAAA test@host", true}, + // Accept: []string + {"slice of strings", []string{ + "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQ test1@h", + "ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTY test2@h", + }, true}, + // Accept: []any strings (YAML pattern) + {"interface slice of strings", []any{ + "ssh-ed25519 AAAAB3NzaC1lZDI1NTE5AAAA test@h", + "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQ test2@h", + }, true}, + + // Reject: empty string + {"empty string", "", false}, + // Reject: empty slice + {"empty slice", []string{}, false}, + // Reject: mixed types + {"mixed slice", []any{"ssh-ed25519 AAAA u@h", 42}, false}, + // Reject: malformed key + {"malformed", "ssh-ed25519 NOT_BASE64", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := validate.Struct(&S{Keys: tc.val}) + if tc.ok { + require.NoError(t, err) + } else { + require.Error(t, err) + } + }) + } +} + +// +// --- k8s_cpu predicate (raw shape) ------------------------------------------ +// + +func TestValidator_K8SCPU(t *testing.T) { + type S struct { + CPU any `validate:"k8s_cpu"` + } + cases := []struct { + name string + val any + ok bool + }{ + // Accept strings + {"plain int string", "2", true}, + {"decimal string", "2.5", true}, + {"millicores string", "500m", true}, + {"trimmed", " 3.0 ", true}, + {"empty string", "", true}, // policy: empty allowed; caller decides default + {"unlimited", "unlimited", true}, + + // Consider disallowing these? + // Accept numerics + {"int", 3, true}, + {"float64", 1.25, true}, + {"uint", uint(4), true}, + + // Reject negatives / junk + {"negative string", "-1", false}, + {"negative int", -2, false}, + {"junk", "abc", false}, + {"bad millicores", "12xm", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := validate.Struct(&S{CPU: tc.val}) + if tc.ok { + require.NoError(t, err) + } else { + require.Error(t, err) + } + }) + } +} + +// +// --- k8s_memory predicate (raw shape) --------------------------------------- +// + +func TestValidator_K8SMemory(t *testing.T) { + type S struct { + Mem any `validate:"k8s_memory"` + } + cases := []struct { + name string + val any + ok bool + }{ + // Accept strings (binary units, decimal SI, bare) + {"Mi", "512Mi", true}, + {"Gi", "16Gi", true}, + {"Ki", "1024Ki", true}, + {"decimal SI M", "500M", true}, + {"decimal SI G", "1G", true}, + {"bare", "1536", true}, + {"trimmed", " 2.5Gi ", true}, + {"empty string", "", true}, + {"unlimited", "unlimited", true}, + + // Consider disallowing these? + // Accept numerics (treated as Gi by normalizer later) + {"int", 2, true}, + {"float64", 1.5, true}, + + // Reject negatives / junk / unknown unit + {"negative string", "-1Gi", false}, + {"junk", "abc", false}, + {"unknown unit", "12GB", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := validate.Struct(&S{Mem: tc.val}) + if tc.ok { + require.NoError(t, err) + } else { + require.Error(t, err) + } + }) + } +} + +// +// --- ValidateDevEnvConfig (post-normalization semantics) -------------------- +// + +func TestValidateDevEnvConfig_SSHRequirement(t *testing.T) { + // Missing keys -> error + cfgMissing := &DevEnvConfig{ + Name: "alice", + BaseConfig: BaseConfig{ + SSHPublicKey: nil, + }, + } + err := ValidateDevEnvConfig(cfgMissing) + require.Error(t, err) + lower := strings.ToLower(err.Error()) + assert.Contains(t, lower, "ssh") + assert.Contains(t, lower, "required") + + // Present and valid -> ok + cfgOK := &DevEnvConfig{ + Name: "alice", + BaseConfig: BaseConfig{ + SSHPublicKey: "ssh-ed25519 AAAAB3NzaC1lZDI1NTE5AAAA user@host", + }, + } + require.NoError(t, ValidateDevEnvConfig(cfgOK)) +} + +func TestValidateDevEnvConfig_ResourcesNonNegative(t *testing.T) { + // CPU negative + cfgCPU := &DevEnvConfig{ + Name: "alice", + BaseConfig: BaseConfig{ + Resources: ResourceConfig{ + CPU: -1, // canonical millicores (already normalized) + }, + SSHPublicKey: "ssh-ed25519 AAAAB3NzaC1lZDI1NTE5AAAA user@h", + }, + } + err := ValidateDevEnvConfig(cfgCPU) + require.Error(t, err) + assert.Contains(t, strings.ToLower(err.Error()), "cpu") + + // Memory negative + cfgMem := &DevEnvConfig{ + Name: "alice", + BaseConfig: BaseConfig{ + Resources: ResourceConfig{ + Memory: -1, // canonical MiB (already normalized) + }, + SSHPublicKey: "ssh-ed25519 AAAAB3NzaC1lZDI1NTE5AAAA user@h", + }, + } + err = ValidateDevEnvConfig(cfgMem) + require.Error(t, err) + assert.Contains(t, strings.ToLower(err.Error()), "memory") + + // GPU negative + cfgGPU := &DevEnvConfig{ + Name: "alice", + BaseConfig: BaseConfig{ + Resources: ResourceConfig{ + GPU: -1, + }, + SSHPublicKey: "ssh-ed25519 AAAAB3NzaC1lZDI1NTE5AAAA user@h", + }, + } + err = ValidateDevEnvConfig(cfgGPU) + require.Error(t, err) + assert.Contains(t, strings.ToLower(err.Error()), "gpu") + + // All non-negative -> ok + cfgOK := &DevEnvConfig{ + Name: "alice", + BaseConfig: BaseConfig{ + Resources: ResourceConfig{ + CPU: 2500, + Memory: 16 * 1024, + GPU: 0, + }, + SSHPublicKey: "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQ user@h", + }, + } + require.NoError(t, ValidateDevEnvConfig(cfgOK)) +} + +// +// --- ValidateBaseConfig: no tag failures by default ------------------------- +// + +func TestValidateBaseConfig_Smoke(t *testing.T) { + // With no validation tags on BaseConfig itself, this should succeed. + var bc BaseConfig + require.NoError(t, ValidateBaseConfig(&bc)) +} From 03bb15e38b21f2ad907508da5430d2ac1ce1ace9 Mon Sep 17 00:00:00 2001 From: Max Gagnon <82063767+maxwellgagnon@users.noreply.github.com> Date: Fri, 10 Oct 2025 15:31:28 -0700 Subject: [PATCH 03/14] Update statefulset.yaml and generate.go to reflect cpu and memory formatting --- cmd/devenv/generate.go | 21 +++++++++++++++---- .../testdata/golden/statefulset.yaml | 4 ++-- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/cmd/devenv/generate.go b/cmd/devenv/generate.go index 7bef16c..28411ac 100644 --- a/cmd/devenv/generate.go +++ b/cmd/devenv/generate.go @@ -309,11 +309,24 @@ func printConfigSummary(cfg *config.DevEnvConfig) { fmt.Printf(" Git: %s <%s>\n", cfg.Git.Name, cfg.Git.Email) } - if cfg.Resources.CPU != nil || cfg.Resources.Memory != "" { - cpuStr := formatCPU(cfg.Resources.CPU) - fmt.Printf(" Resources: CPU=%s, Memory=%s, GPU=%d\n", - cpuStr, cfg.Resources.Memory, cfg.Resources.GPU) + cpuStr := cfg.CPU() // e.g., "4000m" or "0" + memStr := cfg.Memory() // e.g., "16Gi" or "" + hasCPU := cpuStr != "0" + hasMem := memStr != "" + + if hasCPU || hasMem { + fmt.Printf(" Resources:") + if hasCPU { + fmt.Printf(" CPU=%s", cpuStr) + } + if hasMem { + if hasCPU { + fmt.Printf(",") + } + fmt.Printf(" Memory=%s", memStr) + } + fmt.Println() } if len(cfg.Volumes) > 0 { diff --git a/internal/templates/testdata/golden/statefulset.yaml b/internal/templates/testdata/golden/statefulset.yaml index ed8bcbf..e2ec0a7 100644 --- a/internal/templates/testdata/golden/statefulset.yaml +++ b/internal/templates/testdata/golden/statefulset.yaml @@ -66,11 +66,11 @@ spec: resources: limits: nvidia.com/gpu: "2" - cpu: "4" + cpu: "4000m" memory: "16Gi" requests: nvidia.com/gpu: 2 - cpu: "4" + cpu: "4000m" memory: "16Gi" volumeMounts: From 69c3b327bd03198fd03f120d9b5ee00a2d649a9c Mon Sep 17 00:00:00 2001 From: Max Gagnon <82063767+maxwellgagnon@users.noreply.github.com> Date: Fri, 10 Oct 2025 15:32:00 -0700 Subject: [PATCH 04/14] Update additional file for cpu/mem formatting --- internal/templates/renderer_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/templates/renderer_test.go b/internal/templates/renderer_test.go index bcfe4c6..ef3df17 100644 --- a/internal/templates/renderer_test.go +++ b/internal/templates/renderer_test.go @@ -31,8 +31,8 @@ func TestRenderTemplate(t *testing.T) { APT: []string{"vim", "curl"}, }, Resources: config.ResourceConfig{ - CPU: "4", - Memory: "16Gi", + CPU: 4000, + Memory: 16 * 1024, Storage: "100Gi", GPU: 2, }, From 74c7f162edf5ad3464688a98903c955a6e0a90b7 Mon Sep 17 00:00:00 2001 From: Max Gagnon <82063767+maxwellgagnon@users.noreply.github.com> Date: Mon, 20 Oct 2025 12:08:40 -0700 Subject: [PATCH 05/14] Improve error message descriptions --- internal/config/parser.go | 36 ++++++++++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 8 deletions(-) diff --git a/internal/config/parser.go b/internal/config/parser.go index c048039..f30aac0 100644 --- a/internal/config/parser.go +++ b/internal/config/parser.go @@ -36,12 +36,20 @@ func LoadGlobalConfig(configDir string) (*BaseConfig, error) { return nil, fmt.Errorf("failed to parse YAML in global config %s: %w", globalConfigPath, err) } + // Normalize CPU (raw may be string/int/float/nil) if err := normalizeCPU(&globalConfig.Resources); err != nil { - return nil, fmt.Errorf("normalize cpu: %w", err) + return nil, fmt.Errorf( + "failed to normalize CPU in %q (cpu=%v): %w", + globalConfigPath, globalConfig.Resources.CPURaw, err, + ) } + // Normalize Memory (raw may be string/int/float/nil) if err := normalizeMemory(&globalConfig.Resources); err != nil { - return nil, fmt.Errorf("normalize memory: %w", err) + return nil, fmt.Errorf( + "failed to normalize memory in %q (memory=%v): %w", + globalConfigPath, globalConfig.Resources.MemoryRaw, err, + ) } return &globalConfig, nil } @@ -78,12 +86,18 @@ func LoadDeveloperConfig(configDir, developerName string) (*DevEnvConfig, error) config.DeveloperDir = developerDir + // Normalize flexible → canonical if err := normalizeCPU(&config.Resources); err != nil { - return nil, fmt.Errorf("normalize cpu: %w", err) + return nil, fmt.Errorf( + "failed to normalize CPU in %q (cpu=%v): %w", + configPath, config.Resources.CPURaw, err, + ) } - if err := normalizeMemory(&config.Resources); err != nil { - return nil, fmt.Errorf("normalize memory: %w", err) + return nil, fmt.Errorf( + "failed to normalize memory in %q (memory=%v): %w", + configPath, config.Resources.MemoryRaw, err, + ) } // Basic validation @@ -132,12 +146,18 @@ func LoadDeveloperConfigWithBaseConfig(configDir, developerName string, baseConf userConfig.DeveloperDir = developerDir // Step 7: Normalize flexible/raw fields → canonical representation + // Normalize flexible → canonical if err := normalizeCPU(&userConfig.Resources); err != nil { - return nil, fmt.Errorf("normalize cpu: %w", err) + return nil, fmt.Errorf( + "failed to normalize CPU in %q (cpu=%v): %w", + configPath, userConfig.Resources.CPURaw, err, + ) } - if err := normalizeMemory(&userConfig.Resources); err != nil { - return nil, fmt.Errorf("normalize memory: %w", err) + return nil, fmt.Errorf( + "failed to normalize memory in %q (memory=%v): %w", + configPath, userConfig.Resources.MemoryRaw, err, + ) } if err := userConfig.Validate(); err != nil { From cda410d35cbdd919f121772bf7c8c10950c6ea09 Mon Sep 17 00:00:00 2001 From: Max Gagnon <82063767+maxwellgagnon@users.noreply.github.com> Date: Mon, 20 Oct 2025 12:31:58 -0700 Subject: [PATCH 06/14] Restore previously removed tests; fix error type --- internal/config/example_test.go | 79 ++++++++++++++++++++++++++++++++- 1 file changed, 78 insertions(+), 1 deletion(-) diff --git a/internal/config/example_test.go b/internal/config/example_test.go index 96ec178..98cc65e 100644 --- a/internal/config/example_test.go +++ b/internal/config/example_test.go @@ -25,7 +25,7 @@ func ExampleLoadDeveloperConfig() { // Access SSH keys sshKeys, err := cfg.GetSSHKeys() if err != nil { - fmt.Println("error:", err) + log.Fatal(err) return } fmt.Printf("SSH Keys: %d configured\n", len(sshKeys)) @@ -37,3 +37,80 @@ func ExampleLoadDeveloperConfig() { // User ID: 2000 // SSH Keys: 1 configured } + +// ExampleDevEnvConfig_CPU demonstrates flexible CPU resource handling +// with different input types and default values. +func ExampleDevEnvConfig_CPU() { + // Example with string CPU value + cfg1 := &config.DevEnvConfig{ + Name: "alice", + BaseConfig: config.BaseConfig{ + Resources: config.ResourceConfig{ + CPU: 8000, // String value + }, + }, + } + fmt.Printf("String CPU: %s\n", cfg1.CPU()) + + // Example with integer CPU value + cfg2 := &config.DevEnvConfig{ + Name: "bob", + BaseConfig: config.BaseConfig{ + Resources: config.ResourceConfig{ + CPU: 4, // Integer value + }, + }, + } + fmt.Printf("Integer CPU: %s\n", cfg2.CPU()) + + // Example with no CPU specified (uses default) + cfg3 := &config.DevEnvConfig{ + Name: "charlie", + // No Resources specified + } + fmt.Printf("Default CPU: %s\n", cfg3.CPU()) + + // Output: + // String CPU: 8000m + // Integer CPU: 4m + // Default CPU: 0 +} + +// ExampleDevEnvConfig_GetSSHKeys demonstrates SSH key handling with +// both single string and multiple string array formats. +func ExampleDevEnvConfig_GetSSHKeys() { + // Single SSH key as string + cfg1 := &config.DevEnvConfig{ + Name: "alice", + BaseConfig: config.BaseConfig{ + SSHPublicKey: "ssh-rsa AAAAB3NzaC1yc2EAAAADAQAB alice@example.com", + }, + } + + keys1, err := cfg1.GetSSHKeys() + if err != nil { + log.Fatal(err) + } + fmt.Printf("Single key: %d SSH key(s)\n", len(keys1)) + + // Multiple SSH keys as slice + cfg2 := &config.DevEnvConfig{ + Name: "bob", + BaseConfig: config.BaseConfig{ + SSHPublicKey: []interface{}{ + "ssh-rsa AAAAB3NzaC1yc2EAAAADAQAB bob@work.com", + "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAI bob@home.com", + }, + }, + } + + keys2, err := cfg2.GetSSHKeys() + if err != nil { + log.Fatal(err) + } + fmt.Printf("Multiple keys: %d SSH key(s)\n", len(keys2)) + + // Output: + // Single key: 1 SSH key(s) + // Multiple keys: 2 SSH key(s) +} From 61ceece6fe6e8b42eea0518356b1b7de83bf5718 Mon Sep 17 00:00:00 2001 From: Max Gagnon <82063767+maxwellgagnon@users.noreply.github.com> Date: Mon, 20 Oct 2025 17:39:33 -0700 Subject: [PATCH 07/14] normalize resources on demand; address pr comments; add resources.go --- cmd/devenv/generate.go | 12 +- internal/config/example_test.go | 4 +- internal/config/parser.go | 456 ---------------------------- internal/config/parser_test.go | 115 +------ internal/config/resources.go | 294 ++++++++++++++++++ internal/config/resources_test.go | 313 +++++++++++++++++++ internal/config/types.go | 38 ++- internal/config/types_test.go | 260 +++++++--------- internal/config/validation.go | 48 +-- internal/config/validation_test.go | 1 - internal/templates/renderer_test.go | 4 +- 11 files changed, 771 insertions(+), 774 deletions(-) create mode 100644 internal/config/resources.go create mode 100644 internal/config/resources_test.go diff --git a/cmd/devenv/generate.go b/cmd/devenv/generate.go index 28411ac..2324141 100644 --- a/cmd/devenv/generate.go +++ b/cmd/devenv/generate.go @@ -4,6 +4,7 @@ import ( "fmt" "os" "path/filepath" + "strings" "time" "github.com/nauticalab/devenv-engine/internal/config" @@ -316,17 +317,14 @@ func printConfigSummary(cfg *config.DevEnvConfig) { hasMem := memStr != "" if hasCPU || hasMem { - fmt.Printf(" Resources:") + var parts []string if hasCPU { - fmt.Printf(" CPU=%s", cpuStr) + parts = append(parts, fmt.Sprintf("CPU=%s", cpuStr)) } if hasMem { - if hasCPU { - fmt.Printf(",") - } - fmt.Printf(" Memory=%s", memStr) + parts = append(parts, fmt.Sprintf("Memory=%s", memStr)) } - fmt.Println() + fmt.Printf(" Resources: %s\n", strings.Join(parts, ", ")) } if len(cfg.Volumes) > 0 { diff --git a/internal/config/example_test.go b/internal/config/example_test.go index 98cc65e..68501bd 100644 --- a/internal/config/example_test.go +++ b/internal/config/example_test.go @@ -46,7 +46,7 @@ func ExampleDevEnvConfig_CPU() { Name: "alice", BaseConfig: config.BaseConfig{ Resources: config.ResourceConfig{ - CPU: 8000, // String value + CPU: 8, // String value }, }, } @@ -72,7 +72,7 @@ func ExampleDevEnvConfig_CPU() { // Output: // String CPU: 8000m - // Integer CPU: 4m + // Integer CPU: 4000m // Default CPU: 0 } diff --git a/internal/config/parser.go b/internal/config/parser.go index f30aac0..26888b5 100644 --- a/internal/config/parser.go +++ b/internal/config/parser.go @@ -2,10 +2,8 @@ package config import ( "fmt" - "math" "os" "path/filepath" - "strconv" "strings" "gopkg.in/yaml.v3" @@ -36,21 +34,6 @@ func LoadGlobalConfig(configDir string) (*BaseConfig, error) { return nil, fmt.Errorf("failed to parse YAML in global config %s: %w", globalConfigPath, err) } - // Normalize CPU (raw may be string/int/float/nil) - if err := normalizeCPU(&globalConfig.Resources); err != nil { - return nil, fmt.Errorf( - "failed to normalize CPU in %q (cpu=%v): %w", - globalConfigPath, globalConfig.Resources.CPURaw, err, - ) - } - - // Normalize Memory (raw may be string/int/float/nil) - if err := normalizeMemory(&globalConfig.Resources); err != nil { - return nil, fmt.Errorf( - "failed to normalize memory in %q (memory=%v): %w", - globalConfigPath, globalConfig.Resources.MemoryRaw, err, - ) - } return &globalConfig, nil } @@ -86,20 +69,6 @@ func LoadDeveloperConfig(configDir, developerName string) (*DevEnvConfig, error) config.DeveloperDir = developerDir - // Normalize flexible → canonical - if err := normalizeCPU(&config.Resources); err != nil { - return nil, fmt.Errorf( - "failed to normalize CPU in %q (cpu=%v): %w", - configPath, config.Resources.CPURaw, err, - ) - } - if err := normalizeMemory(&config.Resources); err != nil { - return nil, fmt.Errorf( - "failed to normalize memory in %q (memory=%v): %w", - configPath, config.Resources.MemoryRaw, err, - ) - } - // Basic validation if err := config.Validate(); err != nil { return nil, fmt.Errorf("invalid configuration in %s: %w", configPath, err) @@ -146,20 +115,6 @@ func LoadDeveloperConfigWithBaseConfig(configDir, developerName string, baseConf userConfig.DeveloperDir = developerDir // Step 7: Normalize flexible/raw fields → canonical representation - // Normalize flexible → canonical - if err := normalizeCPU(&userConfig.Resources); err != nil { - return nil, fmt.Errorf( - "failed to normalize CPU in %q (cpu=%v): %w", - configPath, userConfig.Resources.CPURaw, err, - ) - } - if err := normalizeMemory(&userConfig.Resources); err != nil { - return nil, fmt.Errorf( - "failed to normalize memory in %q (memory=%v): %w", - configPath, userConfig.Resources.MemoryRaw, err, - ) - } - if err := userConfig.Validate(); err != nil { return nil, fmt.Errorf("invalid configuration in %s: %w", configPath, err) } @@ -318,414 +273,3 @@ func normalizeSSHKeys(sshKeyField any) ([]string, error) { return nil, fmt.Errorf("SSH key field must be string or array of strings, got %T", sshKeyField) } } - -// normalizeCPU reads r.CPURaw and sets r.CPU (millicores). -// With strict policy, invalid or non-positive inputs propagate as zero with an error, or -// you can choose to return ok=false and let the loader decide. Here I return an error. -func normalizeCPU(r *ResourceConfig) error { - if r.CPURaw == nil { - return nil // no user value; keep default already in r.CPU - } - m, ok := toMillicores(r.CPURaw) - if !ok { - // make policy explicit: treat invalid as an error - return fmt.Errorf("invalid cpu value: %v", r.CPURaw) - } - r.CPU = m - return nil -} - -// toMillicores converts a flexible CPU value to millicores. -// Policy: syntactically valid but non-positive (<=0) values are INVALID → ok=false. -func toMillicores(v any) (int64, bool) { - switch x := v.(type) { - case nil: - return 0, false - - case string: - s := strings.TrimSpace(x) - if s == "" { - return 0, false - } - // Already in millicores: "500m" - if strings.HasSuffix(s, "m") { - d := strings.TrimSpace(strings.TrimSuffix(s, "m")) - if !allDigits(d) { - return 0, false - } - n, err := strconv.ParseInt(d, 10, 64) - if err != nil || n <= 0 { - return 0, false - } - return n, true - } - // Plain number: "2", "2.5" - f, err := strconv.ParseFloat(s, 64) - if err != nil || f <= 0 { - return 0, false - } - return int64(math.Round(f * 1000.0)), true - - // Signed ints: cores - case int: - if x <= 0 { - return 0, false - } - return int64(x) * 1000, true - case int8: - if x <= 0 { - return 0, false - } - return int64(x) * 1000, true - case int16: - if x <= 0 { - return 0, false - } - return int64(x) * 1000, true - case int32: - if x <= 0 { - return 0, false - } - return int64(x) * 1000, true - case int64: - if x <= 0 { - return 0, false - } - return x * 1000, true - - // Unsigned ints: cores (cannot be negative) - case uint: - if x == 0 { - return 0, false - } - return int64(x) * 1000, true - case uint8: - if x == 0 { - return 0, false - } - return int64(x) * 1000, true - case uint16: - if x == 0 { - return 0, false - } - return int64(x) * 1000, true - case uint32: - if x == 0 { - return 0, false - } - return int64(x) * 1000, true - case uint64: - if x == 0 || x > math.MaxInt64/1000 { - return 0, false - } - return int64(x) * 1000, true - - // Floats (cores) - case float32: - f := float64(x) - if f <= 0 || math.IsNaN(f) || math.IsInf(f, 0) { - return 0, false - } - return int64(math.Round(f * 1000.0)), true - case float64: - if x <= 0 || math.IsNaN(x) || math.IsInf(x, 0) { - return 0, false - } - return int64(math.Round(x * 1000.0)), true - - default: - return 0, false - } -} - -func allDigits(s string) bool { - if s == "" { - return false - } - for _, r := range s { - if r < '0' || r > '9' { - return false - } - } - return true -} - -// normalizeMemory reads r.MemoryRaw (e.g., "16Gi", "512Mi", "500M", "1.5", 2, …) -// and sets r.Memory to Mi (mebibytes). Invalid or non-positive → 0. -func normalizeMemory(r *ResourceConfig) error { - if r.MemoryRaw == nil { - return nil // no user value; keep default already in r.Memory - } - mi, ok := toMi(r.MemoryRaw) - if !ok || mi <= 0 { - r.Memory = 0 - return nil - } - r.Memory = mi - return nil -} - -// toMi converts a flexible memory value into canonical Mi (mebibytes). -// Strict policy: syntactically valid but non-positive (<= 0) values are INVALID (ok=false). -func toMi(v any) (int64, bool) { - switch x := v.(type) { - case nil: - return 0, false - - case string: - s := strings.TrimSpace(x) - if s == "" { - return 0, false - } - return parseStringToMi(s) - - // Signed ints: interpret as Gi - case int: - if x <= 0 { - return 0, false - } - if int64(x) > math.MaxInt64/1024 { - return 0, false - } - return int64(x) * 1024, true - case int8: - if x <= 0 { - return 0, false - } - return int64(x) * 1024, true - case int16: - if x <= 0 { - return 0, false - } - return int64(x) * 1024, true - case int32: - if x <= 0 { - return 0, false - } - return int64(x) * 1024, true - case int64: - if x <= 0 || x > math.MaxInt64/1024 { - return 0, false - } - return x * 1024, true - - // Unsigned ints: interpret as Gi (guard overflow and zero) - case uint: - if x == 0 || uint64(x) > uint64(math.MaxInt64/1024) { - return 0, false - } - return int64(x) * 1024, true - case uint8: - if x == 0 { - return 0, false - } - return int64(x) * 1024, true - case uint16: - if x == 0 { - return 0, false - } - return int64(x) * 1024, true - case uint32: - if x == 0 { - return 0, false - } - return int64(x) * 1024, true - case uint64: - if x == 0 || x > uint64(math.MaxInt64/1024) { - return 0, false - } - return int64(x) * 1024, true - - // Floats: interpret as Gi - case float32: - return roundGiToMi(float64(x)) - case float64: - return roundGiToMi(x) - - default: - return 0, false - } -} - -// roundGiToMi converts Gi (float) to Mi with rounding. -// Returns (0,false) if gi <= 0, NaN/Inf, or rounding yields 0 Mi. -func roundGiToMi(gi float64) (int64, bool) { - if gi <= 0 || math.IsNaN(gi) || math.IsInf(gi, 0) { - return 0, false - } - miFloat := gi * 1024.0 - mi := int64(math.Round(miFloat)) - if mi <= 0 { - return 0, false - } - return mi, true -} - -// parseStringToMi handles binary units (Ki/Mi/Gi/Ti/Pi/Ei), -// decimal SI (k/M/G/T/P/E), and bare numeric strings (Gi). -// Returns (0,false) if value <= 0 or on invalid input. -func parseStringToMi(s string) (int64, bool) { - // ---- Binary SI (powers of 1024) ---- - if hasSuffixFold(s, "Ki") { - n, ok := parseNumber(strings.TrimSpace(s[:len(s)-2])) - if !ok || n <= 0 { - return 0, false - } - // KiB → MiB - miFloat := n / 1024.0 - mi := int64(math.Round(miFloat)) - if mi <= 0 { - return 0, false - } - return mi, true - } - if hasSuffixFold(s, "Mi") { - n, ok := parseNumber(strings.TrimSpace(s[:len(s)-2])) - if !ok || n <= 0 { - return 0, false - } - mi := int64(math.Round(n)) - if mi <= 0 { - return 0, false - } - return mi, true - } - if hasSuffixFold(s, "Gi") { - n, ok := parseNumber(strings.TrimSpace(s[:len(s)-2])) - if !ok || n <= 0 { - return 0, false - } - return roundGiToMi(n) - } - if hasSuffixFold(s, "Ti") { - n, ok := parseNumber(strings.TrimSpace(s[:len(s)-2])) - if !ok || n <= 0 { - return 0, false - } - miFloat := n * 1024.0 * 1024.0 - if miFloat > float64(math.MaxInt64) { - return 0, false - } - mi := int64(math.Round(miFloat)) - if mi <= 0 { - return 0, false - } - return mi, true - } - if hasSuffixFold(s, "Pi") { - n, ok := parseNumber(strings.TrimSpace(s[:len(s)-2])) - if !ok || n <= 0 { - return 0, false - } - miFloat := n * 1024.0 * 1024.0 * 1024.0 - if miFloat > float64(math.MaxInt64) { - return 0, false - } - mi := int64(math.Round(miFloat)) - if mi <= 0 { - return 0, false - } - return mi, true - } - if hasSuffixFold(s, "Ei") { - n, ok := parseNumber(strings.TrimSpace(s[:len(s)-2])) - if !ok || n <= 0 { - return 0, false - } - // Ei → Mi (giant); guard overflow - miFloat := n * 1024.0 * 1024.0 * 1024.0 * 1024.0 - if miFloat > float64(math.MaxInt64) { - return 0, false - } - mi := int64(math.Round(miFloat)) - if mi <= 0 { - return 0, false - } - return mi, true - } - - // ---- Decimal SI (powers of 1000) ---- - if hasSuffixFold(s, "k") { - n, ok := parseNumber(strings.TrimSpace(s[:len(s)-1])) - if !ok || n <= 0 { - return 0, false - } - return bytesToMi(n * 1e3) - } - if hasSuffixFold(s, "M") { - n, ok := parseNumber(strings.TrimSpace(s[:len(s)-1])) - if !ok || n <= 0 { - return 0, false - } - return bytesToMi(n * 1e6) - } - if hasSuffixFold(s, "G") { - n, ok := parseNumber(strings.TrimSpace(s[:len(s)-1])) - if !ok || n <= 0 { - return 0, false - } - return bytesToMi(n * 1e9) - } - if hasSuffixFold(s, "T") { - n, ok := parseNumber(strings.TrimSpace(s[:len(s)-1])) - if !ok || n <= 0 { - return 0, false - } - return bytesToMi(n * 1e12) - } - if hasSuffixFold(s, "P") { - n, ok := parseNumber(strings.TrimSpace(s[:len(s)-1])) - if !ok || n <= 0 { - return 0, false - } - return bytesToMi(n * 1e15) - } - if hasSuffixFold(s, "E") { - n, ok := parseNumber(strings.TrimSpace(s[:len(s)-1])) - if !ok || n <= 0 { - return 0, false - } - return bytesToMi(n * 1e18) - } - - // ---- Bare number: interpret as Gi (not bytes) ---- - if n, ok := parseNumber(s); ok && n > 0 { - return roundGiToMi(n) - } - return 0, false -} - -// bytesToMi converts decimal bytes to Mi with rounding. -// Returns (0,false) if bytes <= 0, NaN/Inf, or if the result would overflow or round to 0. -func bytesToMi(bytes float64) (int64, bool) { - if bytes <= 0 || math.IsNaN(bytes) || math.IsInf(bytes, 0) { - return 0, false - } - miFloat := bytes / (1024.0 * 1024.0) - if miFloat > float64(math.MaxInt64) { - return 0, false - } - mi := int64(math.Round(miFloat)) - if mi <= 0 { - return 0, false - } - return mi, true -} - -// parseNumber accepts integers/decimals/scientific notation (e.g., "1e6"). -func parseNumber(s string) (float64, bool) { - s = strings.TrimSpace(s) - if s == "" { - return 0, false - } - f, err := strconv.ParseFloat(s, 64) - if err != nil { - return 0, false - } - return f, true -} - -// Case-insensitive suffix check. -func hasSuffixFold(s, suf string) bool { - return len(s) >= len(suf) && strings.EqualFold(s[len(s)-len(suf):], suf) -} diff --git a/internal/config/parser_test.go b/internal/config/parser_test.go index 971bfaf..5f49fca 100644 --- a/internal/config/parser_test.go +++ b/internal/config/parser_test.go @@ -35,9 +35,9 @@ resources: assert.Equal(t, "custom:latest", cfg.Image) assert.False(t, cfg.InstallHomebrew) // override default true - // Canonical resource units: CPU in millicores, Memory in Mi - assert.Equal(t, int64(4000), cfg.Resources.CPU) // 4 cores -> 4000m - assert.Equal(t, int64(16*1024), cfg.Resources.Memory) // 16Gi -> 16384 Mi + // Raw resource units + assert.Equal(t, int(4), cfg.Resources.CPU) + assert.Equal(t, string("16Gi"), cfg.Resources.Memory) // Also verify formatted getters via DevEnvConfig wrapper dev := &DevEnvConfig{BaseConfig: *cfg} @@ -72,8 +72,8 @@ resources: assert.Equal(t, 1000, cfg.UID) // Canonical resource defaults (CPU millicores, Memory Mi) - assert.Equal(t, int64(2000), cfg.Resources.CPU) // 2 cores - assert.Equal(t, int64(8*1024), cfg.Resources.Memory) // 8Gi + assert.Equal(t, int(2), cfg.Resources.CPU) // 2 cores + assert.Equal(t, string("8Gi"), cfg.Resources.Memory) // 8Gi assert.Equal(t, "20Gi", cfg.Resources.Storage) assert.Equal(t, 0, cfg.Resources.GPU) @@ -138,9 +138,9 @@ resources: assert.Equal(t, []string{"numpy", "pandas"}, cfg.Packages.Python) assert.Equal(t, []string{"vim"}, cfg.Packages.APT) - // Canonical resources: CPU millicores, Memory Mi - assert.Equal(t, int64(4000), cfg.Resources.CPU) // 4 cores → 4000m - assert.Equal(t, int64(16*1024), cfg.Resources.Memory) // 16Gi → 16384 Mi + // Raw Resources + assert.Equal(t, int(4), cfg.Resources.CPU) + assert.Equal(t, string("16Gi"), cfg.Resources.Memory) // Getter formatting (K8s quantities) assert.Equal(t, "4000m", cfg.CPU()) @@ -271,10 +271,10 @@ git: assert.True(t, cfg.ClearLocalPackages) // inherited from global // Canonical resource units (CPU millicores, Memory MiB) - assert.Equal(t, int64(4000), cfg.Resources.CPU) // 4 cores → 4000m - assert.Equal(t, int64(16*1024), cfg.Resources.Memory) // 16Gi → 16384Mi - assert.Equal(t, "4000m", cfg.CPU()) // formatted getter - assert.Equal(t, "16Gi", cfg.Memory()) // formatted getter + assert.Equal(t, int(4), cfg.Resources.CPU) + assert.Equal(t, string("16Gi"), cfg.Resources.Memory) + assert.Equal(t, "4000m", cfg.CPU()) // formatted getter + assert.Equal(t, "16Gi", cfg.Memory()) // formatted getter // Additive list merging (global first, then user) assert.Equal(t, []string{"curl", "git", "vim"}, cfg.Packages.APT) @@ -323,8 +323,8 @@ installHomebrew: false assert.Equal(t, "/opt/venv/bin", cfg.PythonBinPath) // system default // Canonical resource defaults and formatted getters - assert.Equal(t, int64(2000), cfg.Resources.CPU) // default 2 cores - assert.Equal(t, int64(8*1024), cfg.Resources.Memory) // default 8Gi + assert.Equal(t, int(2), cfg.Resources.CPU) // default 2 cores + assert.Equal(t, string("8Gi"), cfg.Resources.Memory) // default 8Gi assert.Equal(t, "2000m", cfg.CPU()) assert.Equal(t, "8Gi", cfg.Memory()) }) @@ -605,90 +605,3 @@ func TestNormalizeSSHKeys(t *testing.T) { }) } } - -func Test_toMi_NormalizesInputs(t *testing.T) { - tests := []struct { - name string - in any - want int64 // Mi - ok bool - }{ - // --- Binary units (powers of 2) --- - {"Gi exact", "16Gi", 16 * 1024, true}, - {"Mi exact", "512Mi", 512, true}, - {"Ki to Mi", "1024Ki", 1, true}, - {"Gi decimal", "2.5Gi", 2560, true}, - {"trim + casefold", " 2gi ", 2 * 1024, true}, // if you allow case-insensitive units - - // --- Decimal SI (powers of 10) --- - {"500M", "500M", 477, true}, // 500e6 / 2^20 ≈ 476.84 → round to 477 - {"1G", "1G", 954, true}, - - // --- Bare numeric strings (policy: treat as Gi) --- - {"bare int string", "15", 15 * 1024, true}, - {"bare float string", "1.5", 1536, true}, - - // --- Non-string numerics (policy: Gi) --- - {"int means Gi", 2, 2 * 1024, true}, - {"float means Gi", 1.5, 1536, true}, - {"uint means Gi", uint(4), 4 * 1024, true}, - - // --- Zero/negative are invalid --- - {"zero Gi invalid", "0Gi", 0, false}, - {"zero Mi invalid", "0Mi", 0, false}, - {"bare zero invalid", "0", 0, false}, - {"zero int invalid", 0, 0, false}, - {"negative Gi invalid", "-1Gi", 0, false}, - {"negative int invalid", -1, 0, false}, - - // --- Invalid shapes --- - {"invalid unit", "12GB", 0, false}, // unsupported 'GB' - {"nonnumeric", "abc", 0, false}, - {"nil", nil, 0, false}, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - got, ok := toMi(tc.in) - assert.Equal(t, tc.ok, ok) - assert.Equal(t, tc.want, got) - }) - } -} - -func TestNormalizeCPU_ToMillicores(t *testing.T) { - tests := []struct { - name string - raw any - want int64 - ok bool - }{ - {"int", 3, 3000, true}, - {"float", 1.25, 1250, true}, - {"string int", "4", 4000, true}, - {"string decimal", "2.5", 2500, true}, - {"millicores", "500m", 500, true}, - - {"zero string invalid", "0", 0, false}, - {"zero millicores invalid", "0m", 0, false}, - {"zero int invalid", 0, 0, false}, - {"negative int invalid", -1, 0, false}, - {"negative string decimal invalid", "-2.5", 0, false}, - {"negative millicores invalid", "-250m", 0, false}, - - {"spaces", " 3 ", 3000, true}, - {"leading zeros", "0003", 3000, true}, - - {"invalid", "abc", 0, false}, - {"invalid suffix", "5M", 0, false}, // only lowercase 'm' for millicores - {"nil", nil, 0, false}, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - got, ok := toMillicores(tc.raw) - assert.Equal(t, tc.ok, ok) - assert.Equal(t, tc.want, got) - }) - } -} diff --git a/internal/config/resources.go b/internal/config/resources.go new file mode 100644 index 0000000..0aabf42 --- /dev/null +++ b/internal/config/resources.go @@ -0,0 +1,294 @@ +package config + +import ( + "fmt" + "math" + "strconv" + "strings" +) + +// ============================================================================ +// --- CPU normalization pipeline --------------------------------------------- +// ============================================================================ + +// normalizeCPUText coerces a flexible value (string/int/float) into a +// canonical textual CPU quantity that k8s accepts, e.g. "2", "2.5", or "500m". +// It trims whitespace and lowercases the unit. It does NOT add "m" for you; +// integers/floats remain core-based unless the input already used "m". +func normalizeCPUText(v any) (string, error) { + switch x := v.(type) { + case nil: + return "", nil // absent; caller decides how to treat + case string: + s := strings.TrimSpace(x) + if s == "" { + return "", nil + } + s = strings.ToLower(s) + // Already millicores, e.g. "500m" + if strings.HasSuffix(s, "m") { + d := strings.TrimSpace(strings.TrimSuffix(s, "m")) + // Reject signs explicitly (ParseUint also rejects "-") + if strings.HasPrefix(d, "+") || strings.HasPrefix(d, "-") { + return "", fmt.Errorf("invalid millicores: %q", x) + } + // digits-only, non-negative, and no overflow + if _, err := strconv.ParseUint(d, 10, 64); err != nil { + return "", fmt.Errorf("invalid millicores: %q", x) + } + // normalize leading zeros (keep a single "0" if all zeros) + d = strings.TrimLeft(d, "0") + if d == "" { + d = "0" + } + return d + "m", nil + } + // Otherwise it must be a float/int string like "2", "2.5" + f, err := strconv.ParseFloat(s, 64) + if err != nil || math.IsNaN(f) || math.IsInf(f, 0) { + return "", fmt.Errorf("invalid cpu number: %q", s) + } + // Keep minimal decimal; "2" stays "2", "2.5" stays "2.5" + return strconv.FormatFloat(f, 'f', -1, 64), nil + + case int: + return strconv.FormatInt(int64(x), 10), nil + + case int64: + return strconv.FormatInt(x, 10), nil + + case float64: + if math.IsNaN(x) || math.IsInf(x, 0) { + return "", fmt.Errorf("invalid cpu number: %v", x) + } + return strconv.FormatFloat(x, 'f', -1, 64), nil + + default: + return "", fmt.Errorf("unsupported cpu type: %T", v) + } +} + +// cpuTextToMillicores converts a textual CPU quantity ("2", "2.5", "500m") +// into millicores. +// Policy: +// - empty text => 0, nil (treat as "not specified") +// - negative => error +// - "Xm" => parse as integer millicores +// - number => cores → round(f*1000) millicores +func cpuTextToMillicores(s string) (int64, error) { + if s == "" { + return 0, nil + } + if strings.HasSuffix(s, "m") { + d := strings.TrimSuffix(s, "m") + n, err := strconv.ParseInt(d, 10, 64) + if err != nil { + return 0, fmt.Errorf("invalid millicores: %q", s) + } + if n < 0 { + return 0, fmt.Errorf("cpu must be >= 0 millicores (got %d)", n) + } + return n, nil + } + f, err := strconv.ParseFloat(s, 64) + if err != nil { + return 0, fmt.Errorf("invalid cpu number: %q", s) + } + if f < 0 { + return 0, fmt.Errorf("cpu must be >= 0 cores (got %v)", f) + } + return int64(math.Round(f * 1000.0)), nil +} + +// getCanonicalCPU parses ResourceConfig.CPU on demand and returns millicores. +// This is the single entry-point your higher-level code should call. +func (r *ResourceConfig) getCanonicalCPU() (int64, error) { + text, err := normalizeCPUText(r.CPU) + if err != nil { + return 0, err + } + return cpuTextToMillicores(text) +} + +// ============================================================================ +// --- memory normalization pipeline ------------------------------------------ +// ============================================================================ + +// normalizeMemoryText coerces a flexible raw value (string/int/float/…) into a +// normalized textual quantity. It preserves the user's units (if supplied) but +// canonicalizes unit casing (e.g., "mi" -> "Mi", "g" -> "G") and trims space. +// +// Examples in -> out: +// +// " 16Gi " -> "16Gi" +// "512mi" -> "512Mi" +// "500m" -> "500m" // (this is CPU-like; if user supplies it by mistake, +// // memoryTextToMi will reject it) +// "1.5" -> "1.5" // bare number (policy: interpret as Gi later) +// 2 -> "2" +// 1.25 -> "1.25" +func normalizeMemoryText(v any) (string, error) { + switch x := v.(type) { + case nil: + return "", nil + + case string: + s := strings.TrimSpace(x) + if s == "" { + return "", nil + } + Misuffixes := [6]string{"Ki", "Mi", "Gi", "Ti", "Pi", "Ei"} + for _, suffix := range Misuffixes { + if hasSuffixFold(s, suffix) { + return strings.TrimSpace(s[:len(s)-2]) + suffix, nil + } + } + + Msuffixes := [6]string{"K", "M", "G", "T", "P", "E"} + for _, suffix := range Msuffixes { + if hasSuffixFold(s, suffix) { + return strings.TrimSpace(s[:len(s)-1]) + suffix, nil + } + } + return s, nil + + case int: + return strconv.FormatInt(int64(x), 10), nil + + case int64: + return strconv.FormatInt(x, 10), nil + + case float64: + if math.IsNaN(x) || math.IsInf(x, 0) { + return "", fmt.Errorf("invalid memory number: %v", x) + } + return strconv.FormatFloat(x, 'f', -1, 64), nil + + default: + return "", fmt.Errorf("unsupported memory type: %T", v) + } +} + +var binToMi = map[string]float64{ + "Ki": 1.0 / 1024.0, + "Mi": 1.0, + "Gi": 1024.0, + "Ti": 1024.0 * 1024.0, + "Pi": 1024.0 * 1024.0 * 1024.0, + "Ei": 1024.0 * 1024.0 * 1024.0 * 1024.0, +} + +var decBytesToMi = map[string]float64{ + "k": 1e3, + "M": 1e6, + "G": 1e9, + "T": 1e12, + "P": 1e15, + "E": 1e18, +} + +// memoryTextToMi converts a normalized textual quantity to canonical MiB. +// Policy: +// - empty => (0, nil) // “not specified” +// - negative => error +// - binary units: Ki/Mi/Gi/Ti/Pi/Ei +// - decimal bytes: k/M/G/T/P/E (10^3 … 10^18 bytes), converted to Mi +// - bare number => Gi by policy (e.g., "1.5" Gi -> 1536 Mi) +// - rejects CPU-like "m" suffix +func memoryTextToMi(s string) (int64, error) { + if s == "" { + return 0, nil + } + + // Try binary suffixes (Ki/Mi/Gi/…) + for suf, factor := range binToMi { + if strings.HasSuffix(strings.ToLower(s), strings.ToLower(suf)) { + numStr := strings.TrimSpace(s[:len(s)-len(suf)]) + n, ok := parseNumber(numStr) + if !ok || n < 0 { + return 0, fmt.Errorf("invalid %s quantity: %q", suf, s) + } + return roundFloatToInt64(n * factor) + } + } + + // // Reject CPU-like suffix "m" TODO: Revisit if this is feature to support + // if strings.HasSuffix(s, "m") || strings.HasSuffix(strings.ToLower(s), "m") { + // return 0, fmt.Errorf("invalid memory unit %q (did you mean CPU millicores?)", s) + // } + + // Try decimal byte suffixes (k/M/G/…) + for suf, mul := range decBytesToMi { + if strings.HasSuffix(strings.ToLower(s), strings.ToLower(suf)) { + numStr := strings.TrimSpace(s[:len(s)-len(suf)]) + n, ok := parseNumber(numStr) + if !ok || n < 0 { + return 0, fmt.Errorf("invalid %s bytes quantity: %q", suf, s) + } + // Convert decimal bytes → MiB. + bytes := n * mul + return bytesToMi(bytes) + } + } + + // Bare number => Gi by policy + if n, ok := parseNumber(s); ok { + if n < 0 { + return 0, fmt.Errorf("memory must be >= 0") + } + return roundFloatToInt64(n * 1024.0) + } + return 0, fmt.Errorf("invalid memory quantity: %q", s) +} + +// getCanonicalMemory parses ResourceConfig.Memory on demand and returns MiB. +func (r *ResourceConfig) getCanonicalMemory() (int64, error) { + text, err := normalizeMemoryText(r.Memory) + if err != nil { + return 0, err + } + return memoryTextToMi(text) +} + +// ---------------- helpers ---------------- + +func bytesToMi(bytes float64) (int64, error) { + if bytes < 0 || math.IsNaN(bytes) || math.IsInf(bytes, 0) { + return 0, fmt.Errorf("memory must be >= 0") + } + miFloat := bytes / (1024.0 * 1024.0) + if miFloat > float64(math.MaxInt64) { + return 0, fmt.Errorf("memory overflows supported range") + } + return roundFloatToInt64(miFloat) +} + +func roundFloatToInt64(v float64) (int64, error) { + if math.IsNaN(v) || math.IsInf(v, 0) { + return 0, fmt.Errorf("invalid number") + } + rounded := int64(math.Round(v)) + if rounded == 0 && v != 0 { + // extremely small but non-zero; treat as 0 Mi without error + return 0, nil + } + return rounded, nil +} + +// parseNumber accepts integers/decimals/scientific notation (e.g., "1e6"). +func parseNumber(s string) (float64, bool) { + s = strings.TrimSpace(s) + if s == "" { + return 0, false + } + f, err := strconv.ParseFloat(s, 64) + if err != nil { + return 0, false + } + return f, true +} + +// Case-insensitive suffix check. +func hasSuffixFold(s, suf string) bool { + return len(s) >= len(suf) && strings.EqualFold(s[len(s)-len(suf):], suf) +} diff --git a/internal/config/resources_test.go b/internal/config/resources_test.go new file mode 100644 index 0000000..9c2f6f2 --- /dev/null +++ b/internal/config/resources_test.go @@ -0,0 +1,313 @@ +package config + +import ( + "math" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// +// -------------------- CPU -------------------- +// + +func Test_normalizeCPUText(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + in any + want string + ok bool // true means expect no error + }{ + // Absent / empty + {"nil -> empty", nil, "", true}, + {"empty string -> empty", " ", "", true}, + + // Already millicores (lowercased, trimmed, leading zeros collapsed) + {"millicores simple", "500m", "500m", true}, + {"millicores uppercase M", "500M", "500m", true}, // lowercased + {"millicores trimmed", " 0500m ", "500m", true}, + {"millicores zero", "000m", "0m", true}, + {"millicores negative -> error", "-100m", "", false}, + + // Core-based numbers in text (keep minimal decimals) + {"int string", "2", "2", true}, + {"float string", "2.5", "2.5", true}, + {"float trimmed", " 3 ", "3", true}, + {"invalid string", "abc", "", false}, + + // Numeric types + {"int", 4, "4", true}, + {"int64", int64(7), "7", true}, + {"float64", 1.25, "1.25", true}, + {"float64 NaN -> error", math.NaN(), "", false}, + {"float64 +Inf -> error", math.Inf(+1), "", false}, + + // Unsupported type + {"bool -> error", true, "", false}, + } + + for _, tc := range tests { + tc := tc + t.Run(tc.name, func(t *testing.T) { + got, err := normalizeCPUText(tc.in) + if tc.ok { + require.NoError(t, err) + assert.Equal(t, tc.want, got) + } else { + require.Error(t, err) + } + }) + } +} + +func Test_cpuTextToMillicores(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + in string + want int64 + ok bool // true means expect no error + }{ + // Empty means "not specified" + {"empty -> 0,nil", "", 0, true}, + + // Millicores + {"500m -> 500", "500m", 500, true}, + {"0m -> 0", "0m", 0, true}, + {"invalid millicores", "abc m", 0, false}, + {"negative millicores -> error", "-1m", 0, false}, + + // Core numbers + {"2 -> 2000", "2", 2000, true}, + {"2.5 -> 2500", "2.5", 2500, true}, + {"negative cores -> error", "-1", 0, false}, + {"nonnumeric -> error", "abc", 0, false}, + } + + for _, tc := range tests { + tc := tc + t.Run(tc.name, func(t *testing.T) { + got, err := cpuTextToMillicores(tc.in) + if tc.ok { + require.NoError(t, err) + assert.Equal(t, tc.want, got) + } else { + require.Error(t, err) + } + }) + } +} + +func Test_getCanonicalCPU_Integration(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + raw any + want int64 + ok bool // true means expect no error + }{ + {"nil -> 0", nil, 0, true}, + {"'500m' -> 500", "500m", 500, true}, + {"'2.5' -> 2500", "2.5", 2500, true}, + {"int 3 -> 3000", 3, 3000, true}, + {"invalid -> error", "abc", 0, false}, + {"negative string -> error", "-1", 0, false}, + } + + for _, tc := range tests { + tc := tc + t.Run(tc.name, func(t *testing.T) { + r := &ResourceConfig{CPU: tc.raw} + got, err := r.getCanonicalCPU() + if tc.ok { + require.NoError(t, err) + assert.Equal(t, tc.want, got) + } else { + require.Error(t, err) + } + }) + } +} + +// +// -------------------- Memory -------------------- +// + +func Test_normalizeMemoryText(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + in any + want string + ok bool // true means expect no error + }{ + // Absent / empty + {"nil -> empty", nil, "", true}, + {"empty -> empty", " ", "", true}, + + // Binary SI (case-insensitive) → canonical case preserved + {"'512mi' -> '512Mi'", "512mi", "512Mi", true}, + {"' 2gi ' -> '2Gi'", " 2gi ", "2Gi", true}, + {"'1Ti' -> '1Ti'", "1Ti", "1Ti", true}, + + // Decimal bytes (keep suffix case) + {"'500M' -> '500M'", "500M", "500M", true}, + {"'1G' -> '1G'", "1G", "1G", true}, + + // Bare numeric + {"'1.5' -> '1.5'", "1.5", "1.5", true}, + + // Numeric types + {"int -> '2'", 2, "2", true}, + {"int64 -> '7'", int64(7), "7", true}, + {"float64 -> '1.25'", 1.25, "1.25", true}, + {"float NaN -> error", math.NaN(), "", false}, + {"float Inf -> error", math.Inf(+1), "", false}, + + // Unsupported type + {"bool -> error", true, "", false}, + } + + for _, tc := range tests { + tc := tc + t.Run(tc.name, func(t *testing.T) { + got, err := normalizeMemoryText(tc.in) + if tc.ok { + require.NoError(t, err) + assert.Equal(t, tc.want, got) + } else { + require.Error(t, err) + } + }) + } +} + +func Test_memoryTextToMi(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + in string + want int64 + ok bool // true means expect no error + }{ + {"empty -> 0,nil", "", 0, true}, + + // Binary SI + {"16Gi -> 16384", "16Gi", 16 * 1024, true}, + {"512Mi -> 512", "512Mi", 512, true}, + {"1024Ki -> 1", "1024Ki", 1, true}, + {"1.5Gi -> 1536", "1.5Gi", 1536, true}, + + // Decimal bytes → Mi (rounded) + {"500M -> 477Mi", "500M", 477, true}, + {"1G -> 954Mi", "1G", 954, true}, + + // Bare number => Gi policy + {"'15' -> 15360", "15", 15 * 1024, true}, + {"'1.25' -> 1280", "1.25", 1280, true}, + + // Rejections / errors + {"negative Gi -> error", "-1Gi", 0, false}, + {"invalid unit -> error", "12GB", 0, false}, + {"nonnumeric -> error", "abc", 0, false}, + } + + for _, tc := range tests { + tc := tc + t.Run(tc.name, func(t *testing.T) { + got, err := memoryTextToMi(tc.in) + if tc.ok { + require.NoError(t, err) + assert.Equal(t, tc.want, got) + } else { + require.Error(t, err) + } + }) + } +} + +func Test_getCanonicalMemory_Integration(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + raw any + want int64 + ok bool + }{ + {"nil -> 0", nil, 0, true}, + {"Gi", "2Gi", 2 * 1024, true}, + {"Mi", "512Mi", 512, true}, + {"decimal bytes", "500M", 477, true}, + {"bare Gi", "1.5", 1536, true}, + {"int Gi", 2, 2 * 1024, true}, + {"float Gi", 1.25, 1280, true}, + {"invalid", "abc", 0, false}, + } + + for _, tc := range tests { + tc := tc + t.Run(tc.name, func(t *testing.T) { + r := &ResourceConfig{Memory: tc.raw} + got, err := r.getCanonicalMemory() + if tc.ok { + require.NoError(t, err) + assert.Equal(t, tc.want, got) + } else { + require.Error(t, err) + } + }) + } +} + +// +// -------------------- Helpers (spot checks) -------------------- +// + +func Test_bytesToMi_SpotChecks(t *testing.T) { + t.Parallel() + + // 1 byte -> ~9.5e-7 Mi -> rounds to 0 without error + got, err := bytesToMi(1) + require.NoError(t, err) + assert.Equal(t, int64(0), got) + + // Negative -> error + _, err = bytesToMi(-1) + require.Error(t, err) + + // Very large -> overflow error (simulate near MaxInt64 Mi in bytes) + _, err = bytesToMi(math.MaxFloat64) + require.Error(t, err) +} + +func Test_roundFloatToInt64_SpotChecks(t *testing.T) { + t.Parallel() + + // Normal rounding + got, err := roundFloatToInt64(1.49) + require.NoError(t, err) + assert.Equal(t, int64(1), got) + + got, err = roundFloatToInt64(1.5) + require.NoError(t, err) + assert.Equal(t, int64(2), got) + + // Tiny non-zero -> 0, no error + got, err = roundFloatToInt64(1e-12) + require.NoError(t, err) + assert.Equal(t, int64(0), got) + + // NaN / Inf -> error + _, err = roundFloatToInt64(math.NaN()) + require.Error(t, err) + _, err = roundFloatToInt64(math.Inf(+1)) + require.Error(t, err) +} diff --git a/internal/config/types.go b/internal/config/types.go index f6fa06d..edd2e3c 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -73,12 +73,10 @@ type PackageConfig struct { // ResourceConfig represents resource allocation type ResourceConfig struct { - CPURaw any `yaml:"cpu,omitempty" validate:"omitempty,k8s_cpu"` // Can be string or int - MemoryRaw any `yaml:"memory,omitempty" validate:"omitempty,k8s_memory"` - CPU int64 `yaml:"-"` // canonical - Memory int64 `yaml:"-"` // canonical - Storage string `yaml:"storage,omitempty" validate:"omitempty,k8s_memory"` - GPU int `yaml:"gpu,omitempty" validate:"omitempty,min=0,max=8"` // Number of GPUs requested + CPU any `yaml:"cpu,omitempty" validate:"omitempty,k8s_cpu"` + Memory any `yaml:"memory,omitempty" validate:"omitempty,k8s_memory"` + Storage string `yaml:"storage,omitempty" validate:"omitempty,k8s_memory"` + GPU int `yaml:"gpu,omitempty" validate:"omitempty,min=0,max=8"` // Number of GPUs requested } // VolumeMount represents a volume mount configuration @@ -106,10 +104,10 @@ func NewBaseConfigWithDefaults() BaseConfig { ClearVSCodeCache: false, PythonBinPath: "/opt/venv/bin", Resources: ResourceConfig{ - CPU: 2000, // Default CPU (2 cores) - Memory: 8 * 1024, // Default Memory (8Gi -> 8192Mi) - Storage: "20Gi", // Default Storage - GPU: 0, // Default GPU + CPU: 2, // Default CPU + Memory: "8Gi", // Default Memory + Storage: "20Gi", // Default Storage + GPU: 0, // Default GPU }, Packages: PackageConfig{ Python: []string{}, // Empty slice - no default packages @@ -160,25 +158,23 @@ func (c *DevEnvConfig) GPU() int { // Assumes Resources.CPU has already been normalized to millicores. // Returns "0" if CPU <= 0 so callers can omit the field or treat as no request. func (c *DevEnvConfig) CPU() string { - if c.Resources.CPU <= 0 { + CPU_in_millicores, err := c.Resources.getCanonicalCPU() + if err != nil || CPU_in_millicores <= 0 { return "0" } - return fmt.Sprintf("%dm", c.Resources.CPU) + return fmt.Sprintf("%dm", CPU_in_millicores) } -// Memory returns the canonical memory quantity formatted for Kubernetes. -// Assumes Resources.Memory has been normalized to Mi (mebibytes). -// Returns "" if Memory <= 0 so callers can omit the field. -// Uses "Gi" when an exact Gi multiple, otherwise "Mi". +// Memory returns "Gi" or "Mi" ("" means omit). func (c *DevEnvConfig) Memory() string { - mi := c.Resources.Memory - if mi <= 0 { + memory_in_Mi, err := c.Resources.getCanonicalMemory() + if err != nil || memory_in_Mi <= 0 { return "" } - if mi%1024 == 0 { - return fmt.Sprintf("%dGi", mi/1024) + if memory_in_Mi%1024 == 0 { + return fmt.Sprintf("%dGi", memory_in_Mi/1024) } - return fmt.Sprintf("%dMi", mi) + return fmt.Sprintf("%dMi", memory_in_Mi) } // CPURequest returns the CPU resource request as a string suitable for Kubernetes manifests. diff --git a/internal/config/types_test.go b/internal/config/types_test.go index 3e35180..d2c4ba0 100644 --- a/internal/config/types_test.go +++ b/internal/config/types_test.go @@ -8,10 +8,7 @@ import ( "github.com/stretchr/testify/require" ) -// TestNewBaseConfigWithDefaults verifies that the factory defaults produce a -// ready-to-use config with canonical resource units and non-nil empty slices. -// This test documents the contract of NewBaseConfigWithDefaults so future changes -// (e.g., image tag, default resources, or slice initialization) don’t regress silently. +// TestNewBaseConfigWithDefaults verifies non-parsed or normalized factory defaults func TestNewBaseConfigWithDefaults(t *testing.T) { cfg := NewBaseConfigWithDefaults() @@ -25,11 +22,9 @@ func TestNewBaseConfigWithDefaults(t *testing.T) { assert.False(t, cfg.ClearLocalPackages) assert.False(t, cfg.ClearVSCodeCache) - // --- Resource defaults (canonical units) --- - // Canonical CPU is millicores; default should be 2 cores = 2000m. - assert.Equal(t, int64(2000), cfg.Resources.CPU) - // Canonical Memory is Mi; default should be 8 Gi = 8192 Mi. - assert.Equal(t, int64(8192), cfg.Resources.Memory) + // --- Resources --- + assert.Equal(t, 2, cfg.Resources.CPU) + assert.Equal(t, "8Gi", cfg.Resources.Memory) assert.Equal(t, "20Gi", cfg.Resources.Storage) assert.Equal(t, 0, cfg.Resources.GPU) @@ -37,10 +32,6 @@ func TestNewBaseConfigWithDefaults(t *testing.T) { assert.Equal(t, "2000m", (&DevEnvConfig{BaseConfig: cfg}).CPU()) assert.Equal(t, "8Gi", (&DevEnvConfig{BaseConfig: cfg}).Memory()) - // Raw fields should be empty in defaults (we set canonical values directly). - assert.Nil(t, cfg.Resources.CPURaw) - assert.Nil(t, cfg.Resources.MemoryRaw) - // --- Empty-but-non-nil slices (ergonomic contract) --- // Callers can append without nil checks; order is preserved. assert.NotNil(t, cfg.Packages.Python) @@ -165,19 +156,38 @@ func TestDevEnvConfig_GetUserID(t *testing.T) { } } -// TestDevEnvConfig_CPU_Format verifies that CPU() is a pure formatter over the -// canonical millicores field (Resources.CPU). It does not parse raw inputs or -// apply defaults—those happen during normalization. -func TestDevEnvConfig_CPU_Format(t *testing.T) { +// TestDevEnvConfig_CPU_Format verifies that CPU() is correctly formatting cpu information to millicores. +func TestDevEnvConfig_CPU(t *testing.T) { tests := []struct { name string - milli int64 + milli any want string }{ - {name: "zero -> 0", milli: 0, want: "0"}, - {name: "positive -> Xm", milli: 2500, want: "2500m"}, - {name: "large -> Xm", milli: 123456, want: "123456m"}, - {name: "negative clamps", milli: -1, want: "0"}, + // int + {name: "int, positive -> Xm", milli: 23, want: "23000m"}, + {name: "int, negative -> 0", milli: -42, want: "0"}, + {name: "int, zero -> 0", milli: 0, want: "0"}, + + // float + {name: "float, positive -> Xm", milli: 77.3, want: "77300m"}, + {name: "float, negative -> 0", milli: -223.21, want: "0"}, + {name: "float, zero -> 0", milli: 0.0, want: "0"}, + + // string + {name: "string containing: int, positive -> Xm", milli: 89, want: "89000m"}, + {name: "string containing: int, negative -> 0", milli: -37, want: "0"}, + {name: "string containing: int, zero -> 0", milli: 0, want: "0"}, + {name: "string containing: float, positive -> Xm", milli: 34.7, want: "34700m"}, + {name: "string containing: float, negative -> 0", milli: -2.1, want: "0"}, + {name: "string containing: float, zero -> 0", milli: 0.0, want: "0"}, + + // string that contains 'm' + {name: "m-string containing: int, positive -> Xm", milli: "89m", want: "89m"}, + {name: "m-string containing: int, negative -> 0", milli: "-37m", want: "0"}, + {name: "m-string containing: int, zero -> 0", milli: "0", want: "0"}, + {name: "m-string containing: float, positive -> Xm", milli: "34.7m", want: "0"}, // Cannot have fractions of millicores + {name: "m-string containing: float, negative -> 0", milli: "-2.1m", want: "0"}, + {name: "m-string containing: float, zero -> 0", milli: "0.0m", want: "0"}, } for _, tc := range tests { @@ -192,39 +202,38 @@ func TestDevEnvConfig_CPU_Format(t *testing.T) { } } -// CPU() must format canonical Resources.CPU and ignore CPURaw if canonical isn't set. -func TestDevEnvConfig_CPU_IgnoresRawWithoutNormalization(t *testing.T) { - // Raw provided, canonical zero => CPU() should not look at CPURaw. - cfg := &DevEnvConfig{ - BaseConfig: BaseConfig{ - Resources: ResourceConfig{CPURaw: "2.5", CPU: 0}, - }, - } - require.Equal(t, "0", cfg.CPU()) - - // Canonical set => CPU() formats it regardless of CPURaw. - cfg2 := &DevEnvConfig{ - BaseConfig: BaseConfig{ - Resources: ResourceConfig{CPURaw: "bad", CPU: 2500}, - }, - } - require.Equal(t, "2500m", cfg2.CPU()) -} - -// TestDevEnvConfig_Memory_Format verifies that Memory() is a pure formatter over the -// canonical Mi field (Resources.Memory). It does not parse raw inputs; normalization -// must have already populated Memory in MiB. Exact Gi multiples render as "Gi", otherwise "Mi". -func TestDevEnvConfig_Memory_Format(t *testing.T) { +// TestDevEnvConfig_Memory verifies that Memory() is correctly formatting memory information. +func TestDevEnvConfig_Memory(t *testing.T) { tests := []struct { name string - memMi int64 + memMi any want string }{ - {name: "zero -> empty", memMi: 0, want: ""}, - {name: "exact Gi prints Gi", memMi: 16 * 1024, want: "16Gi"}, - {name: "non-Gi prints Mi", memMi: 1536, want: "1536Mi"}, // 1.5 Gi - {name: "small Mi prints Mi", memMi: 477, want: "477Mi"}, - {name: "negative -> empty", memMi: -1, want: ""}, + // int + {name: "int, zero -> empty", memMi: 0, want: ""}, + {name: "int, positive -> XGi", memMi: 1, want: "1Gi"}, + {name: "int, negative -> empty", memMi: -1, want: ""}, + + // float + {name: "float, positive -> XMi", memMi: 1.5, want: "1536Mi"}, + {name: "float, negative -> XMi", memMi: -1.5, want: ""}, + {name: "float, zero -> XMi", memMi: 0.0, want: ""}, + + // string + {name: "string containing: int, positive -> XMi", memMi: "1", want: "1Gi"}, + {name: "string containing: int, negative -> XMi", memMi: "-1", want: ""}, + {name: "string containing: int, zero -> XMi", memMi: "0", want: ""}, + {name: "string containing: float, positive -> XMi", memMi: "1.1", want: "1126Mi"}, + {name: "string containing: float, negative -> XMi", memMi: "-1.0", want: ""}, + {name: "string containing: float, zero -> XMi", memMi: "0.0", want: ""}, + + //string with suffix + {name: "suffix-string containing: int, positive -> XMi", memMi: "1Gi", want: "1Gi"}, + {name: "suffix-string containing: int, negative -> XMi", memMi: "-1Ki", want: ""}, + {name: "suffix-string containing: int, zero -> XMi", memMi: "0Gi", want: ""}, + {name: "suffix-string containing: float, positive -> XMi", memMi: "1.0Ti", want: "1024Gi"}, + {name: "suffix-string containing: float, negative -> XMi", memMi: "-1.4Ei", want: ""}, + {name: "suffix-string containing: float, zero -> XMi", memMi: "0.0Pi", want: ""}, } for _, tc := range tests { @@ -239,53 +248,6 @@ func TestDevEnvConfig_Memory_Format(t *testing.T) { } } -// Memory() must format canonical Resources.Memory and ignore MemoryRaw if canonical isn't set. -func TestDevEnvConfig_Memory_IgnoresRawWithoutNormalization(t *testing.T) { - cfg := &DevEnvConfig{ - BaseConfig: BaseConfig{ - Resources: ResourceConfig{MemoryRaw: "1.5Gi", Memory: 0}, - }, - } - require.Equal(t, "", cfg.Memory()) - - cfg2 := &DevEnvConfig{ - BaseConfig: BaseConfig{ - Resources: ResourceConfig{MemoryRaw: "bad", Memory: 1536}, - }, - } - require.Equal(t, "1536Mi", cfg2.Memory()) -} - -// TestDevEnvConfig_Memory_FormatFromCanonical verifies that Memory() is a pure -// formatter over the canonical Mi field (Resources.Memory). Exact Gi multiples -// render with "Gi"; all other positive values render with "Mi"; non-positive -// values render as an empty string. -func TestDevEnvConfig_Memory_FormatFromCanonical(t *testing.T) { - tests := []struct { - name string - memory int64 // canonical Mi - want string - }{ - {name: "zero -> empty", memory: 0, want: ""}, - {name: "negative -> empty", memory: -1, want: ""}, - {name: "exact Gi", memory: 16 * 1024, want: "16Gi"}, - {name: "non-Gi (1.5Gi)", memory: 1536, want: "1536Mi"}, - {name: "non-Gi (small Mi)", memory: 477, want: "477Mi"}, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - cfg := &DevEnvConfig{ - BaseConfig: BaseConfig{ - Resources: ResourceConfig{Memory: tc.memory}, - }, - } - got := cfg.Memory() - assert.Equal(t, tc.want, got) - }) - } -} - // TestDevEnvConfig_GPU documents the contract of GPU(): it returns a non-negative // GPU count by clamping negatives to zero, and defaults to zero when unset. func TestDevEnvConfig_GPU(t *testing.T) { @@ -520,78 +482,88 @@ func TestDevEnvConfig_GetDeveloperDir(t *testing.T) { // exercise parsing/normalization; it only covers the formatting layer. func TestDevEnvConfig_CPURequest_AliasOfCPU(t *testing.T) { tests := []struct { - name string - milli int64 // canonical millicores + name string + raw any // CPURaw: string/int/float forms supported by your parser + want string }{ - {name: "zero", milli: 0}, - {name: "positive", milli: 2500}, - {name: "large", milli: 123456}, + // Valid forms + {name: "core integer string", raw: "4", want: "4000m"}, + {name: "fractional cores string", raw: "2.5", want: "2500m"}, + {name: "millicores string", raw: "500m", want: "500m"}, + {name: "int cores", raw: 3, want: "3000m"}, + {name: "float cores", raw: 1.25, want: "1250m"}, + + // Degenerate / invalid → empty (omit in manifests) + {name: "zero", raw: "0", want: "0"}, + {name: "negative", raw: -1, want: "0"}, + {name: "invalid string", raw: "abc", want: "0"}, + {name: "nil", raw: nil, want: "0"}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { cfg := &DevEnvConfig{ BaseConfig: BaseConfig{ - Resources: ResourceConfig{CPU: tc.milli}, + Resources: ResourceConfig{ + CPU: tc.raw, + }, }, } gotCPU := cfg.CPU() + assert.Equal(t, tc.want, gotCPU, "CPU() should format from CPURaw via getCanonicalCPU()") + gotReq := cfg.CPURequest() - assert.Equal(t, gotCPU, gotReq) + assert.Equal(t, gotCPU, gotReq, "CPURequest() must be an exact alias of CPU()") }) } - - // Guard: CPURaw alone (without normalization) has no effect on getters. - t.Run("raw value ignored without normalization", func(t *testing.T) { - cfg := &DevEnvConfig{ - BaseConfig: BaseConfig{ - Resources: ResourceConfig{CPURaw: "2.5", CPU: 0}, - }, - } - assert.Equal(t, "0", cfg.CPU()) - assert.Equal(t, cfg.CPU(), cfg.CPURequest()) - }) } -// TestDevEnvConfig_MemoryRequest_AliasOfMemory documents that MemoryRequest() -// returns exactly what Memory() returns for canonical mebibytes (Mi). This -// exercises the formatting layer only; parsing/normalization is tested elsewhere. +// TestDevEnvConfig_MemoryRequest_AliasOfMemory verifies that MemoryRequest() +// returns exactly what Memory() returns when Memory() computes from MemoryRaw +// via getCanonicalMemory(). We only test the wrapper/presentation behavior here; +// detailed normalization cases live in resources_test.go. func TestDevEnvConfig_MemoryRequest_AliasOfMemory(t *testing.T) { tests := []struct { - name string - memMi int64 // canonical Mi + name string + raw any // MemoryRaw: forms supported by your parser + want string }{ - {name: "zero -> empty", memMi: 0}, - {name: "exact Gi formats Gi", memMi: 16 * 1024}, // "16Gi" - {name: "non-Gi (1.5Gi) -> Mi", memMi: 1536}, // "1536Mi" - {name: "non-Gi (small Mi)", memMi: 477}, // "477Mi" - // Under strict normalization, canonical should not be negative; this is defensive. - {name: "negative -> empty (defensive)", memMi: -1}, + // --- Valid forms --- + {name: "Gi exact", raw: "16Gi", want: "16Gi"}, + {name: "Mi exact", raw: "512Mi", want: "512Mi"}, + {name: "fractional Gi -> Mi", raw: "1.5Gi", want: "1536Mi"}, // 1.5 * 1024 = 1536 Mi + {name: "trim & case-insensitive", raw: " 2gi ", want: "2Gi"}, + {name: "bare numeric (Gi policy)", raw: "15", want: "15Gi"}, + + // Non-string numerics (if supported by your parser; typically YAML gives float64/int) + {name: "int means Gi", raw: 2, want: "2Gi"}, + {name: "float means Gi", raw: 1.25, want: "1280Mi"}, // 1.25 * 1024 = 1280 Mi + + // --- Degenerate / invalid → empty string (omit in manifests) --- + {name: "zero Gi -> empty", raw: "0Gi", want: ""}, + {name: "zero bare -> empty", raw: "0", want: ""}, + {name: "negative -> empty", raw: "-1Gi", want: ""}, + {name: "invalid unit -> empty", raw: "12GB", want: ""}, + {name: "nonnumeric -> empty", raw: "abc", want: ""}, + {name: "nil -> empty", raw: nil, want: ""}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { cfg := &DevEnvConfig{ BaseConfig: BaseConfig{ - Resources: ResourceConfig{Memory: tc.memMi}, + Resources: ResourceConfig{ + Memory: tc.raw, + }, }, } gotMem := cfg.Memory() + assert.Equal(t, tc.want, gotMem, "Memory() should format from MemoryRaw via getCanonicalMemory()") + gotReq := cfg.MemoryRequest() - assert.Equal(t, gotMem, gotReq) + assert.Equal(t, gotMem, gotReq, "MemoryRequest() must be an exact alias of Memory()") }) } - - // Guard: MemoryRaw alone (without normalization) has no effect on getters. - t.Run("raw value ignored without normalization", func(t *testing.T) { - cfg := &DevEnvConfig{ - BaseConfig: BaseConfig{ - Resources: ResourceConfig{MemoryRaw: "1.5Gi", Memory: 0}, - }, - } - assert.Equal(t, "", cfg.Memory()) - assert.Equal(t, cfg.Memory(), cfg.MemoryRequest()) - }) } // TestDevEnvConfig_Embedding verifies Go struct embedding behavior for this API: @@ -653,15 +625,11 @@ func TestNewBaseConfigWithDefaults_ExactValues(t *testing.T) { require.Equal(t, "/opt/venv/bin", cfg.PythonBinPath) // Resources (canonical) - require.Equal(t, int64(2000), cfg.Resources.CPU) // 2 cores - require.Equal(t, int64(8192), cfg.Resources.Memory) // 8Gi + require.Equal(t, int(2), cfg.Resources.CPU) // 2 cores + require.Equal(t, string("8Gi"), cfg.Resources.Memory) // 8Gi require.Equal(t, "20Gi", cfg.Resources.Storage) require.Equal(t, 0, cfg.Resources.GPU) - // Raw fields should be empty when canonical defaults are used - require.Nil(t, cfg.Resources.CPURaw) - require.Nil(t, cfg.Resources.MemoryRaw) - // Packages: non-nil, length 0 require.NotNil(t, cfg.Packages.Python) require.Len(t, cfg.Packages.Python, 0) diff --git a/internal/config/validation.go b/internal/config/validation.go index 645b946..de52a01 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -2,6 +2,7 @@ package config import ( "fmt" + "math" "regexp" "strconv" "strings" @@ -106,23 +107,11 @@ func validateKubernetesCPU(fl validator.FieldLevel) bool { case int: return v >= 0 - case int8: - return v >= 0 - case int16: - return v >= 0 - case int32: - return v >= 0 case int64: return v >= 0 - case uint, uint8, uint16, uint32, uint64: - // Unsigned are non-negative by definition. - return true - - case float32: - return !float32IsNaNOrInf(v) && v >= 0 case float64: - return !float64IsNaNOrInf(v) && v >= 0 + return !math.IsNaN(v) && !math.IsInf(v, 0) && v >= 0 default: return false @@ -161,22 +150,12 @@ func validateKubernetesMemory(fl validator.FieldLevel) bool { case int: return v >= 0 - case int8: - return v >= 0 - case int16: - return v >= 0 - case int32: - return v >= 0 + case int64: return v >= 0 - case uint, uint8, uint16, uint32, uint64: - return true - - case float32: - return !float32IsNaNOrInf(v) && v >= 0 case float64: - return !float64IsNaNOrInf(v) && v >= 0 + return !math.IsNaN(v) && !math.IsInf(v, 0) && v >= 0 default: return false @@ -200,13 +179,14 @@ func ValidateDevEnvConfig(config *DevEnvConfig) error { return fmt.Errorf("at least one SSH public key is required") } - // Canonical resources (already normalized) must be non-negative. - if config.Resources.CPU < 0 { - return fmt.Errorf("cpu must be >= 0") + if mc, err := config.Resources.getCanonicalCPU(); err != nil || mc < 0 { + return err // "cpu must be >= 0" } - if config.Resources.Memory < 0 { - return fmt.Errorf("memory must be >= 0") + + if mi, err := config.Resources.getCanonicalMemory(); err != nil || mi < 0 { + return err // "memory must be >= 0" } + if config.Resources.GPU < 0 { return fmt.Errorf("gpu must be >= 0") } @@ -273,11 +253,3 @@ func formatFieldError(fieldError validator.FieldError) string { return fmt.Sprintf("'%s' failed validation '%s', got '%v'", fieldName, tag, value) } } - -// Small float guards for validator predicates (avoid NaN/Inf). -func float32IsNaNOrInf(f float32) bool { - return f != f || f > 1e38 || f < -1e38 -} -func float64IsNaNOrInf(f float64) bool { - return f != f || f > 1e308 || f < -1e308 -} diff --git a/internal/config/validation_test.go b/internal/config/validation_test.go index 489e903..4c44399 100644 --- a/internal/config/validation_test.go +++ b/internal/config/validation_test.go @@ -80,7 +80,6 @@ func TestValidator_K8SCPU(t *testing.T) { // Accept numerics {"int", 3, true}, {"float64", 1.25, true}, - {"uint", uint(4), true}, // Reject negatives / junk {"negative string", "-1", false}, diff --git a/internal/templates/renderer_test.go b/internal/templates/renderer_test.go index ef3df17..bcfe4c6 100644 --- a/internal/templates/renderer_test.go +++ b/internal/templates/renderer_test.go @@ -31,8 +31,8 @@ func TestRenderTemplate(t *testing.T) { APT: []string{"vim", "curl"}, }, Resources: config.ResourceConfig{ - CPU: 4000, - Memory: 16 * 1024, + CPU: "4", + Memory: "16Gi", Storage: "100Gi", GPU: 2, }, From 8385f8397d1301074e67abe1872466d37afd428f Mon Sep 17 00:00:00 2001 From: Max Gagnon <82063767+maxwellgagnon@users.noreply.github.com> Date: Tue, 21 Oct 2025 12:14:41 -0700 Subject: [PATCH 08/14] Remove support for int64; clean up code --- internal/config/parser.go | 1 - internal/config/resources.go | 36 ++++++++----------------------- internal/config/resources_test.go | 8 +------ internal/config/types.go | 1 - internal/config/validation.go | 5 ----- 5 files changed, 10 insertions(+), 41 deletions(-) diff --git a/internal/config/parser.go b/internal/config/parser.go index 26888b5..3c048c3 100644 --- a/internal/config/parser.go +++ b/internal/config/parser.go @@ -114,7 +114,6 @@ func LoadDeveloperConfigWithBaseConfig(configDir, developerName string, baseConf // Step 6: Set developer directory and validate userConfig.DeveloperDir = developerDir - // Step 7: Normalize flexible/raw fields → canonical representation if err := userConfig.Validate(); err != nil { return nil, fmt.Errorf("invalid configuration in %s: %w", configPath, err) } diff --git a/internal/config/resources.go b/internal/config/resources.go index 0aabf42..03164e3 100644 --- a/internal/config/resources.go +++ b/internal/config/resources.go @@ -54,9 +54,6 @@ func normalizeCPUText(v any) (string, error) { case int: return strconv.FormatInt(int64(x), 10), nil - case int64: - return strconv.FormatInt(x, 10), nil - case float64: if math.IsNaN(x) || math.IsInf(x, 0) { return "", fmt.Errorf("invalid cpu number: %v", x) @@ -122,9 +119,8 @@ func (r *ResourceConfig) getCanonicalCPU() (int64, error) { // // " 16Gi " -> "16Gi" // "512mi" -> "512Mi" -// "500m" -> "500m" // (this is CPU-like; if user supplies it by mistake, -// // memoryTextToMi will reject it) -// "1.5" -> "1.5" // bare number (policy: interpret as Gi later) +// "500m" -> "500m" +// "1.5" -> "1.5" // 2 -> "2" // 1.25 -> "1.25" func normalizeMemoryText(v any) (string, error) { @@ -155,9 +151,6 @@ func normalizeMemoryText(v any) (string, error) { case int: return strconv.FormatInt(int64(x), 10), nil - case int64: - return strconv.FormatInt(x, 10), nil - case float64: if math.IsNaN(x) || math.IsInf(x, 0) { return "", fmt.Errorf("invalid memory number: %v", x) @@ -204,8 +197,8 @@ func memoryTextToMi(s string) (int64, error) { for suf, factor := range binToMi { if strings.HasSuffix(strings.ToLower(s), strings.ToLower(suf)) { numStr := strings.TrimSpace(s[:len(s)-len(suf)]) - n, ok := parseNumber(numStr) - if !ok || n < 0 { + n, err := strconv.ParseFloat(numStr, 64) + if err != nil || math.IsNaN(n) || math.IsInf(n, 0) || n < 0 { return 0, fmt.Errorf("invalid %s quantity: %q", suf, s) } return roundFloatToInt64(n * factor) @@ -221,8 +214,8 @@ func memoryTextToMi(s string) (int64, error) { for suf, mul := range decBytesToMi { if strings.HasSuffix(strings.ToLower(s), strings.ToLower(suf)) { numStr := strings.TrimSpace(s[:len(s)-len(suf)]) - n, ok := parseNumber(numStr) - if !ok || n < 0 { + n, err := strconv.ParseFloat(numStr, 64) + if err != nil || math.IsNaN(n) || math.IsInf(n, 0) || n < 0 { return 0, fmt.Errorf("invalid %s bytes quantity: %q", suf, s) } // Convert decimal bytes → MiB. @@ -232,10 +225,12 @@ func memoryTextToMi(s string) (int64, error) { } // Bare number => Gi by policy - if n, ok := parseNumber(s); ok { + n, err := strconv.ParseFloat(strings.TrimSpace(s), 64) + if err == nil && !math.IsNaN(n) && !math.IsInf(n, 0) { if n < 0 { return 0, fmt.Errorf("memory must be >= 0") } + // Bare number => Gi by policy, convert to Mi. return roundFloatToInt64(n * 1024.0) } return 0, fmt.Errorf("invalid memory quantity: %q", s) @@ -275,19 +270,6 @@ func roundFloatToInt64(v float64) (int64, error) { return rounded, nil } -// parseNumber accepts integers/decimals/scientific notation (e.g., "1e6"). -func parseNumber(s string) (float64, bool) { - s = strings.TrimSpace(s) - if s == "" { - return 0, false - } - f, err := strconv.ParseFloat(s, 64) - if err != nil { - return 0, false - } - return f, true -} - // Case-insensitive suffix check. func hasSuffixFold(s, suf string) bool { return len(s) >= len(suf) && strings.EqualFold(s[len(s)-len(suf):], suf) diff --git a/internal/config/resources_test.go b/internal/config/resources_test.go index 9c2f6f2..0846ac8 100644 --- a/internal/config/resources_test.go +++ b/internal/config/resources_test.go @@ -40,7 +40,7 @@ func Test_normalizeCPUText(t *testing.T) { // Numeric types {"int", 4, "4", true}, - {"int64", int64(7), "7", true}, + {"int64", int(7), "7", true}, {"float64", 1.25, "1.25", true}, {"float64 NaN -> error", math.NaN(), "", false}, {"float64 +Inf -> error", math.Inf(+1), "", false}, @@ -50,7 +50,6 @@ func Test_normalizeCPUText(t *testing.T) { } for _, tc := range tests { - tc := tc t.Run(tc.name, func(t *testing.T) { got, err := normalizeCPUText(tc.in) if tc.ok { @@ -89,7 +88,6 @@ func Test_cpuTextToMillicores(t *testing.T) { } for _, tc := range tests { - tc := tc t.Run(tc.name, func(t *testing.T) { got, err := cpuTextToMillicores(tc.in) if tc.ok { @@ -120,7 +118,6 @@ func Test_getCanonicalCPU_Integration(t *testing.T) { } for _, tc := range tests { - tc := tc t.Run(tc.name, func(t *testing.T) { r := &ResourceConfig{CPU: tc.raw} got, err := r.getCanonicalCPU() @@ -165,7 +162,6 @@ func Test_normalizeMemoryText(t *testing.T) { // Numeric types {"int -> '2'", 2, "2", true}, - {"int64 -> '7'", int64(7), "7", true}, {"float64 -> '1.25'", 1.25, "1.25", true}, {"float NaN -> error", math.NaN(), "", false}, {"float Inf -> error", math.Inf(+1), "", false}, @@ -175,7 +171,6 @@ func Test_normalizeMemoryText(t *testing.T) { } for _, tc := range tests { - tc := tc t.Run(tc.name, func(t *testing.T) { got, err := normalizeMemoryText(tc.in) if tc.ok { @@ -220,7 +215,6 @@ func Test_memoryTextToMi(t *testing.T) { } for _, tc := range tests { - tc := tc t.Run(tc.name, func(t *testing.T) { got, err := memoryTextToMi(tc.in) if tc.ok { diff --git a/internal/config/types.go b/internal/config/types.go index edd2e3c..b0c7cfa 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -155,7 +155,6 @@ func (c *DevEnvConfig) GPU() int { } // CPU returns the canonical CPU quantity formatted for Kubernetes (e.g., "2500m"). -// Assumes Resources.CPU has already been normalized to millicores. // Returns "0" if CPU <= 0 so callers can omit the field or treat as no request. func (c *DevEnvConfig) CPU() string { CPU_in_millicores, err := c.Resources.getCanonicalCPU() diff --git a/internal/config/validation.go b/internal/config/validation.go index de52a01..989cdad 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -107,8 +107,6 @@ func validateKubernetesCPU(fl validator.FieldLevel) bool { case int: return v >= 0 - case int64: - return v >= 0 case float64: return !math.IsNaN(v) && !math.IsInf(v, 0) && v >= 0 @@ -151,9 +149,6 @@ func validateKubernetesMemory(fl validator.FieldLevel) bool { case int: return v >= 0 - case int64: - return v >= 0 - case float64: return !math.IsNaN(v) && !math.IsInf(v, 0) && v >= 0 From 6fa046ddd5e2a2f977541e6c71677bb32ac67f17 Mon Sep 17 00:00:00 2001 From: Max Gagnon <82063767+maxwellgagnon@users.noreply.github.com> Date: Sun, 26 Oct 2025 16:01:55 -0700 Subject: [PATCH 09/14] Change normalizeMemoryText & normalizeCPUText function names to increase clarity --- internal/config/resources.go | 10 +++++----- internal/config/resources_test.go | 8 ++++---- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/internal/config/resources.go b/internal/config/resources.go index 03164e3..5ac2250 100644 --- a/internal/config/resources.go +++ b/internal/config/resources.go @@ -11,11 +11,11 @@ import ( // --- CPU normalization pipeline --------------------------------------------- // ============================================================================ -// normalizeCPUText coerces a flexible value (string/int/float) into a +// normalizeToCPUText coerces a flexible value (string/int/float) into a // canonical textual CPU quantity that k8s accepts, e.g. "2", "2.5", or "500m". // It trims whitespace and lowercases the unit. It does NOT add "m" for you; // integers/floats remain core-based unless the input already used "m". -func normalizeCPUText(v any) (string, error) { +func normalizeToCPUText(v any) (string, error) { switch x := v.(type) { case nil: return "", nil // absent; caller decides how to treat @@ -100,7 +100,7 @@ func cpuTextToMillicores(s string) (int64, error) { // getCanonicalCPU parses ResourceConfig.CPU on demand and returns millicores. // This is the single entry-point your higher-level code should call. func (r *ResourceConfig) getCanonicalCPU() (int64, error) { - text, err := normalizeCPUText(r.CPU) + text, err := normalizeToCPUText(r.CPU) if err != nil { return 0, err } @@ -123,7 +123,7 @@ func (r *ResourceConfig) getCanonicalCPU() (int64, error) { // "1.5" -> "1.5" // 2 -> "2" // 1.25 -> "1.25" -func normalizeMemoryText(v any) (string, error) { +func normalizeToMemoryText(v any) (string, error) { switch x := v.(type) { case nil: return "", nil @@ -238,7 +238,7 @@ func memoryTextToMi(s string) (int64, error) { // getCanonicalMemory parses ResourceConfig.Memory on demand and returns MiB. func (r *ResourceConfig) getCanonicalMemory() (int64, error) { - text, err := normalizeMemoryText(r.Memory) + text, err := normalizeToMemoryText(r.Memory) if err != nil { return 0, err } diff --git a/internal/config/resources_test.go b/internal/config/resources_test.go index 0846ac8..d311819 100644 --- a/internal/config/resources_test.go +++ b/internal/config/resources_test.go @@ -12,7 +12,7 @@ import ( // -------------------- CPU -------------------- // -func Test_normalizeCPUText(t *testing.T) { +func Test_normalizeToCPUText(t *testing.T) { t.Parallel() tests := []struct { @@ -51,7 +51,7 @@ func Test_normalizeCPUText(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - got, err := normalizeCPUText(tc.in) + got, err := normalizeToCPUText(tc.in) if tc.ok { require.NoError(t, err) assert.Equal(t, tc.want, got) @@ -135,7 +135,7 @@ func Test_getCanonicalCPU_Integration(t *testing.T) { // -------------------- Memory -------------------- // -func Test_normalizeMemoryText(t *testing.T) { +func Test_normalizeToMemoryText(t *testing.T) { t.Parallel() tests := []struct { @@ -172,7 +172,7 @@ func Test_normalizeMemoryText(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - got, err := normalizeMemoryText(tc.in) + got, err := normalizeToMemoryText(tc.in) if tc.ok { require.NoError(t, err) assert.Equal(t, tc.want, got) From 8b68c5bf2da90a0938c6cbdfc941ec9f22634cfb Mon Sep 17 00:00:00 2001 From: Max Gagnon <82063767+maxwellgagnon@users.noreply.github.com> Date: Sun, 26 Oct 2025 16:04:26 -0700 Subject: [PATCH 10/14] Add doc strings to two functions --- internal/config/resources.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/internal/config/resources.go b/internal/config/resources.go index 5ac2250..ed26c70 100644 --- a/internal/config/resources.go +++ b/internal/config/resources.go @@ -246,7 +246,10 @@ func (r *ResourceConfig) getCanonicalMemory() (int64, error) { } // ---------------- helpers ---------------- - +// bytesToMi converts a size in decimal bytes to mebibytes (MiB) and rounds to +// the nearest int64 MiB. It rejects negative, NaN, or infinite inputs and +// returns an error in those cases. If the converted MiB value would exceed +// math.MaxInt64, it returns an overflow error. func bytesToMi(bytes float64) (int64, error) { if bytes < 0 || math.IsNaN(bytes) || math.IsInf(bytes, 0) { return 0, fmt.Errorf("memory must be >= 0") @@ -258,6 +261,9 @@ func bytesToMi(bytes float64) (int64, error) { return roundFloatToInt64(miFloat) } +// roundFloatToInt64 rounds v to the nearest int64 using standard rounding. +// NaN or infinite values return an error. Extremely small non-zero magnitudes +// that round to 0 (|v| < 0.5) are treated as 0 without error. func roundFloatToInt64(v float64) (int64, error) { if math.IsNaN(v) || math.IsInf(v, 0) { return 0, fmt.Errorf("invalid number") From df9b6bcb919fa1deda60db5b1218c0fca44e12e8 Mon Sep 17 00:00:00 2001 From: Max Gagnon <82063767+maxwellgagnon@users.noreply.github.com> Date: Sun, 26 Oct 2025 16:07:42 -0700 Subject: [PATCH 11/14] Update docstring to be more Go-like --- internal/config/resources.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/config/resources.go b/internal/config/resources.go index ed26c70..8405a99 100644 --- a/internal/config/resources.go +++ b/internal/config/resources.go @@ -276,7 +276,9 @@ func roundFloatToInt64(v float64) (int64, error) { return rounded, nil } -// Case-insensitive suffix check. +// hasSuffixFold reports whether s ends with suf, case-insensitively. +// It uses strings.EqualFold, so comparisons are Unicode case-folded +// (locale-independent) but not Unicode-normalized. func hasSuffixFold(s, suf string) bool { return len(s) >= len(suf) && strings.EqualFold(s[len(s)-len(suf):], suf) } From 693f1095df28185646c1d31b641d6c04b81deb6a Mon Sep 17 00:00:00 2001 From: Max Gagnon <82063767+maxwellgagnon@users.noreply.github.com> Date: Sun, 26 Oct 2025 16:09:47 -0700 Subject: [PATCH 12/14] Improve error message clarity --- internal/config/resources.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/config/resources.go b/internal/config/resources.go index 8405a99..cb1f79a 100644 --- a/internal/config/resources.go +++ b/internal/config/resources.go @@ -252,7 +252,7 @@ func (r *ResourceConfig) getCanonicalMemory() (int64, error) { // math.MaxInt64, it returns an overflow error. func bytesToMi(bytes float64) (int64, error) { if bytes < 0 || math.IsNaN(bytes) || math.IsInf(bytes, 0) { - return 0, fmt.Errorf("memory must be >= 0") + return 0, fmt.Errorf("memory must be a valid, finite number >= 0.0 (not NaN or +/-Inf); got %g", bytes) } miFloat := bytes / (1024.0 * 1024.0) if miFloat > float64(math.MaxInt64) { From 3aa9aa600a6ede9899a3bbf8d5d39b87f9741ea9 Mon Sep 17 00:00:00 2001 From: Max Gagnon <82063767+maxwellgagnon@users.noreply.github.com> Date: Sun, 26 Oct 2025 16:16:14 -0700 Subject: [PATCH 13/14] Update doc strings to improve clarity --- internal/config/types.go | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/internal/config/types.go b/internal/config/types.go index b0c7cfa..1b2c7e5 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -154,8 +154,13 @@ func (c *DevEnvConfig) GPU() int { return c.Resources.GPU } -// CPU returns the canonical CPU quantity formatted for Kubernetes (e.g., "2500m"). -// Returns "0" if CPU <= 0 so callers can omit the field or treat as no request. +// CPU returns the canonical Kubernetes CPU quantity for this config as a +// millicore-formatted string (e.g., "2500m"). The value is computed on demand +// by parsing/normalizing the raw CPU input (e.g., "2", "2.5", "500m", 3) +// via getCanonicalCPU(), which yields a count of millicores. If normalization +// fails or the resulting value is non-positive, CPU returns "0" so callers +// can omit the field or treat it as no explicit CPU request in generated +// manifests. func (c *DevEnvConfig) CPU() string { CPU_in_millicores, err := c.Resources.getCanonicalCPU() if err != nil || CPU_in_millicores <= 0 { @@ -164,7 +169,13 @@ func (c *DevEnvConfig) CPU() string { return fmt.Sprintf("%dm", CPU_in_millicores) } -// Memory returns "Gi" or "Mi" ("" means omit). +// Memory returns the canonical Kubernetes memory quantity for this config, +// choosing "Gi" when the normalized value is an exact Gi multiple and "Mi" +// otherwise. The value is computed on demand by parsing/normalizing the raw +// memory input (e.g., "16Gi", "512Mi", "500M", 1.5) via getCanonicalMemory(), +// which yields a count of mebibytes (Mi). If normalization fails or the +// resulting value is non-positive, Memory returns the empty string so callers +// can omit the field in generated manifests. func (c *DevEnvConfig) Memory() string { memory_in_Mi, err := c.Resources.getCanonicalMemory() if err != nil || memory_in_Mi <= 0 { From 45f167419816a73c9fcf10dcbb5b46c5dcf38384 Mon Sep 17 00:00:00 2001 From: Max Gagnon <82063767+maxwellgagnon@users.noreply.github.com> Date: Sun, 26 Oct 2025 16:18:51 -0700 Subject: [PATCH 14/14] Remove unreachable return statement --- internal/config/example_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/config/example_test.go b/internal/config/example_test.go index 68501bd..9d2e607 100644 --- a/internal/config/example_test.go +++ b/internal/config/example_test.go @@ -26,7 +26,6 @@ func ExampleLoadDeveloperConfig() { sshKeys, err := cfg.GetSSHKeys() if err != nil { log.Fatal(err) - return } fmt.Printf("SSH Keys: %d configured\n", len(sshKeys))