Replace hand-written PythonBinPath absolute-path check with mount_path validation tag#34
Open
spapa013 wants to merge 11 commits intonauticalab:mainfrom
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
- 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
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
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
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.
…eck 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
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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
1 participant
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.
Summary
validatePythonBinPathAbsolute()function with the existingmount_pathstruct tag onPythonBinPath, consistent with howHomeDirMountBase,LocalPath, andContainerPathare validated.ValidateDevEnvConfigandValidateBaseConfig.Context
Identified during PLT-863. The
mount_pathvalidator already covers the same contract: non-empty, absolute, syntactically valid path. The hand-written check was redundant and kept validation logic split across two places with no obvious connection.Testing
TestValidateBaseConfig_PythonBinPathMustBeAbsoluteandTestValidateDevEnvConfig_PythonBinPathMustBeAbsoluteto assert against the tag-based error message format.go test ./internal/config/...— all tests pass.Closes PLT-983