Closed
Conversation
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
…rated-without-corresponding-http-service-when Ensure ingress generation follows HTTP/hostname contract and split manifest generation into plan/render/post-render stages
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR merges changes from develop/spapadop, aligning ingress manifest generation with the HTTP/hostname configuration contract and refactoring manifest generation into explicit plan/render/post-render phases to prevent template-selection drift.
Changes:
- Gate ingress rendering on
httpPort+hostNamevia shared config helpers and dev render planning. - Introduce
RenderPlan,GenerationSpec, and a post-render cleanup phase (with--no-cleanup) to separate planning, rendering, and output management. - Add/update tests and golden fixtures to cover the new ingress contract and cleanup semantics.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| cmd/devenv/generate.go | Switch CLI generation to GenerationSpec + generateManifests, add --no-cleanup, and run post-render steps after successful rendering. |
| internal/config/types.go | Add HasHTTPPort, HasHostName, and ShouldRenderIngress helpers to centralize ingress eligibility. |
| internal/config/validation.go | Enforce config dependencies (httpPort→hostName, auth option constraints) consistent with rendering behavior. |
| internal/config/validation_test.go | Add validation tests for ingress/auth dependency rules. |
| internal/templates/generation_spec.go | Add GenerationSpec to bundle config, plan, template root, output dir, and post-render options. |
| internal/templates/plan.go | Add RenderPlan plus dev/system plan builders to compute template sets before rendering. |
| internal/templates/plan_test.go | Add contract and invariant tests for dev/system template planning and managed-template ownership. |
| internal/templates/post_render.go | Add post-render cleanup of unplanned managed outputs. |
| internal/templates/post_render_test.go | Add tests for cleanup behavior (remove stale, preserve planned/unmanaged, respect cleanup disable). |
| internal/templates/renderer.go | Bind renderer to config + target template list; simplify RenderTemplate/RenderAll signatures. |
| internal/templates/renderer_test.go | Update renderer tests for new APIs and add cases verifying ingress inclusion/exclusion and stale-file behavior on render failure. |
| internal/templates/testdata/golden/ingress.yaml | Update golden ingress fixture to use a valid hostname-based rule and TLS wildcard. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| templateNames := append([]string{}, devBaseTemplates...) | ||
|
|
||
| if cfg != nil && cfg.ShouldRenderIngress() { | ||
| templateNames = append(templateNames, "ingress") |
Collaborator
Author
There was a problem hiding this comment.
The reason why we don't use devOptionalTemplates here is because In the future devOptionalTemplates could contain more than ingress, however, the relevant line in BuildDevRenderPlan specifically checks whether ingress should be included in templateNames, and it will always be ingress.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR merges changes from develop/spapadop.
Closes PLT-851 #29