From 47d3294930c231d1de92af71c29f4872e897a196 Mon Sep 17 00:00:00 2001 From: spapa013 Date: Wed, 11 Mar 2026 23:22:18 +0000 Subject: [PATCH 01/12] fix(config): require hostname for HTTP ingress rendering Align config validation with the ingress manifests that generation can produce. The original validation path did not fully enforce the conditions needed to render ingress safely. This change centralizes HTTP/hostname checks, tightens auth validation, and updates tests and fixtures to reflect the actual supported config combinations. - add helpers for HTTP exposure and hostname presence - require hostName when httpPort is set - reject enableAuth combined with skipAuth - require authURL and authSignIn when auth is enabled - expand validation tests for ingress and auth dependencies - update the ingress golden fixture to use a valid hostname --- internal/config/types.go | 15 ++++ internal/config/validation.go | 16 +++++ internal/config/validation_test.go | 70 +++++++++++++++++++ .../templates/testdata/golden/ingress.yaml | 4 +- 4 files changed, 103 insertions(+), 2 deletions(-) diff --git a/internal/config/types.go b/internal/config/types.go index 7dbd14e..6bea234 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -222,6 +222,21 @@ func (c *DevEnvConfig) NodePort() int { return c.SSHPort } +// HasHTTPPort reports whether HTTP exposure is configured. +func (c *DevEnvConfig) HasHTTPPort() bool { + return c != nil && c.HTTPPort != 0 +} + +// HasHostName reports whether a non-empty ingress hostname is configured. +func (c *DevEnvConfig) HasHostName() bool { + return c != nil && strings.TrimSpace(c.HostName) != "" +} + +// ShouldRenderIngress reports whether ingress can be safely rendered. +func (c *DevEnvConfig) ShouldRenderIngress() bool { + return c.HasHTTPPort() && c.HasHostName() +} + // VolumeMounts returns the configured volume mount specifications. // Returns the slice of VolumeMount configurations for binding local directories // into the developer environment container. diff --git a/internal/config/validation.go b/internal/config/validation.go index cabf662..4d051f5 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -250,6 +250,22 @@ func ValidateDevEnvConfig(config *DevEnvConfig) error { return fmt.Errorf("gpu must be >= 0") } + if config.HasHTTPPort() && !config.HasHostName() { + return fmt.Errorf("hostName is required when httpPort is set") + } + + if config.EnableAuth && config.SkipAuth { + return fmt.Errorf("enableAuth and skipAuth cannot both be true") + } + + if config.EnableAuth && strings.TrimSpace(config.AuthURL) == "" { + return fmt.Errorf("authURL is required when enableAuth is true") + } + + if config.EnableAuth && strings.TrimSpace(config.AuthSignIn) == "" { + return fmt.Errorf("authSignIn is required when enableAuth is true") + } + return nil } diff --git a/internal/config/validation_test.go b/internal/config/validation_test.go index 08d5a4f..1a5f428 100644 --- a/internal/config/validation_test.go +++ b/internal/config/validation_test.go @@ -347,3 +347,73 @@ func TestValidateDevEnvConfig_VolumeMountPaths(t *testing.T) { assert.Contains(t, err.Error(), "ContainerPath") }) } + +func TestValidateDevEnvConfig_IngressDependencies(t *testing.T) { + newCfg := func() *DevEnvConfig { + return &DevEnvConfig{ + Name: "alice", + BaseConfig: BaseConfig{ + SSHPublicKey: "ssh-ed25519 AAAAB3NzaC1lZDI1NTE5AAAA user@host", + }, + } + } + + t.Run("requires hostName when httpPort is set", func(t *testing.T) { + cfg := newCfg() + cfg.HTTPPort = 8080 + + err := ValidateDevEnvConfig(cfg) + require.Error(t, err) + assert.Contains(t, err.Error(), "hostName is required when httpPort is set") + }) + + t.Run("allows httpPort with hostName", func(t *testing.T) { + cfg := newCfg() + cfg.HTTPPort = 8080 + cfg.HostName = "devenv.example.com" + + require.NoError(t, ValidateDevEnvConfig(cfg)) + }) + + t.Run("requires authURL when enableAuth is true", func(t *testing.T) { + cfg := newCfg() + cfg.EnableAuth = true + cfg.AuthSignIn = "https://auth.example.com/start" + + err := ValidateDevEnvConfig(cfg) + require.Error(t, err) + assert.Contains(t, err.Error(), "authURL is required when enableAuth is true") + }) + + t.Run("requires authSignIn when enableAuth is true", func(t *testing.T) { + cfg := newCfg() + cfg.EnableAuth = true + cfg.AuthURL = "https://auth.example.com/auth" + + err := ValidateDevEnvConfig(cfg) + require.Error(t, err) + assert.Contains(t, err.Error(), "authSignIn is required when enableAuth is true") + }) + + t.Run("allows enableAuth with auth URLs", func(t *testing.T) { + cfg := newCfg() + cfg.EnableAuth = true + cfg.AuthURL = "https://auth.example.com/auth" + cfg.AuthSignIn = "https://auth.example.com/start" + + require.NoError(t, ValidateDevEnvConfig(cfg)) + }) + + t.Run("rejects enableAuth with skipAuth", func(t *testing.T) { + cfg := newCfg() + cfg.EnableAuth = true + cfg.SkipAuth = true + cfg.AuthURL = "https://auth.example.com/auth" + cfg.AuthSignIn = "https://auth.example.com/start" + + err := ValidateDevEnvConfig(cfg) + require.Error(t, err) + assert.Contains(t, err.Error(), "enableAuth and skipAuth cannot both be true") + }) + +} diff --git a/internal/templates/testdata/golden/ingress.yaml b/internal/templates/testdata/golden/ingress.yaml index 4d0c5ba..d8b00e6 100644 --- a/internal/templates/testdata/golden/ingress.yaml +++ b/internal/templates/testdata/golden/ingress.yaml @@ -10,7 +10,7 @@ metadata: spec: ingressClassName: nginx rules: - - host: testuser. + - host: testuser.devenv.example.com http: paths: - path: / @@ -22,5 +22,5 @@ spec: name: http tls: - hosts: - - "*." + - "*.devenv.example.com" secretName: http-testuser-tls From b32dc7dcfe5676d0160f00822eae89b576da1dba Mon Sep 17 00:00:00 2001 From: spapa013 Date: Wed, 11 Mar 2026 23:23:17 +0000 Subject: [PATCH 02/12] refactor(templates): introduce explicit render plans Extract template selection and ownership into an explicit RenderPlan. The original renderer implicitly owned the template sets for dev and system generation. This change moves that planning logic into a separate module so template selection is explicit, testable, and reusable by the rest of the generation pipeline. - add RenderPlan with TemplateNames and ManagedTemplates - add BuildDevRenderPlan and BuildSystemRenderPlan - define dev/system base and optional template sets outside the renderer - add contract tests for dev and system plans - add invariants to ensure all planned templates are managed - cover ingress inclusion and exclusion through plan tests --- internal/templates/plan.go | 41 +++++++++++++ internal/templates/plan_test.go | 103 ++++++++++++++++++++++++++++++++ 2 files changed, 144 insertions(+) create mode 100644 internal/templates/plan.go create mode 100644 internal/templates/plan_test.go diff --git a/internal/templates/plan.go b/internal/templates/plan.go new file mode 100644 index 0000000..b426935 --- /dev/null +++ b/internal/templates/plan.go @@ -0,0 +1,41 @@ +package templates + +import "github.com/nauticalab/devenv-engine/internal/config" + +var devBaseTemplates = []string{"statefulset", "service", "env-vars", "startup-scripts"} + +var devOptionalTemplates = []string{"ingress"} + +var devManagedTemplates = append(append([]string{}, devBaseTemplates...), devOptionalTemplates...) + +var systemBaseTemplates = []string{"namespace"} + +var systemManagedTemplates = append([]string{}, systemBaseTemplates...) + +// RenderPlan defines template selection and ownership. +type RenderPlan struct { + TemplateNames []string + ManagedTemplates []string +} + +// BuildDevRenderPlan computes the template set from config before rendering. +func BuildDevRenderPlan(cfg *config.DevEnvConfig) RenderPlan { + templateNames := append([]string{}, devBaseTemplates...) + + if cfg != nil && cfg.ShouldRenderIngress() { + templateNames = append(templateNames, "ingress") + } + + return RenderPlan{ + TemplateNames: templateNames, + ManagedTemplates: append([]string{}, devManagedTemplates...), + } +} + +// BuildSystemRenderPlan computes the template set for system-level manifests. +func BuildSystemRenderPlan() RenderPlan { + return RenderPlan{ + TemplateNames: append([]string{}, systemBaseTemplates...), + ManagedTemplates: append([]string{}, systemManagedTemplates...), + } +} diff --git a/internal/templates/plan_test.go b/internal/templates/plan_test.go new file mode 100644 index 0000000..9338c4c --- /dev/null +++ b/internal/templates/plan_test.go @@ -0,0 +1,103 @@ +package templates + +import ( + "testing" + + "github.com/nauticalab/devenv-engine/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestBuildDevRenderPlan(t *testing.T) { + t.Run("excludes ingress when HTTP port is unset", func(t *testing.T) { + cfg := &config.DevEnvConfig{} + + plan := BuildDevRenderPlan(cfg) + + assert.Equal(t, expectedDevTemplateNames(false), plan.TemplateNames) + assert.Equal(t, copyTemplateNames(devManagedTemplates), plan.ManagedTemplates) + }) + + t.Run("includes ingress when HTTP port is set", func(t *testing.T) { + cfg := &config.DevEnvConfig{HTTPPort: 8080, BaseConfig: config.BaseConfig{HostName: "devenv.example.com"}} + + plan := BuildDevRenderPlan(cfg) + + assert.Equal(t, expectedDevTemplateNames(true), plan.TemplateNames) + assert.Equal(t, copyTemplateNames(devManagedTemplates), plan.ManagedTemplates) + }) + + t.Run("excludes ingress when hostName is missing", func(t *testing.T) { + cfg := &config.DevEnvConfig{HTTPPort: 8080} + + plan := BuildDevRenderPlan(cfg) + + assert.Equal(t, expectedDevTemplateNames(false), plan.TemplateNames) + assert.Equal(t, copyTemplateNames(devManagedTemplates), plan.ManagedTemplates) + }) +} + +func TestBuildDevRenderPlan_Contract(t *testing.T) { + t.Run("http disabled", func(t *testing.T) { + plan := BuildDevRenderPlan(&config.DevEnvConfig{}) + assert.Equal(t, []string{"statefulset", "service", "env-vars", "startup-scripts"}, plan.TemplateNames) + assert.Equal(t, []string{"statefulset", "service", "env-vars", "startup-scripts", "ingress"}, plan.ManagedTemplates) + }) + + t.Run("http enabled", func(t *testing.T) { + plan := BuildDevRenderPlan(&config.DevEnvConfig{HTTPPort: 8080, BaseConfig: config.BaseConfig{HostName: "devenv.example.com"}}) + assert.Equal(t, []string{"statefulset", "service", "env-vars", "startup-scripts", "ingress"}, plan.TemplateNames) + assert.Equal(t, []string{"statefulset", "service", "env-vars", "startup-scripts", "ingress"}, plan.ManagedTemplates) + }) +} + +func TestBuildSystemRenderPlan(t *testing.T) { + plan := BuildSystemRenderPlan() + + assert.Equal(t, copyTemplateNames(systemBaseTemplates), plan.TemplateNames) + assert.Equal(t, copyTemplateNames(systemManagedTemplates), plan.ManagedTemplates) +} + +func TestBuildSystemRenderPlan_Contract(t *testing.T) { + plan := BuildSystemRenderPlan() + assert.Equal(t, []string{"namespace"}, plan.TemplateNames) + assert.Equal(t, []string{"namespace"}, plan.ManagedTemplates) +} + +func TestRenderPlans_TargetTemplatesAreManaged(t *testing.T) { + t.Run("dev plan", func(t *testing.T) { + plan := BuildDevRenderPlan(&config.DevEnvConfig{HTTPPort: 8080, BaseConfig: config.BaseConfig{HostName: "devenv.example.com"}}) + requireTargetSubsetOfManaged(t, plan.TemplateNames, plan.ManagedTemplates) + }) + + t.Run("system plan", func(t *testing.T) { + plan := BuildSystemRenderPlan() + requireTargetSubsetOfManaged(t, plan.TemplateNames, plan.ManagedTemplates) + }) +} + +func requireTargetSubsetOfManaged(t *testing.T, targetTemplates []string, managedTemplates []string) { + t.Helper() + + managedSet := make(map[string]struct{}, len(managedTemplates)) + for _, templateName := range managedTemplates { + managedSet[templateName] = struct{}{} + } + + for _, templateName := range targetTemplates { + _, ok := managedSet[templateName] + require.Truef(t, ok, "target template %q is not managed", templateName) + } +} + +func expectedDevTemplateNames(includeOptional bool) []string { + templateNames := copyTemplateNames(devBaseTemplates) + if includeOptional { + templateNames = append(templateNames, devOptionalTemplates...) + } + return templateNames +} + +func copyTemplateNames(templateNames []string) []string { + return append([]string{}, templateNames...) +} From b0078180fe4c715f63db848b9f8ba4313fa5cab7 Mon Sep 17 00:00:00 2001 From: spapa013 Date: Wed, 11 Mar 2026 23:28:38 +0000 Subject: [PATCH 03/12] refactor(templates): introduce post-render cleanup flow Introduce an explicit post-render phase for manifest output handling. This change adds a dedicated post-render step that manages generated outputs after a successful render, including cleanup of stale managed files from previous runs that are no longer planned. - add RunPostRender for post-render output handling - add cleanup behavior for managed outputs that are no longer planned - add post-render tests for managed, planned, and unmanaged outputs - keep renderer tests focused on rendering behavior and failure paths - make the invalid output directory test deterministic - document the dev/system renderer wrappers as convenience helpers --- internal/templates/post_render.go | 54 ++++++++++ internal/templates/post_render_test.go | 81 +++++++++++++++ internal/templates/renderer.go | 33 +++--- internal/templates/renderer_test.go | 133 +++++++++++++++++++------ 4 files changed, 253 insertions(+), 48 deletions(-) create mode 100644 internal/templates/post_render.go create mode 100644 internal/templates/post_render_test.go diff --git a/internal/templates/post_render.go b/internal/templates/post_render.go new file mode 100644 index 0000000..a691ae4 --- /dev/null +++ b/internal/templates/post_render.go @@ -0,0 +1,54 @@ +package templates + +import ( + "fmt" + "os" + "path/filepath" +) + +type PostRenderOptions struct { + CleanupUnplanned bool +} + +func NewPostRenderOptions(cleanupUnplanned bool) PostRenderOptions { + return PostRenderOptions{CleanupUnplanned: cleanupUnplanned} +} + +// RunPostRender executes post-render steps for a completed render pass. +func RunPostRender(outputDir string, plan RenderPlan, opts PostRenderOptions) error { + if opts.CleanupUnplanned { + if err := runPostRenderCleanup(outputDir, plan); err != nil { + return fmt.Errorf("cleanup unplanned outputs: %w", err) + } + } + + return nil +} + +// runPostRenderCleanup removes managed outputs that are no longer planned. +func runPostRenderCleanup(outputDir string, plan RenderPlan) error { + planned := make(map[string]struct{}, len(plan.TemplateNames)) + for _, templateName := range plan.TemplateNames { + planned[templateName] = struct{}{} + } + + for _, templateName := range plan.ManagedTemplates { + if _, ok := planned[templateName]; ok { + continue + } + if err := removeOutput(outputDir, templateName); err != nil { + return fmt.Errorf("failed to remove output for template %s: %w", templateName, err) + } + } + + return nil +} + +func removeOutput(outputDir string, templateName string) error { + outputPath := filepath.Join(outputDir, fmt.Sprintf("%s.yaml", templateName)) + err := os.Remove(outputPath) + if err != nil && !os.IsNotExist(err) { + return err + } + return nil +} diff --git a/internal/templates/post_render_test.go b/internal/templates/post_render_test.go new file mode 100644 index 0000000..2055240 --- /dev/null +++ b/internal/templates/post_render_test.go @@ -0,0 +1,81 @@ +package templates + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRunPostRender_RemovesUnplannedManagedOutputs(t *testing.T) { + tempDir := t.TempDir() + staleIngress := filepath.Join(tempDir, "ingress.yaml") + require.NoError(t, os.WriteFile(staleIngress, []byte("stale"), 0o644)) + + plan := RenderPlan{ + TemplateNames: []string{"statefulset", "service", "env-vars", "startup-scripts"}, + ManagedTemplates: []string{"statefulset", "service", "env-vars", "startup-scripts", "ingress"}, + } + + err := RunPostRender(tempDir, plan, NewPostRenderOptions(true)) + require.NoError(t, err) + + _, err = os.Stat(staleIngress) + assert.ErrorIs(t, err, os.ErrNotExist) +} + +func TestRunPostRender_PreservesPlannedOutputs(t *testing.T) { + tempDir := t.TempDir() + plannedIngress := filepath.Join(tempDir, "ingress.yaml") + require.NoError(t, os.WriteFile(plannedIngress, []byte("planned"), 0o644)) + + plan := RenderPlan{ + TemplateNames: []string{"statefulset", "service", "env-vars", "startup-scripts", "ingress"}, + ManagedTemplates: []string{"statefulset", "service", "env-vars", "startup-scripts", "ingress"}, + } + + err := RunPostRender(tempDir, plan, NewPostRenderOptions(true)) + require.NoError(t, err) + + content, err := os.ReadFile(plannedIngress) + require.NoError(t, err) + assert.Equal(t, "planned", string(content)) +} + +func TestRunPostRender_IgnoresUnmanagedOutputs(t *testing.T) { + tempDir := t.TempDir() + unmanagedFile := filepath.Join(tempDir, "notes.yaml") + require.NoError(t, os.WriteFile(unmanagedFile, []byte("keep"), 0o644)) + + plan := RenderPlan{ + TemplateNames: []string{"namespace"}, + ManagedTemplates: []string{"namespace"}, + } + + err := RunPostRender(tempDir, plan, NewPostRenderOptions(true)) + require.NoError(t, err) + + content, err := os.ReadFile(unmanagedFile) + require.NoError(t, err) + assert.Equal(t, "keep", string(content)) +} + +func TestRunPostRender_PreservesStaleOutputsWhenCleanupDisabled(t *testing.T) { + tempDir := t.TempDir() + staleIngress := filepath.Join(tempDir, "ingress.yaml") + require.NoError(t, os.WriteFile(staleIngress, []byte("stale"), 0o644)) + + plan := RenderPlan{ + TemplateNames: []string{"statefulset", "service", "env-vars", "startup-scripts"}, + ManagedTemplates: []string{"statefulset", "service", "env-vars", "startup-scripts", "ingress"}, + } + + err := RunPostRender(tempDir, plan, PostRenderOptions{CleanupUnplanned: false}) + require.NoError(t, err) + + content, err := os.ReadFile(staleIngress) + require.NoError(t, err) + assert.Equal(t, "stale", string(content)) +} diff --git a/internal/templates/renderer.go b/internal/templates/renderer.go index 665b9be..b5cef60 100644 --- a/internal/templates/renderer.go +++ b/internal/templates/renderer.go @@ -12,11 +12,6 @@ import ( "github.com/nauticalab/devenv-engine/internal/config" ) -var devTemplatesToRender = []string{"statefulset", "service", "env-vars", - "startup-scripts", "ingress"} - -var systemTemplatesToRender = []string{"namespace"} - // Embed all devTemplates and scripts at compile time // //go:embed template_files @@ -27,22 +22,29 @@ type Renderer[T config.BaseConfig | config.DevEnvConfig] struct { outputDir string templateRoot string targetTemplates []string + config *T } -// NewRenderer creates a new template renderer -func NewDevRenderer(outputDir string) *Renderer[config.DevEnvConfig] { - return NewRendererWithFS[config.DevEnvConfig](outputDir, "template_files/dev", devTemplatesToRender) +// NewDevRenderer is a convenience wrapper for dev-specific render tests and +// direct callers. If the codebase standardizes on GenerationSpec + NewRenderer, +// consider removing this wrapper. +func NewDevRenderer(outputDir string, cfg *config.DevEnvConfig, templateNames []string) *Renderer[config.DevEnvConfig] { + return NewRenderer[config.DevEnvConfig](outputDir, "template_files/dev", templateNames, cfg) } -func NewSystemRenderer(outputDir string) *Renderer[config.BaseConfig] { - return NewRendererWithFS[config.BaseConfig](outputDir, "template_files/system", systemTemplatesToRender) +// NewSystemRenderer is a convenience wrapper for system-specific direct callers. +// If the codebase standardizes on GenerationSpec + NewRenderer, consider +// removing this wrapper. +func NewSystemRenderer(outputDir string, cfg *config.BaseConfig, templateNames []string) *Renderer[config.BaseConfig] { + return NewRenderer[config.BaseConfig](outputDir, "template_files/system", templateNames, cfg) } -func NewRendererWithFS[T config.BaseConfig | config.DevEnvConfig](outputDir string, templateRoot string, targetTemplates []string) *Renderer[T] { +func NewRenderer[T config.BaseConfig | config.DevEnvConfig](outputDir string, templateRoot string, targetTemplates []string, cfg *T) *Renderer[T] { return &Renderer[T]{ outputDir: outputDir, templateRoot: templateRoot, targetTemplates: targetTemplates, + config: cfg, } } @@ -85,7 +87,7 @@ func templateFuncs(templateRoot string) template.FuncMap { } } -func (r *Renderer[T]) RenderTemplate(templateName string, config *T) error { +func (r *Renderer[T]) RenderTemplate(templateName string) error { // Get the template content from embedded files templateContent, err := templates.ReadFile(filepath.Join(r.templateRoot, fmt.Sprintf("manifests/%s.tmpl", templateName))) if err != nil { @@ -115,7 +117,7 @@ func (r *Renderer[T]) RenderTemplate(templateName string, config *T) error { defer outputFile.Close() // Execute template with DevEnvConfig - simple and clean! - if err := tmpl.Execute(outputFile, config); err != nil { + if err := tmpl.Execute(outputFile, r.config); err != nil { return fmt.Errorf("failed to render template %s: %w", templateName, err) } @@ -123,11 +125,12 @@ func (r *Renderer[T]) RenderTemplate(templateName string, config *T) error { return nil } -func (r *Renderer[T]) RenderAll(config *T) error { +func (r *Renderer[T]) RenderAll() error { for _, templateName := range r.targetTemplates { - if err := r.RenderTemplate(templateName, config); err != nil { + if err := r.RenderTemplate(templateName); err != nil { return fmt.Errorf("failed to render template %s: %w", templateName, err) } } + return nil } diff --git a/internal/templates/renderer_test.go b/internal/templates/renderer_test.go index 2d9053c..3c1c055 100644 --- a/internal/templates/renderer_test.go +++ b/internal/templates/renderer_test.go @@ -24,9 +24,10 @@ func TestRenderTemplate(t *testing.T) { "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQC7... testuser@example.com", "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAI... testuser2@example.com", }, - UID: 2000, - Image: "ubuntu:22.04", + UID: 2000, + Image: "ubuntu:22.04", Namespace: "devenv-test", + HostName: "devenv.example.com", Packages: config.PackageConfig{ Python: []string{"numpy", "pandas"}, APT: []string{"vim", "curl"}, @@ -66,10 +67,10 @@ func TestRenderTemplate(t *testing.T) { tempDir := t.TempDir() // Create renderer - renderer := NewDevRenderer(tempDir) + renderer := NewDevRenderer(tempDir, testConfig, BuildDevRenderPlan(testConfig).TemplateNames) // Render template - err := renderer.RenderTemplate(templateName, testConfig) + err := renderer.RenderTemplate(templateName) require.NoError(t, err, "Failed to render template %s", templateName) // Read the generated output @@ -105,36 +106,100 @@ func TestRenderTemplate(t *testing.T) { // TestRenderAll tests the RenderAll function that renders all templates func TestRenderAll(t *testing.T) { - // Create minimal test configuration - testConfig := &config.DevEnvConfig{ - Name: "minimal", - BaseConfig: config.BaseConfig{ - SSHPublicKey: "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQC7... minimal@example.com", - Namespace: "devenv-test", - }, - SSHPort: 30002, - } + t.Run("includes ingress when HTTP port is set", func(t *testing.T) { + testConfig := &config.DevEnvConfig{ + Name: "minimal", + BaseConfig: config.BaseConfig{ + SSHPublicKey: "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQC7... minimal@example.com", + Namespace: "devenv-test", + HostName: "devenv.example.com", + }, + SSHPort: 30002, + HTTPPort: 8080, + } + + tempDir := t.TempDir() + renderer := NewDevRenderer(tempDir, testConfig, BuildDevRenderPlan(testConfig).TemplateNames) + + err := renderer.RenderAll() + require.NoError(t, err, "RenderAll should not return error") + + expectedFiles := templateNamesToFiles(BuildDevRenderPlan(testConfig).TemplateNames) + + for _, filename := range expectedFiles { + filePath := filepath.Join(tempDir, filename) + _, err := os.Stat(filePath) + assert.NoError(t, err, "Expected file %s should exist", filename) + + content, err := os.ReadFile(filePath) + require.NoError(t, err) + assert.NotEmpty(t, content, "File %s should not be empty", filename) + } + }) + + t.Run("skips ingress when HTTP port is unset", func(t *testing.T) { + testConfig := &config.DevEnvConfig{ + Name: "minimal", + BaseConfig: config.BaseConfig{ + SSHPublicKey: "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQC7... minimal@example.com", + Namespace: "devenv-test", + }, + SSHPort: 30002, + } - tempDir := t.TempDir() - renderer := NewDevRenderer(tempDir) + tempDir := t.TempDir() + renderer := NewDevRenderer(tempDir, testConfig, BuildDevRenderPlan(testConfig).TemplateNames) + + err := renderer.RenderAll() + require.NoError(t, err, "RenderAll should not return error") - // Test RenderAll - err := renderer.RenderAll(testConfig) - require.NoError(t, err, "RenderAll should not return error") + expectedFiles := templateNamesToFiles(BuildDevRenderPlan(testConfig).TemplateNames) + for _, filename := range expectedFiles { + filePath := filepath.Join(tempDir, filename) + _, err := os.Stat(filePath) + assert.NoError(t, err, "Expected file %s should exist", filename) + } - // Verify all expected files were created - expectedFiles := []string{"statefulset.yaml", "service.yaml", "env-vars.yaml", "startup-scripts.yaml", "ingress.yaml"} + _, err = os.Stat(filepath.Join(tempDir, "ingress.yaml")) + assert.ErrorIs(t, err, os.ErrNotExist, "ingress.yaml should not be generated without HTTP port") + }) - for _, filename := range expectedFiles { - filePath := filepath.Join(tempDir, filename) - _, err := os.Stat(filePath) - assert.NoError(t, err, "Expected file %s should exist", filename) + t.Run("preserves stale ingress when render fails", func(t *testing.T) { + testConfig := &config.DevEnvConfig{ + Name: "minimal", + BaseConfig: config.BaseConfig{ + SSHPublicKey: "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQC7... minimal@example.com", + Namespace: "devenv-test", + }, + SSHPort: 30002, + } + + tempDir := t.TempDir() + staleIngress := filepath.Join(tempDir, "ingress.yaml") + require.NoError(t, os.WriteFile(staleIngress, []byte("stale"), 0o644)) + + renderer := NewRenderer[config.DevEnvConfig]( + tempDir, + "template_files/dev", + []string{"nonexistent-template"}, + testConfig, + ) + + err := renderer.RenderAll() + require.Error(t, err) + + content, readErr := os.ReadFile(staleIngress) + require.NoError(t, readErr) + assert.Equal(t, "stale", string(content), "stale ingress.yaml should be preserved when render fails") + }) +} - // Verify file is not empty - content, err := os.ReadFile(filePath) - require.NoError(t, err) - assert.NotEmpty(t, content, "File %s should not be empty", filename) +func templateNamesToFiles(templateNames []string) []string { + files := make([]string, 0, len(templateNames)) + for _, templateName := range templateNames { + files = append(files, templateName+".yaml") } + return files } // TestRenderTemplate_ErrorCases tests error handling in template rendering @@ -148,17 +213,19 @@ func TestRenderTemplate_ErrorCases(t *testing.T) { t.Run("invalid template name", func(t *testing.T) { tempDir := t.TempDir() - renderer := NewDevRenderer(tempDir) + renderer := NewDevRenderer(tempDir, testConfig, BuildDevRenderPlan(testConfig).TemplateNames) - err := renderer.RenderTemplate("nonexistent", testConfig) + err := renderer.RenderTemplate("nonexistent") assert.Error(t, err, "Should return error for invalid template") }) t.Run("invalid output directory", func(t *testing.T) { - // Use a path that can't be created (assuming /root is not writable in test) - renderer := NewDevRenderer("/root/impossible/path") + // Make the parent path a file so MkdirAll fails deterministically. + parentFile := filepath.Join(t.TempDir(), "not-a-directory") + require.NoError(t, os.WriteFile(parentFile, []byte("x"), 0o644)) + renderer := NewDevRenderer(filepath.Join(parentFile, "child"), testConfig, BuildDevRenderPlan(testConfig).TemplateNames) - err := renderer.RenderTemplate("configmap", testConfig) + err := renderer.RenderTemplate("env-vars") assert.Error(t, err, "Should return error for invalid output directory") }) } From dd874ac0c18dfa6dfd315fd7d738fca29302c41e Mon Sep 17 00:00:00 2001 From: spapa013 Date: Wed, 11 Mar 2026 23:29:33 +0000 Subject: [PATCH 04/12] refactor(generate): standardize manifest generation on GenerationSpec Refactor manifest generation around an explicit GenerationSpec. The original generate command assembled rendering inputs directly inside the system and developer generation paths. This change packages the generation contract into a typed spec, adds a shared generateManifests pipeline, and introduces a no-cleanup flag for post-render behavior. - add GenerationSpec with dev and system builders - add NewPostRenderOptions for explicit post-render policy creation - add --no-cleanup and map it into post-render behavior - standardize the CLI flow on spec -> render -> post-render - add a shared generateManifests helper for dev and system generation - switch the command path to the generic NewRenderer constructor --- cmd/devenv/generate.go | 34 +++++++++++++++++++-------- internal/templates/generation_spec.go | 31 ++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 10 deletions(-) create mode 100644 internal/templates/generation_spec.go diff --git a/cmd/devenv/generate.go b/cmd/devenv/generate.go index 2324141..c71d50b 100644 --- a/cmd/devenv/generate.go +++ b/cmd/devenv/generate.go @@ -31,6 +31,7 @@ var ( configDir string // Input directory for developer configs dryRun bool allDevs bool + noCleanup bool ) var generateCmd = &cobra.Command{ @@ -75,6 +76,7 @@ func init() { generateCmd.Flags().StringVar(&configDir, "config-dir", "./developers", "Directory containing developer configuration files") generateCmd.Flags().BoolVar(&dryRun, "dry-run", false, "Show what would be generated without creating files") generateCmd.Flags().BoolVar(&allDevs, "all-developers", false, "Generate manifests for all developers") + generateCmd.Flags().BoolVar(&noCleanup, "no-cleanup", false, "Preserve files from previous runs instead of removing unplanned outputs") } @@ -266,12 +268,11 @@ func generateSingleDeveloper(developerName string) { } func generateSystemManifests(cfg *config.BaseConfig, outputDir string) error { - // Create template renderer - renderer := templates.NewSystemRenderer(outputDir) + postRenderOpts := templates.NewPostRenderOptions(!noCleanup) + spec := templates.BuildSystemGenerationSpec(cfg, outputDir, postRenderOpts) - // Render all main templates - if err := renderer.RenderAll(cfg); err != nil { - return fmt.Errorf("failed to render templates: %w", err) + if err := generateManifests(spec); err != nil { + return err } fmt.Printf("🎉 Successfully generated system manifests\n") @@ -281,12 +282,11 @@ func generateSystemManifests(cfg *config.BaseConfig, outputDir string) error { // generateDeveloperManifests creates Kubernetes manifests for a developer func generateDeveloperManifests(cfg *config.DevEnvConfig, outputDir string) error { - // Create template renderer - renderer := templates.NewDevRenderer(outputDir) + postRenderOpts := templates.NewPostRenderOptions(!noCleanup) + spec := templates.BuildDevGenerationSpec(cfg, outputDir, postRenderOpts) - // Render all main templates - if err := renderer.RenderAll(cfg); err != nil { - return fmt.Errorf("failed to render templates: %w", err) + if err := generateManifests(spec); err != nil { + return err } fmt.Printf("🎉 Successfully generated manifests for %s\n", cfg.Name) @@ -294,6 +294,20 @@ func generateDeveloperManifests(cfg *config.DevEnvConfig, outputDir string) erro return nil } +func generateManifests[T config.BaseConfig | config.DevEnvConfig](spec templates.GenerationSpec[T]) error { + renderer := templates.NewRenderer(spec.OutputDir, spec.TemplateRoot, spec.Plan.TemplateNames, spec.Config) + + if err := renderer.RenderAll(); err != nil { + return fmt.Errorf("failed to render templates: %w", err) + } + + if err := templates.RunPostRender(spec.OutputDir, spec.Plan, spec.PostRenderOptions); err != nil { + return fmt.Errorf("failed to run post-render steps: %w", err) + } + + return nil +} + // Helper function to print config summary func printConfigSummary(cfg *config.DevEnvConfig) { fmt.Printf("\nConfiguration Summary:\n") diff --git a/internal/templates/generation_spec.go b/internal/templates/generation_spec.go new file mode 100644 index 0000000..3513a2f --- /dev/null +++ b/internal/templates/generation_spec.go @@ -0,0 +1,31 @@ +package templates + +import "github.com/nauticalab/devenv-engine/internal/config" + +type GenerationSpec[T config.BaseConfig | config.DevEnvConfig] struct { + Config *T + Plan RenderPlan + OutputDir string + TemplateRoot string + PostRenderOptions PostRenderOptions +} + +func BuildDevGenerationSpec(cfg *config.DevEnvConfig, outputDir string, postRenderOpts PostRenderOptions) GenerationSpec[config.DevEnvConfig] { + return GenerationSpec[config.DevEnvConfig]{ + Config: cfg, + Plan: BuildDevRenderPlan(cfg), + OutputDir: outputDir, + TemplateRoot: "template_files/dev", + PostRenderOptions: postRenderOpts, + } +} + +func BuildSystemGenerationSpec(cfg *config.BaseConfig, outputDir string, postRenderOpts PostRenderOptions) GenerationSpec[config.BaseConfig] { + return GenerationSpec[config.BaseConfig]{ + Config: cfg, + Plan: BuildSystemRenderPlan(), + OutputDir: outputDir, + TemplateRoot: "template_files/system", + PostRenderOptions: postRenderOpts, + } +} From 8a5af0a5eea8b3f8141b33b6299149b44b235929 Mon Sep 17 00:00:00 2001 From: spapa013 Date: Tue, 17 Mar 2026 21:04:27 +0000 Subject: [PATCH 05/12] refactor(templates): simplify render plans and template cleanup scopes - keep RenderPlan focused on TemplateNames only - build dev plans by filtering the full dev template set - add explicit cleanup-scope helpers for dev and system templates - make BuildDevRenderPlan return an error for nil config - update renderer/tests to use the new plan contract --- internal/templates/plan.go | 49 +++++++++++++---------- internal/templates/plan_test.go | 61 +++++++++++------------------ internal/templates/renderer.go | 5 +-- internal/templates/renderer_test.go | 34 ++++++++++------ 4 files changed, 74 insertions(+), 75 deletions(-) diff --git a/internal/templates/plan.go b/internal/templates/plan.go index b426935..e8ba985 100644 --- a/internal/templates/plan.go +++ b/internal/templates/plan.go @@ -1,41 +1,50 @@ package templates -import "github.com/nauticalab/devenv-engine/internal/config" +import ( + "fmt" -var devBaseTemplates = []string{"statefulset", "service", "env-vars", "startup-scripts"} + "github.com/nauticalab/devenv-engine/internal/config" +) -var devOptionalTemplates = []string{"ingress"} +var devTemplates = []string{"statefulset", "service", "env-vars", "startup-scripts", "ingress"} -var devManagedTemplates = append(append([]string{}, devBaseTemplates...), devOptionalTemplates...) +var systemTemplates = []string{"namespace"} -var systemBaseTemplates = []string{"namespace"} - -var systemManagedTemplates = append([]string{}, systemBaseTemplates...) - -// RenderPlan defines template selection and ownership. +// RenderPlan defines template selection for a render pass. type RenderPlan struct { - TemplateNames []string - ManagedTemplates []string + TemplateNames []string } // BuildDevRenderPlan computes the template set from config before rendering. -func BuildDevRenderPlan(cfg *config.DevEnvConfig) RenderPlan { - templateNames := append([]string{}, devBaseTemplates...) +func BuildDevRenderPlan(cfg *config.DevEnvConfig) (RenderPlan, error) { + if cfg == nil { + return RenderPlan{}, fmt.Errorf("BuildDevRenderPlan requires non-nil config") + } - if cfg != nil && cfg.ShouldRenderIngress() { - templateNames = append(templateNames, "ingress") + templateNames := make([]string, 0, len(devTemplates)) + for _, templateName := range devTemplates { + if templateName == "ingress" && !cfg.ShouldRenderIngress() { + continue + } + templateNames = append(templateNames, templateName) } return RenderPlan{ - TemplateNames: templateNames, - ManagedTemplates: append([]string{}, devManagedTemplates...), - } + TemplateNames: templateNames, + }, nil } // BuildSystemRenderPlan computes the template set for system-level manifests. func BuildSystemRenderPlan() RenderPlan { return RenderPlan{ - TemplateNames: append([]string{}, systemBaseTemplates...), - ManagedTemplates: append([]string{}, systemManagedTemplates...), + TemplateNames: append([]string{}, systemTemplates...), } } + +func DevCleanupScope() []string { + return append([]string{}, devTemplates...) +} + +func SystemCleanupScope() []string { + return append([]string{}, systemTemplates...) +} diff --git a/internal/templates/plan_test.go b/internal/templates/plan_test.go index 9338c4c..d2d676f 100644 --- a/internal/templates/plan_test.go +++ b/internal/templates/plan_test.go @@ -12,88 +12,71 @@ func TestBuildDevRenderPlan(t *testing.T) { t.Run("excludes ingress when HTTP port is unset", func(t *testing.T) { cfg := &config.DevEnvConfig{} - plan := BuildDevRenderPlan(cfg) + plan, err := BuildDevRenderPlan(cfg) + require.NoError(t, err) assert.Equal(t, expectedDevTemplateNames(false), plan.TemplateNames) - assert.Equal(t, copyTemplateNames(devManagedTemplates), plan.ManagedTemplates) }) t.Run("includes ingress when HTTP port is set", func(t *testing.T) { cfg := &config.DevEnvConfig{HTTPPort: 8080, BaseConfig: config.BaseConfig{HostName: "devenv.example.com"}} - plan := BuildDevRenderPlan(cfg) + plan, err := BuildDevRenderPlan(cfg) + require.NoError(t, err) assert.Equal(t, expectedDevTemplateNames(true), plan.TemplateNames) - assert.Equal(t, copyTemplateNames(devManagedTemplates), plan.ManagedTemplates) }) t.Run("excludes ingress when hostName is missing", func(t *testing.T) { cfg := &config.DevEnvConfig{HTTPPort: 8080} - plan := BuildDevRenderPlan(cfg) + plan, err := BuildDevRenderPlan(cfg) + require.NoError(t, err) assert.Equal(t, expectedDevTemplateNames(false), plan.TemplateNames) - assert.Equal(t, copyTemplateNames(devManagedTemplates), plan.ManagedTemplates) }) } +func TestBuildDevRenderPlan_NilConfigReturnsError(t *testing.T) { + _, err := BuildDevRenderPlan(nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "BuildDevRenderPlan requires non-nil config") +} + func TestBuildDevRenderPlan_Contract(t *testing.T) { t.Run("http disabled", func(t *testing.T) { - plan := BuildDevRenderPlan(&config.DevEnvConfig{}) + plan, err := BuildDevRenderPlan(&config.DevEnvConfig{}) + require.NoError(t, err) assert.Equal(t, []string{"statefulset", "service", "env-vars", "startup-scripts"}, plan.TemplateNames) - assert.Equal(t, []string{"statefulset", "service", "env-vars", "startup-scripts", "ingress"}, plan.ManagedTemplates) }) t.Run("http enabled", func(t *testing.T) { - plan := BuildDevRenderPlan(&config.DevEnvConfig{HTTPPort: 8080, BaseConfig: config.BaseConfig{HostName: "devenv.example.com"}}) + plan, err := BuildDevRenderPlan(&config.DevEnvConfig{HTTPPort: 8080, BaseConfig: config.BaseConfig{HostName: "devenv.example.com"}}) + require.NoError(t, err) assert.Equal(t, []string{"statefulset", "service", "env-vars", "startup-scripts", "ingress"}, plan.TemplateNames) - assert.Equal(t, []string{"statefulset", "service", "env-vars", "startup-scripts", "ingress"}, plan.ManagedTemplates) }) } func TestBuildSystemRenderPlan(t *testing.T) { plan := BuildSystemRenderPlan() - assert.Equal(t, copyTemplateNames(systemBaseTemplates), plan.TemplateNames) - assert.Equal(t, copyTemplateNames(systemManagedTemplates), plan.ManagedTemplates) + assert.Equal(t, copyTemplateNames(systemTemplates), plan.TemplateNames) } func TestBuildSystemRenderPlan_Contract(t *testing.T) { plan := BuildSystemRenderPlan() assert.Equal(t, []string{"namespace"}, plan.TemplateNames) - assert.Equal(t, []string{"namespace"}, plan.ManagedTemplates) } -func TestRenderPlans_TargetTemplatesAreManaged(t *testing.T) { - t.Run("dev plan", func(t *testing.T) { - plan := BuildDevRenderPlan(&config.DevEnvConfig{HTTPPort: 8080, BaseConfig: config.BaseConfig{HostName: "devenv.example.com"}}) - requireTargetSubsetOfManaged(t, plan.TemplateNames, plan.ManagedTemplates) - }) - - t.Run("system plan", func(t *testing.T) { - plan := BuildSystemRenderPlan() - requireTargetSubsetOfManaged(t, plan.TemplateNames, plan.ManagedTemplates) - }) -} - -func requireTargetSubsetOfManaged(t *testing.T, targetTemplates []string, managedTemplates []string) { - t.Helper() - - managedSet := make(map[string]struct{}, len(managedTemplates)) - for _, templateName := range managedTemplates { - managedSet[templateName] = struct{}{} - } - - for _, templateName := range targetTemplates { - _, ok := managedSet[templateName] - require.Truef(t, ok, "target template %q is not managed", templateName) - } +func TestTemplateScopes(t *testing.T) { + assert.Equal(t, []string{"statefulset", "service", "env-vars", "startup-scripts", "ingress"}, DevCleanupScope()) + assert.Equal(t, []string{"namespace"}, SystemCleanupScope()) } func expectedDevTemplateNames(includeOptional bool) []string { - templateNames := copyTemplateNames(devBaseTemplates) + templateNames := []string{"statefulset", "service", "env-vars", "startup-scripts"} if includeOptional { - templateNames = append(templateNames, devOptionalTemplates...) + templateNames = append(templateNames, "ingress") } return templateNames } diff --git a/internal/templates/renderer.go b/internal/templates/renderer.go index b5cef60..faee8b3 100644 --- a/internal/templates/renderer.go +++ b/internal/templates/renderer.go @@ -26,15 +26,12 @@ type Renderer[T config.BaseConfig | config.DevEnvConfig] struct { } // NewDevRenderer is a convenience wrapper for dev-specific render tests and -// direct callers. If the codebase standardizes on GenerationSpec + NewRenderer, -// consider removing this wrapper. +// direct callers. func NewDevRenderer(outputDir string, cfg *config.DevEnvConfig, templateNames []string) *Renderer[config.DevEnvConfig] { return NewRenderer[config.DevEnvConfig](outputDir, "template_files/dev", templateNames, cfg) } // NewSystemRenderer is a convenience wrapper for system-specific direct callers. -// If the codebase standardizes on GenerationSpec + NewRenderer, consider -// removing this wrapper. func NewSystemRenderer(outputDir string, cfg *config.BaseConfig, templateNames []string) *Renderer[config.BaseConfig] { return NewRenderer[config.BaseConfig](outputDir, "template_files/system", templateNames, cfg) } diff --git a/internal/templates/renderer_test.go b/internal/templates/renderer_test.go index 3c1c055..1f9dcca 100644 --- a/internal/templates/renderer_test.go +++ b/internal/templates/renderer_test.go @@ -67,10 +67,12 @@ func TestRenderTemplate(t *testing.T) { tempDir := t.TempDir() // Create renderer - renderer := NewDevRenderer(tempDir, testConfig, BuildDevRenderPlan(testConfig).TemplateNames) + plan, err := BuildDevRenderPlan(testConfig) + require.NoError(t, err) + renderer := NewDevRenderer(tempDir, testConfig, plan.TemplateNames) // Render template - err := renderer.RenderTemplate(templateName) + err = renderer.RenderTemplate(templateName) require.NoError(t, err, "Failed to render template %s", templateName) // Read the generated output @@ -119,12 +121,14 @@ func TestRenderAll(t *testing.T) { } tempDir := t.TempDir() - renderer := NewDevRenderer(tempDir, testConfig, BuildDevRenderPlan(testConfig).TemplateNames) + plan, err := BuildDevRenderPlan(testConfig) + require.NoError(t, err) + renderer := NewDevRenderer(tempDir, testConfig, plan.TemplateNames) - err := renderer.RenderAll() + err = renderer.RenderAll() require.NoError(t, err, "RenderAll should not return error") - expectedFiles := templateNamesToFiles(BuildDevRenderPlan(testConfig).TemplateNames) + expectedFiles := templateNamesToFiles(plan.TemplateNames) for _, filename := range expectedFiles { filePath := filepath.Join(tempDir, filename) @@ -148,12 +152,14 @@ func TestRenderAll(t *testing.T) { } tempDir := t.TempDir() - renderer := NewDevRenderer(tempDir, testConfig, BuildDevRenderPlan(testConfig).TemplateNames) + plan, err := BuildDevRenderPlan(testConfig) + require.NoError(t, err) + renderer := NewDevRenderer(tempDir, testConfig, plan.TemplateNames) - err := renderer.RenderAll() + err = renderer.RenderAll() require.NoError(t, err, "RenderAll should not return error") - expectedFiles := templateNamesToFiles(BuildDevRenderPlan(testConfig).TemplateNames) + expectedFiles := templateNamesToFiles(plan.TemplateNames) for _, filename := range expectedFiles { filePath := filepath.Join(tempDir, filename) _, err := os.Stat(filePath) @@ -213,9 +219,11 @@ func TestRenderTemplate_ErrorCases(t *testing.T) { t.Run("invalid template name", func(t *testing.T) { tempDir := t.TempDir() - renderer := NewDevRenderer(tempDir, testConfig, BuildDevRenderPlan(testConfig).TemplateNames) + plan, err := BuildDevRenderPlan(testConfig) + require.NoError(t, err) + renderer := NewDevRenderer(tempDir, testConfig, plan.TemplateNames) - err := renderer.RenderTemplate("nonexistent") + err = renderer.RenderTemplate("nonexistent") assert.Error(t, err, "Should return error for invalid template") }) @@ -223,9 +231,11 @@ func TestRenderTemplate_ErrorCases(t *testing.T) { // Make the parent path a file so MkdirAll fails deterministically. parentFile := filepath.Join(t.TempDir(), "not-a-directory") require.NoError(t, os.WriteFile(parentFile, []byte("x"), 0o644)) - renderer := NewDevRenderer(filepath.Join(parentFile, "child"), testConfig, BuildDevRenderPlan(testConfig).TemplateNames) + plan, err := BuildDevRenderPlan(testConfig) + require.NoError(t, err) + renderer := NewDevRenderer(filepath.Join(parentFile, "child"), testConfig, plan.TemplateNames) - err := renderer.RenderTemplate("env-vars") + err = renderer.RenderTemplate("env-vars") assert.Error(t, err, "Should return error for invalid output directory") }) } From b0da71f46c2f9e2c55ce5c9893648fe7f9032742 Mon Sep 17 00:00:00 2001 From: spapa013 Date: Tue, 17 Mar 2026 21:05:36 +0000 Subject: [PATCH 06/12] refactor(generate): move cleanup to pre-render manifest generation - move cleanup to a pre-render step gated by --no-cleanup - scope cleanup to known template outputs - restore explicit generateSystemManifests / generateDeveloperManifests flows use NewSystemRenderer and NewDevRenderer again instead of hardcoding generic render flow - keep post-render invocation as an explicit pipeline stage --- cmd/devenv/generate.go | 56 ++++++++++++++++++++++++++++-------------- 1 file changed, 37 insertions(+), 19 deletions(-) diff --git a/cmd/devenv/generate.go b/cmd/devenv/generate.go index c71d50b..37fcc9e 100644 --- a/cmd/devenv/generate.go +++ b/cmd/devenv/generate.go @@ -76,7 +76,7 @@ func init() { generateCmd.Flags().StringVar(&configDir, "config-dir", "./developers", "Directory containing developer configuration files") generateCmd.Flags().BoolVar(&dryRun, "dry-run", false, "Show what would be generated without creating files") generateCmd.Flags().BoolVar(&allDevs, "all-developers", false, "Generate manifests for all developers") - generateCmd.Flags().BoolVar(&noCleanup, "no-cleanup", false, "Preserve files from previous runs instead of removing unplanned outputs") + generateCmd.Flags().BoolVar(&noCleanup, "no-cleanup", false, "Preserve files from previous runs instead of deleting prior generated manifests before rendering") } @@ -268,43 +268,61 @@ func generateSingleDeveloper(developerName string) { } func generateSystemManifests(cfg *config.BaseConfig, outputDir string) error { - postRenderOpts := templates.NewPostRenderOptions(!noCleanup) - spec := templates.BuildSystemGenerationSpec(cfg, outputDir, postRenderOpts) + if !noCleanup { + if err := cleanupTemplateOutputs(outputDir, templates.SystemCleanupScope()); err != nil { + return fmt.Errorf("failed to clean output directory: %w", err) + } + } - if err := generateManifests(spec); err != nil { - return err + plan := templates.BuildSystemRenderPlan() + renderer := templates.NewSystemRenderer(outputDir, cfg, plan.TemplateNames) + + if err := renderer.RenderAll(); err != nil { + return fmt.Errorf("failed to render templates: %w", err) } - fmt.Printf("🎉 Successfully generated system manifests\n") + if err := templates.RunPostRender(outputDir, plan, templates.NewPostRenderOptions()); err != nil { + return fmt.Errorf("failed to run post-render steps: %w", err) + } + fmt.Printf("🎉 Successfully generated system manifests\n") return nil } // generateDeveloperManifests creates Kubernetes manifests for a developer func generateDeveloperManifests(cfg *config.DevEnvConfig, outputDir string) error { - postRenderOpts := templates.NewPostRenderOptions(!noCleanup) - spec := templates.BuildDevGenerationSpec(cfg, outputDir, postRenderOpts) - - if err := generateManifests(spec); err != nil { - return err + if !noCleanup { + if err := cleanupTemplateOutputs(outputDir, templates.DevCleanupScope()); err != nil { + return fmt.Errorf("failed to clean output directory: %w", err) + } } - fmt.Printf("🎉 Successfully generated manifests for %s\n", cfg.Name) - - return nil -} - -func generateManifests[T config.BaseConfig | config.DevEnvConfig](spec templates.GenerationSpec[T]) error { - renderer := templates.NewRenderer(spec.OutputDir, spec.TemplateRoot, spec.Plan.TemplateNames, spec.Config) + plan, err := templates.BuildDevRenderPlan(cfg) + if err != nil { + return fmt.Errorf("failed to build render plan: %w", err) + } + renderer := templates.NewDevRenderer(outputDir, cfg, plan.TemplateNames) if err := renderer.RenderAll(); err != nil { return fmt.Errorf("failed to render templates: %w", err) } - if err := templates.RunPostRender(spec.OutputDir, spec.Plan, spec.PostRenderOptions); err != nil { + if err := templates.RunPostRender(outputDir, plan, templates.NewPostRenderOptions()); err != nil { return fmt.Errorf("failed to run post-render steps: %w", err) } + fmt.Printf("🎉 Successfully generated manifests for %s\n", cfg.Name) + return nil +} + +func cleanupTemplateOutputs(outputDir string, templateNames []string) error { + for _, templateName := range templateNames { + outputPath := filepath.Join(outputDir, templateName+".yaml") + if err := os.Remove(outputPath); err != nil && !os.IsNotExist(err) { + return err + } + } + return nil } From 586fa68a857cdd9bb73a9d1b6d2f0c542a1380a0 Mon Sep 17 00:00:00 2001 From: spapa013 Date: Tue, 17 Mar 2026 21:06:08 +0000 Subject: [PATCH 07/12] refactor(templates): remove generation spec and keep post-render as a no-op hook - remove GenerationSpec and its builders - simplify post-render to a no-op extension point for future work - keep PostRenderOptions minimal - reduce tests to cover the current no-op post-render contract --- internal/templates/generation_spec.go | 31 ----------- internal/templates/post_render.go | 49 ++--------------- internal/templates/post_render_test.go | 73 +------------------------- 3 files changed, 6 insertions(+), 147 deletions(-) delete mode 100644 internal/templates/generation_spec.go diff --git a/internal/templates/generation_spec.go b/internal/templates/generation_spec.go deleted file mode 100644 index 3513a2f..0000000 --- a/internal/templates/generation_spec.go +++ /dev/null @@ -1,31 +0,0 @@ -package templates - -import "github.com/nauticalab/devenv-engine/internal/config" - -type GenerationSpec[T config.BaseConfig | config.DevEnvConfig] struct { - Config *T - Plan RenderPlan - OutputDir string - TemplateRoot string - PostRenderOptions PostRenderOptions -} - -func BuildDevGenerationSpec(cfg *config.DevEnvConfig, outputDir string, postRenderOpts PostRenderOptions) GenerationSpec[config.DevEnvConfig] { - return GenerationSpec[config.DevEnvConfig]{ - Config: cfg, - Plan: BuildDevRenderPlan(cfg), - OutputDir: outputDir, - TemplateRoot: "template_files/dev", - PostRenderOptions: postRenderOpts, - } -} - -func BuildSystemGenerationSpec(cfg *config.BaseConfig, outputDir string, postRenderOpts PostRenderOptions) GenerationSpec[config.BaseConfig] { - return GenerationSpec[config.BaseConfig]{ - Config: cfg, - Plan: BuildSystemRenderPlan(), - OutputDir: outputDir, - TemplateRoot: "template_files/system", - PostRenderOptions: postRenderOpts, - } -} diff --git a/internal/templates/post_render.go b/internal/templates/post_render.go index a691ae4..23b5104 100644 --- a/internal/templates/post_render.go +++ b/internal/templates/post_render.go @@ -1,54 +1,13 @@ package templates -import ( - "fmt" - "os" - "path/filepath" -) +// PostRenderOptions reserves space for future post-render behaviors. +type PostRenderOptions struct{} -type PostRenderOptions struct { - CleanupUnplanned bool -} - -func NewPostRenderOptions(cleanupUnplanned bool) PostRenderOptions { - return PostRenderOptions{CleanupUnplanned: cleanupUnplanned} +func NewPostRenderOptions() PostRenderOptions { + return PostRenderOptions{} } // RunPostRender executes post-render steps for a completed render pass. func RunPostRender(outputDir string, plan RenderPlan, opts PostRenderOptions) error { - if opts.CleanupUnplanned { - if err := runPostRenderCleanup(outputDir, plan); err != nil { - return fmt.Errorf("cleanup unplanned outputs: %w", err) - } - } - - return nil -} - -// runPostRenderCleanup removes managed outputs that are no longer planned. -func runPostRenderCleanup(outputDir string, plan RenderPlan) error { - planned := make(map[string]struct{}, len(plan.TemplateNames)) - for _, templateName := range plan.TemplateNames { - planned[templateName] = struct{}{} - } - - for _, templateName := range plan.ManagedTemplates { - if _, ok := planned[templateName]; ok { - continue - } - if err := removeOutput(outputDir, templateName); err != nil { - return fmt.Errorf("failed to remove output for template %s: %w", templateName, err) - } - } - - return nil -} - -func removeOutput(outputDir string, templateName string) error { - outputPath := filepath.Join(outputDir, fmt.Sprintf("%s.yaml", templateName)) - err := os.Remove(outputPath) - if err != nil && !os.IsNotExist(err) { - return err - } return nil } diff --git a/internal/templates/post_render_test.go b/internal/templates/post_render_test.go index 2055240..fecfd04 100644 --- a/internal/templates/post_render_test.go +++ b/internal/templates/post_render_test.go @@ -1,81 +1,12 @@ package templates import ( - "os" - "path/filepath" "testing" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -func TestRunPostRender_RemovesUnplannedManagedOutputs(t *testing.T) { - tempDir := t.TempDir() - staleIngress := filepath.Join(tempDir, "ingress.yaml") - require.NoError(t, os.WriteFile(staleIngress, []byte("stale"), 0o644)) - - plan := RenderPlan{ - TemplateNames: []string{"statefulset", "service", "env-vars", "startup-scripts"}, - ManagedTemplates: []string{"statefulset", "service", "env-vars", "startup-scripts", "ingress"}, - } - - err := RunPostRender(tempDir, plan, NewPostRenderOptions(true)) - require.NoError(t, err) - - _, err = os.Stat(staleIngress) - assert.ErrorIs(t, err, os.ErrNotExist) -} - -func TestRunPostRender_PreservesPlannedOutputs(t *testing.T) { - tempDir := t.TempDir() - plannedIngress := filepath.Join(tempDir, "ingress.yaml") - require.NoError(t, os.WriteFile(plannedIngress, []byte("planned"), 0o644)) - - plan := RenderPlan{ - TemplateNames: []string{"statefulset", "service", "env-vars", "startup-scripts", "ingress"}, - ManagedTemplates: []string{"statefulset", "service", "env-vars", "startup-scripts", "ingress"}, - } - - err := RunPostRender(tempDir, plan, NewPostRenderOptions(true)) - require.NoError(t, err) - - content, err := os.ReadFile(plannedIngress) - require.NoError(t, err) - assert.Equal(t, "planned", string(content)) -} - -func TestRunPostRender_IgnoresUnmanagedOutputs(t *testing.T) { - tempDir := t.TempDir() - unmanagedFile := filepath.Join(tempDir, "notes.yaml") - require.NoError(t, os.WriteFile(unmanagedFile, []byte("keep"), 0o644)) - - plan := RenderPlan{ - TemplateNames: []string{"namespace"}, - ManagedTemplates: []string{"namespace"}, - } - - err := RunPostRender(tempDir, plan, NewPostRenderOptions(true)) - require.NoError(t, err) - - content, err := os.ReadFile(unmanagedFile) - require.NoError(t, err) - assert.Equal(t, "keep", string(content)) -} - -func TestRunPostRender_PreservesStaleOutputsWhenCleanupDisabled(t *testing.T) { - tempDir := t.TempDir() - staleIngress := filepath.Join(tempDir, "ingress.yaml") - require.NoError(t, os.WriteFile(staleIngress, []byte("stale"), 0o644)) - - plan := RenderPlan{ - TemplateNames: []string{"statefulset", "service", "env-vars", "startup-scripts"}, - ManagedTemplates: []string{"statefulset", "service", "env-vars", "startup-scripts", "ingress"}, - } - - err := RunPostRender(tempDir, plan, PostRenderOptions{CleanupUnplanned: false}) - require.NoError(t, err) - - content, err := os.ReadFile(staleIngress) +func TestRunPostRender_NoOp(t *testing.T) { + err := RunPostRender(t.TempDir(), RenderPlan{TemplateNames: []string{"namespace"}}, NewPostRenderOptions()) require.NoError(t, err) - assert.Equal(t, "stale", string(content)) } From 75cea76bb0e40e6cee325aaee6809eeee1810960 Mon Sep 17 00:00:00 2001 From: spapa013 Date: Tue, 17 Mar 2026 22:13:35 +0000 Subject: [PATCH 08/12] feat(config): make devenv.yaml mandatory LoadGlobalConfig now returns an error when devenv.yaml is absent, rather than silently falling back to system defaults. The error message names the missing file and tells the user what to create and where. Tests updated to reflect the new contract; the two test cases that asserted the silent-default behavior have been replaced with error assertions. Refs: PLT-855 --- internal/config/parser.go | 9 ++--- internal/config/parser_test.go | 62 +++++----------------------------- 2 files changed, 14 insertions(+), 57 deletions(-) diff --git a/internal/config/parser.go b/internal/config/parser.go index 3c048c3..52ce94e 100644 --- a/internal/config/parser.go +++ b/internal/config/parser.go @@ -10,17 +10,18 @@ import ( ) // LoadGlobalConfig loads the global configuration file (devenv.yaml) from the config directory. -// Returns a BaseConfig pre-populated with system defaults. If the global config file exists, -// YAML values override the defaults. If the file doesn't exist, returns defaults without error. +// devenv.yaml is mandatory: the function returns an error if the file is absent. +// When the file exists, YAML values are unmarshalled on top of system defaults so that +// any field not explicitly set in the file retains its built-in default value. func LoadGlobalConfig(configDir string) (*BaseConfig, error) { globalConfigPath := filepath.Join(configDir, "devenv.yaml") // Start with system defaults globalConfig := NewBaseConfigWithDefaults() - // Check if global config file exists + // devenv.yaml is required — fail fast with an actionable message if it is missing. if _, err := os.Stat(globalConfigPath); os.IsNotExist(err) { - return &globalConfig, nil // Return defaults if file doesn't exist + return nil, fmt.Errorf("shared config file not found: %s\n\ndevenv.yaml is required. Create it in %s to define shared settings (image, namespace, hostName, auth, etc.).", globalConfigPath, configDir) } // Read the global config file diff --git a/internal/config/parser_test.go b/internal/config/parser_test.go index 5f49fca..e7498a0 100644 --- a/internal/config/parser_test.go +++ b/internal/config/parser_test.go @@ -57,33 +57,13 @@ resources: assert.Equal(t, 0, cfg.Resources.GPU) // default GPU unchanged }) - t.Run("global config file does not exist -> system defaults", func(t *testing.T) { + t.Run("global config file does not exist -> error", func(t *testing.T) { tempDir := t.TempDir() cfg, err := LoadGlobalConfig(tempDir) - require.NoError(t, err) - - // 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, 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) - - // 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) + require.Error(t, err) + assert.Nil(t, cfg) + assert.Contains(t, err.Error(), "devenv.yaml is required") }) t.Run("invalid YAML in global config -> error", func(t *testing.T) { @@ -295,38 +275,14 @@ git: assert.Equal(t, developerDir, cfg.DeveloperDir) }) - t.Run("user config with no global config", func(t *testing.T) { + t.Run("user config with no global config -> error", func(t *testing.T) { tempDir := t.TempDir() - // Only user config (no devenv.yaml) - developerDir := filepath.Join(tempDir, "alice") - require.NoError(t, os.MkdirAll(developerDir, 0o755)) - - userConfigYAML := `name: alice -sshPublicKey: "ssh-rsa AAAAB3NzaC1yc2E alice@example.com" -installHomebrew: false -` - require.NoError(t, os.WriteFile(filepath.Join(developerDir, "devenv-config.yaml"), []byte(userConfigYAML), 0o644)) - - // Global = system defaults (no file present) + // devenv.yaml is mandatory; loading without it must fail before developer config is attempted. globalCfg, err := LoadGlobalConfig(tempDir) - require.NoError(t, err) - - cfg, err := LoadDeveloperConfigWithBaseConfig(tempDir, "alice", globalCfg) - require.NoError(t, err) - - // 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, 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()) + require.Error(t, err) + assert.Nil(t, globalCfg) + assert.Contains(t, err.Error(), "devenv.yaml is required") }) } From c0226fa35e1e96ae81f691b2f1df7c801240620a Mon Sep 17 00:00:00 2001 From: spapa013 Date: Tue, 17 Mar 2026 22:18:33 +0000 Subject: [PATCH 09/12] refactor(config): remove stat+read pattern in config loaders Replace the two-step os.Stat + os.ReadFile pattern in all three config load functions with a single os.ReadFile call, inspecting the error for os.IsNotExist. Eliminates a redundant syscall and the race window between the existence check and the read. Refs: PLT-855 --- internal/config/parser.go | 30 +++++++++++------------------- 1 file changed, 11 insertions(+), 19 deletions(-) diff --git a/internal/config/parser.go b/internal/config/parser.go index 52ce94e..fd1c9bd 100644 --- a/internal/config/parser.go +++ b/internal/config/parser.go @@ -20,13 +20,11 @@ func LoadGlobalConfig(configDir string) (*BaseConfig, error) { globalConfig := NewBaseConfigWithDefaults() // devenv.yaml is required — fail fast with an actionable message if it is missing. - if _, err := os.Stat(globalConfigPath); os.IsNotExist(err) { - return nil, fmt.Errorf("shared config file not found: %s\n\ndevenv.yaml is required. Create it in %s to define shared settings (image, namespace, hostName, auth, etc.).", globalConfigPath, configDir) - } - - // Read the global config file data, err := os.ReadFile(globalConfigPath) if err != nil { + if os.IsNotExist(err) { + return nil, fmt.Errorf("shared config file not found: %s\n\ndevenv.yaml is required. Create it in %s to define shared settings (image, namespace, hostName, auth, etc.).", globalConfigPath, configDir) + } return nil, fmt.Errorf("failed to read global config file %s: %w", globalConfigPath, err) } @@ -49,20 +47,17 @@ func LoadDeveloperConfig(configDir, developerName string) (*DevEnvConfig, error) developerDir := filepath.Join(configDir, developerName) configPath := filepath.Join(developerDir, "devenv-config.yaml") - // Check if the config file exists - if _, err := os.Stat(configPath); os.IsNotExist(err) { - return nil, fmt.Errorf("configuration file not found: %s", configPath) - } + // Create empty config (no defaults) + var config DevEnvConfig - // Read the file data, err := os.ReadFile(configPath) if err != nil { + if os.IsNotExist(err) { + return nil, fmt.Errorf("configuration file not found: %s", configPath) + } return nil, fmt.Errorf("failed to read config file %s: %w", configPath, err) } - // Create empty config (no defaults) - var config DevEnvConfig - // Parse the YAML if err := yaml.Unmarshal(data, &config); err != nil { return nil, fmt.Errorf("failed to parse YAML in %s: %w", configPath, err) @@ -92,14 +87,11 @@ func LoadDeveloperConfigWithBaseConfig(configDir, developerName string, baseConf developerDir := filepath.Join(configDir, developerName) configPath := filepath.Join(developerDir, "devenv-config.yaml") - // Check if the config file exists - if _, err := os.Stat(configPath); os.IsNotExist(err) { - return nil, fmt.Errorf("configuration file not found: %s", configPath) - } - - // Read the file data, err := os.ReadFile(configPath) if err != nil { + if os.IsNotExist(err) { + return nil, fmt.Errorf("configuration file not found: %s", configPath) + } return nil, fmt.Errorf("failed to read config file %s: %w", configPath, err) } From 280c3037f0f856bed0e5234778745780425aed15 Mon Sep 17 00:00:00 2001 From: spapa013 Date: Wed, 18 Mar 2026 00:43:40 +0000 Subject: [PATCH 10/12] feat(config): make home directory mount point configurable Adds HomeDirMountBase to BaseConfig, allowing the host path prefix for per-developer home and linuxbrew volumes to be configured rather than being hardcoded to /mnt/devenv in the StatefulSet template. Defaults to /mnt/devenv to preserve existing behavior. Also removes stray trailing whitespace from the StatefulSet golden file. --- internal/config/types.go | 4 ++++ internal/templates/renderer_test.go | 1 + .../templates/template_files/dev/manifests/statefulset.tmpl | 4 ++-- internal/templates/testdata/golden/statefulset.yaml | 2 +- 4 files changed, 8 insertions(+), 3 deletions(-) diff --git a/internal/config/types.go b/internal/config/types.go index 6bea234..ad65070 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -22,6 +22,9 @@ type BaseConfig struct { // Storage configuration Volumes []VolumeMount `yaml:"volumes,omitempty" validate:"dive"` + // HomeDirMountBase is the host path prefix under which per-developer home + // and linuxbrew volumes are created (e.g. /mnt/devenv → /mnt/devenv//homedir). + HomeDirMountBase string `yaml:"homeDirMountBase,omitempty" validate:"omitempty,mount_path"` // Access configuration SSHPublicKey any `yaml:"sshPublicKey,omitempty" validate:"omitempty,ssh_keys"` // Can be string or []string @@ -128,6 +131,7 @@ func NewBaseConfigWithDefaults() BaseConfig { }, GitRepos: []GitRepo{}, // Empty slice - no default git repositories Volumes: []VolumeMount{}, // Empty slice - no default volumes + HomeDirMountBase: "/mnt/devenv", // Default host path prefix for home/linuxbrew volumes Namespace: "devenv", // Default namespace EnvironmentName: "development", // Default environment name } diff --git a/internal/templates/renderer_test.go b/internal/templates/renderer_test.go index 1f9dcca..a10019f 100644 --- a/internal/templates/renderer_test.go +++ b/internal/templates/renderer_test.go @@ -28,6 +28,7 @@ func TestRenderTemplate(t *testing.T) { Image: "ubuntu:22.04", Namespace: "devenv-test", HostName: "devenv.example.com", + HomeDirMountBase: "/mnt/devenv", Packages: config.PackageConfig{ Python: []string{"numpy", "pandas"}, APT: []string{"vim", "curl"}, diff --git a/internal/templates/template_files/dev/manifests/statefulset.tmpl b/internal/templates/template_files/dev/manifests/statefulset.tmpl index 6cae7cb..1d97899 100644 --- a/internal/templates/template_files/dev/manifests/statefulset.tmpl +++ b/internal/templates/template_files/dev/manifests/statefulset.tmpl @@ -113,11 +113,11 @@ spec: volumes: - name: dev-storage hostPath: - path: /mnt/devenv/{{.Name}}/homedir + path: {{.HomeDirMountBase}}/{{.Name}}/homedir type: DirectoryOrCreate - name: dev-linuxbrew hostPath: - path: /mnt/devenv/{{.Name}}/linuxbrew + path: {{.HomeDirMountBase}}/{{.Name}}/linuxbrew type: DirectoryOrCreate - name: startup-scripts configMap: diff --git a/internal/templates/testdata/golden/statefulset.yaml b/internal/templates/testdata/golden/statefulset.yaml index 33e7ae9..290fcf5 100644 --- a/internal/templates/testdata/golden/statefulset.yaml +++ b/internal/templates/testdata/golden/statefulset.yaml @@ -94,7 +94,7 @@ spec: type: DirectoryOrCreate - name: dev-linuxbrew hostPath: - path: /mnt/devenv/testuser/linuxbrew + path: /mnt/devenv/testuser/linuxbrew type: DirectoryOrCreate - name: startup-scripts configMap: From dd6a3177416f1d4612019cd2ced5bbd677ff1ac9 Mon Sep 17 00:00:00 2001 From: spapa013 Date: Wed, 18 Mar 2026 17:17:55 +0000 Subject: [PATCH 11/12] refactor(config): replace hand-written PythonBinPath absolute-path check with mount_path tag - Update PythonBinPath validate tag from `omitempty,min=1` to `omitempty,mount_path` - Delete validatePythonBinPathAbsolute() and its two call sites in ValidateDevEnvConfig and ValidateBaseConfig - Update tests to match tag-based error message format Refs PLT-983 --- internal/config/types.go | 2 +- internal/config/validation.go | 17 ----------------- internal/config/validation_test.go | 8 ++++---- 3 files changed, 5 insertions(+), 22 deletions(-) diff --git a/internal/config/types.go b/internal/config/types.go index ad65070..96ea6a2 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -33,7 +33,7 @@ type BaseConfig struct { InstallHomebrew bool `yaml:"installHomebrew,omitempty"` ClearLocalPackages bool `yaml:"clearLocalPackages,omitempty"` ClearVSCodeCache bool `yaml:"clearVSCodeCache,omitempty"` - PythonBinPath string `yaml:"pythonBinPath,omitempty" validate:"omitempty,min=1"` + PythonBinPath string `yaml:"pythonBinPath,omitempty" validate:"omitempty,mount_path"` HostName string `yaml:"hostName,omitempty" validate:"omitempty,min=1,hostname"` EnableAuth bool `yaml:"enableAuth,omitempty"` AuthURL string `yaml:"authURL,omitempty" validate:"omitempty,min=1,url"` diff --git a/internal/config/validation.go b/internal/config/validation.go index 4d051f5..a8f1ce9 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -224,9 +224,6 @@ func ValidateDevEnvConfig(config *DevEnvConfig) error { if err := validate.Struct(config); err != nil { return formatValidationError(err) } - if err := validatePythonBinPathAbsolute(config.PythonBinPath); err != nil { - return err - } // Require ≥1 SSH public key with valid format. sshKeys, err := config.GetSSHKeys() @@ -275,20 +272,6 @@ func ValidateBaseConfig(config *BaseConfig) error { if err := validate.Struct(config); err != nil { return formatValidationError(err) } - if err := validatePythonBinPathAbsolute(config.PythonBinPath); err != nil { - return err - } - return nil -} - -func validatePythonBinPathAbsolute(p string) error { - p = strings.TrimSpace(p) - if p == "" { - return nil - } - if !path.IsAbs(p) { - return fmt.Errorf("pythonBinPath must be an absolute path, got %q", p) - } return nil } diff --git a/internal/config/validation_test.go b/internal/config/validation_test.go index 1a5f428..3261709 100644 --- a/internal/config/validation_test.go +++ b/internal/config/validation_test.go @@ -253,8 +253,8 @@ func TestValidateBaseConfig_PythonBinPathMustBeAbsolute(t *testing.T) { bad := &BaseConfig{PythonBinPath: "usr/bin"} err := ValidateBaseConfig(bad) require.Error(t, err) - assert.Contains(t, err.Error(), "pythonBinPath") - assert.Contains(t, err.Error(), "absolute path") + assert.Contains(t, err.Error(), "PythonBinPath") + assert.Contains(t, err.Error(), "absolute mount path") } func TestValidateDevEnvConfig_PythonBinPathMustBeAbsolute(t *testing.T) { @@ -276,8 +276,8 @@ func TestValidateDevEnvConfig_PythonBinPathMustBeAbsolute(t *testing.T) { } err := ValidateDevEnvConfig(bad) require.Error(t, err) - assert.Contains(t, err.Error(), "pythonBinPath") - assert.Contains(t, err.Error(), "absolute path") + assert.Contains(t, err.Error(), "PythonBinPath") + assert.Contains(t, err.Error(), "absolute mount path") } func TestValidator_MountPath(t *testing.T) { From 33c7c68e3fa359f76f5ea9911a1ccd7c00e715c1 Mon Sep 17 00:00:00 2001 From: spapa013 Date: Wed, 18 Mar 2026 17:45:15 +0000 Subject: [PATCH 12/12] refactor(config): replace filepath validator on GitRepo.Directory with mount_path - replaced filepath tag with mount_path on GitRepo.Directory - dropped redundant min=1 (mount_path already rejects empty/whitespace) - added test confirming relative paths are rejected Refs PLT-984 --- internal/config/types.go | 2 +- internal/config/validation_test.go | 31 ++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/internal/config/types.go b/internal/config/types.go index 96ea6a2..0e21210 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -83,7 +83,7 @@ type GitRepo struct { Branch string `yaml:"branch,omitempty" validate:"omitempty,min=1"` Tag string `yaml:"tag,omitempty" validate:"omitempty,min=1"` CommitHash string `yaml:"commitHash,omitempty" validate:"omitempty,min=1"` - Directory string `yaml:"directory,omitempty" validate:"omitempty,min=1,filepath"` + Directory string `yaml:"directory,omitempty" validate:"omitempty,mount_path"` } // ResourceConfig represents resource allocation diff --git a/internal/config/validation_test.go b/internal/config/validation_test.go index 3261709..a38e710 100644 --- a/internal/config/validation_test.go +++ b/internal/config/validation_test.go @@ -348,6 +348,37 @@ func TestValidateDevEnvConfig_VolumeMountPaths(t *testing.T) { }) } +func TestValidateDevEnvConfig_GitRepoDirectory(t *testing.T) { + newCfg := func(directory string) *DevEnvConfig { + return &DevEnvConfig{ + Name: "alice", + BaseConfig: BaseConfig{ + SSHPublicKey: "ssh-ed25519 AAAAB3NzaC1lZDI1NTE5AAAA user@host", + GitRepos: []GitRepo{ + { + URL: "https://github.com/example/repo", + Directory: directory, + }, + }, + }, + } + } + + t.Run("accepts absolute path", func(t *testing.T) { + require.NoError(t, ValidateDevEnvConfig(newCfg("/home/user/repos/myrepo"))) + }) + + t.Run("accepts empty directory (optional field)", func(t *testing.T) { + require.NoError(t, ValidateDevEnvConfig(newCfg(""))) + }) + + t.Run("rejects relative path", func(t *testing.T) { + err := ValidateDevEnvConfig(newCfg("repos/myrepo")) + require.Error(t, err) + assert.Contains(t, err.Error(), "Directory") + }) +} + func TestValidateDevEnvConfig_IngressDependencies(t *testing.T) { newCfg := func() *DevEnvConfig { return &DevEnvConfig{