Ensure ingress generation follows HTTP/hostname contract and split manifest generation into plan/render/post-render stages by spapa013 · Pull Request #31 · nauticalab/devenv-engine

spapa013 · 2026-03-17T21:25:02Z

Summary

This PR fixes a manifest generator consistency bug where ingress.yaml could be generated even when the corresponding HTTP service was not.

In the reproduced case, ingress generation used shared ingress-related config and emitted a backend reference to devenv-http-<name>, but service generation did not emit that service because httpPort was not configured. The result was an ingress object that was accepted by the ingress controller and received address/TLS configuration, but was not actually routable because the backend service did not exist.

This PR makes ingress generation depend on the same HTTP backend requirements as service generation, so generated manifests are internally consistent:

when httpPort is unset, no invalid HTTP ingress backend is emitted
when httpPort is set, the corresponding HTTP service is generated and matches the ingress backend reference

It also refactors manifest generation so conditional template selection is explicit and easier to reason about as the generator grows.

Problem

The original bug was a mismatch between ingress generation and service generation.

Ingress assumes an HTTP backend service exists:

ingress.tmpl routes to devenv-http-<name>

But the HTTP service in service.tmpl is only generated when httpPort is configured.

That meant the generator could produce this invalid combination:

ingress.yaml references devenv-http-<name>
service.yaml does not define devenv-http-<name>

The root issue was generator inconsistency: ingress and service did not share one explicit rule for when HTTP exposure was actually present.

What Changed

1. Ingress generation now follows explicit HTTP backend requirements

Ingress eligibility is now centralized in shared config helpers:

HasHTTPPort()
HasHostName()
ShouldRenderIngress()

The generator now only includes ingress in the render plan when the required HTTP backend configuration is present.

This aligns ingress generation with service generation:

no httpPort means no HTTP service and no ingress
with httpPort, the HTTP service is generated and ingress can safely reference it

2. Validation now reflects the same ingress contract

Config validation now enforces the intended expectations:

httpPort requires hostName
enableAuth and skipAuth cannot both be true
enableAuth requires both authURL and authSignIn

This documents and enforces the dependency between ingress generation and HTTP configuration instead of leaving it implicit in templates.

3. Template selection is now explicit

This bug happened because conditional template generation had no single explicit planning layer.

To address that, template selection is now handled through RenderPlan, which defines:

which templates should be rendered for a given config

For developer manifests, the plan starts from the full dev template set and filters out ingress unless the config satisfies the shared ingress contract.

This made it possible to encode the ingress/service dependency once and test it directly.

4. Rendering and post-render output handling are now separate phases

To keep the fix maintainable as generation grows more conditional, the pipeline is now structured as:

plan
render
post-render

The renderer itself remains focused on materializing templates into files.

The post-render step is currently a no-op hook reserved for future output-handling behavior.

5. Cleanup is scoped to known manifest outputs

The generator now performs cleanup before rendering, gated by --no-cleanup.

Cleanup is scoped to the known template set for the render domain:

system generation removes prior system manifest outputs
developer generation removes prior developer manifest outputs

This avoids leaving stale generated manifests behind when the planned template set shrinks, while keeping deletion bounded to the generator’s known output files.

Testing

Added and updated tests across the pipeline:

validation tests for ingress/auth config dependencies
plan tests for dev/system template contracts
renderer tests for render behavior
post-render test for the current no-op hook
ingress golden fixture updated to reflect a valid hostname-based ingress
go test ./... passes
regenerated manifests are internally consistent for the HTTP-enabled and HTTP-disabled cases

Closes PLT-851

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

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

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

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

- 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

- 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

… 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

codecov · 2026-03-17T22:11:38Z

Codecov Report

❌ Patch coverage is 55.17241% with 26 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
cmd/devenv/generate.go	0.00%	21 Missing ⚠️
internal/templates/renderer.go	70.00%	2 Missing and 1 partial ⚠️
internal/config/types.go	66.66%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!

eywalker · 2026-03-18T01:42:48Z

+
+// RenderPlan defines template selection for a render pass.
+type RenderPlan struct {
+	TemplateNames []string


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?

Agreed. Since RenderPlan had been reduced to a thin wrapper around TemplateNames, I removed the wrapper and now pass []string directly.

In the same spirit of keeping the codebase lean and adding functionality only when needed, I also removed the no-op post_render placeholder. The generation pipeline now only contains the active steps we use today: cleanup, template selection, and rendering.

eywalker · 2026-03-18T23:27:12Z


+// HasHTTPPort reports whether HTTP exposure is configured.
+func (c *DevEnvConfig) HasHTTPPort() bool {
+	return c != nil && c.HTTPPort != 0


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.

I took the leaner approach for now and removed the explicit nil checks from HasHTTPPort() and HasHostName().

The reason I originally included them was to make these predicate helpers nil-safe, so a nil *DevEnvConfig would 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.

Simplify template planning by returning template name slices directly instead of wrapping them in a RenderPlan struct that only contained TemplateNames. - change BuildDevRenderPlan to return []string plus error - change BuildSystemRenderPlan to return []string - update generate flow to pass template names directly to renderers - remove the unused plan argument from RunPostRender - update plan, renderer, and post-render tests to match

Simplify the manifest generation pipeline by removing the unused post-render hook and its empty options type. - remove post_render.go and its no-op test - drop RunPostRender calls from system and developer generation flows - keep the pipeline focused on cleanup, plan, and render

Simplify the ingress-related config helpers by removing explicit nil checks from small predicate methods that are only called on real configs in the current flow. - remove nil guard from HasHTTPPort - remove nil guard from HasHostName - keep ShouldRenderIngress composed from the simplified helpers

Simplify BuildDevRenderPlan by removing the explicit nil check and the corresponding nil-specific test. - remove custom nil error from BuildDevRenderPlan - drop the now-unused fmt import from plan.go - remove the nil-config test from plan_test.go

spapa013 added 7 commits March 11, 2026 23:22

spapa013 requested a review from eywalker March 17, 2026 21:25

spapa013 mentioned this pull request Mar 17, 2026

Define devenv cli behavior when shared devenvyaml is absent #32

Open

3 tasks

eywalker requested changes Mar 18, 2026

View reviewed changes

spapa013 added 4 commits March 19, 2026 20:25

spapa013 requested a review from eywalker March 19, 2026 20:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure ingress generation follows HTTP/hostname contract and split manifest generation into plan/render/post-render stages#31

Ensure ingress generation follows HTTP/hostname contract and split manifest generation into plan/render/post-render stages#31
spapa013 wants to merge 11 commits intonauticalab:mainfrom
spapa013:spapadop/plt-851-ingress-is-generated-without-corresponding-http-service-when

spapa013 commented Mar 17, 2026

Uh oh!

codecov Bot commented Mar 17, 2026 •

edited

Loading

Uh oh!

eywalker Mar 18, 2026

Uh oh!

spapa013 Mar 19, 2026

Uh oh!

eywalker Mar 18, 2026

Uh oh!

spapa013 Mar 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

spapa013 commented Mar 17, 2026

Summary

Problem

What Changed

1. Ingress generation now follows explicit HTTP backend requirements

2. Validation now reflects the same ingress contract

3. Template selection is now explicit

4. Rendering and post-render output handling are now separate phases

5. Cleanup is scoped to known manifest outputs

Testing

Uh oh!

codecov Bot commented Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

eywalker Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

spapa013 Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

eywalker Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

spapa013 Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Mar 17, 2026 •

edited

Loading