-
Notifications
You must be signed in to change notification settings - Fork 2
Ensure ingress generation follows HTTP/hostname contract and split manifest generation into plan/render/post-render stages #31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 7 commits
47d3294
b32dc7d
b007818
dd874ac
8a5af0a
b0da71f
586fa68
77187aa
d6141df
e202c21
719617d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| package templates | ||
|
|
||
| import ( | ||
| "fmt" | ||
|
|
||
| "github.com/nauticalab/devenv-engine/internal/config" | ||
| ) | ||
|
|
||
| var devTemplates = []string{"statefulset", "service", "env-vars", "startup-scripts", "ingress"} | ||
|
|
||
| var systemTemplates = []string{"namespace"} | ||
|
|
||
| // RenderPlan defines template selection for a render pass. | ||
| type RenderPlan struct { | ||
| TemplateNames []string | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if RenderPlan entirely consists of TemlateNames, don't you think it might be sufficient to just pass that around instead of further wrapping it in a struct?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. Since In the same spirit of keeping the codebase lean and adding functionality only when needed, I also removed the no-op |
||
| } | ||
|
|
||
| // BuildDevRenderPlan computes the template set from config before rendering. | ||
| func BuildDevRenderPlan(cfg *config.DevEnvConfig) (RenderPlan, error) { | ||
| if cfg == nil { | ||
| return RenderPlan{}, fmt.Errorf("BuildDevRenderPlan requires non-nil config") | ||
| } | ||
|
|
||
| templateNames := make([]string, 0, len(devTemplates)) | ||
| for _, templateName := range devTemplates { | ||
| if templateName == "ingress" && !cfg.ShouldRenderIngress() { | ||
| continue | ||
| } | ||
| templateNames = append(templateNames, templateName) | ||
| } | ||
|
|
||
| return RenderPlan{ | ||
| TemplateNames: templateNames, | ||
| }, nil | ||
| } | ||
|
|
||
| // BuildSystemRenderPlan computes the template set for system-level manifests. | ||
| func BuildSystemRenderPlan() RenderPlan { | ||
| return RenderPlan{ | ||
| TemplateNames: append([]string{}, systemTemplates...), | ||
| } | ||
| } | ||
|
|
||
| func DevCleanupScope() []string { | ||
| return append([]string{}, devTemplates...) | ||
| } | ||
|
|
||
| func SystemCleanupScope() []string { | ||
| return append([]string{}, systemTemplates...) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| 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, err := BuildDevRenderPlan(cfg) | ||
| require.NoError(t, err) | ||
|
|
||
| assert.Equal(t, expectedDevTemplateNames(false), plan.TemplateNames) | ||
| }) | ||
|
|
||
| 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, err := BuildDevRenderPlan(cfg) | ||
| require.NoError(t, err) | ||
|
|
||
| assert.Equal(t, expectedDevTemplateNames(true), plan.TemplateNames) | ||
| }) | ||
|
|
||
| t.Run("excludes ingress when hostName is missing", func(t *testing.T) { | ||
| cfg := &config.DevEnvConfig{HTTPPort: 8080} | ||
|
|
||
| plan, err := BuildDevRenderPlan(cfg) | ||
| require.NoError(t, err) | ||
|
|
||
| assert.Equal(t, expectedDevTemplateNames(false), plan.TemplateNames) | ||
| }) | ||
| } | ||
|
|
||
| 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, err := BuildDevRenderPlan(&config.DevEnvConfig{}) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, []string{"statefulset", "service", "env-vars", "startup-scripts"}, plan.TemplateNames) | ||
| }) | ||
|
|
||
| t.Run("http enabled", func(t *testing.T) { | ||
| 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) | ||
| }) | ||
| } | ||
|
|
||
| func TestBuildSystemRenderPlan(t *testing.T) { | ||
| plan := BuildSystemRenderPlan() | ||
|
|
||
| assert.Equal(t, copyTemplateNames(systemTemplates), plan.TemplateNames) | ||
| } | ||
|
|
||
| func TestBuildSystemRenderPlan_Contract(t *testing.T) { | ||
| plan := BuildSystemRenderPlan() | ||
| assert.Equal(t, []string{"namespace"}, plan.TemplateNames) | ||
| } | ||
|
|
||
| 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 := []string{"statefulset", "service", "env-vars", "startup-scripts"} | ||
| if includeOptional { | ||
| templateNames = append(templateNames, "ingress") | ||
| } | ||
| return templateNames | ||
| } | ||
|
|
||
| func copyTemplateNames(templateNames []string) []string { | ||
| return append([]string{}, templateNames...) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| package templates | ||
|
|
||
| // PostRenderOptions reserves space for future post-render behaviors. | ||
| type PostRenderOptions struct{} | ||
|
|
||
| func NewPostRenderOptions() PostRenderOptions { | ||
| return PostRenderOptions{} | ||
| } | ||
|
|
||
| // RunPostRender executes post-render steps for a completed render pass. | ||
| func RunPostRender(outputDir string, plan RenderPlan, opts PostRenderOptions) error { | ||
| return nil | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| package templates | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestRunPostRender_NoOp(t *testing.T) { | ||
| err := RunPostRender(t.TempDir(), RenderPlan{TemplateNames: []string{"namespace"}}, NewPostRenderOptions()) | ||
| require.NoError(t, err) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you find out if explicitly checking for nil is the standard best practice or it's more common to just leave it unchecked unless you are going to do some speicial nil handling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took the leaner approach for now and removed the explicit nil checks from
HasHTTPPort()andHasHostName().The reason I originally included them was to make these predicate helpers nil-safe, so a nil
*DevEnvConfigwould be interpreted as “not configured” instead of panicking. That can be a reasonable pattern for small boolean helpers, but in our current codepath we only call them on real configs, and we weren’t relying on any special nil behavior. If you’d prefer these helpers to be intentionally nil-safe as part of their contract, I’m happy to add that back consistently.