From 47d3294930c231d1de92af71c29f4872e897a196 Mon Sep 17 00:00:00 2001 From: spapa013 Date: Wed, 11 Mar 2026 23:22:18 +0000 Subject: [PATCH 1/4] 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 2/4] 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 3/4] 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 4/4] 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, + } +}