Validate targetModels entries for empty modelServerName and nil targets#1086
Validate targetModels entries for empty modelServerName and nil targets#1086madmecodes wants to merge 1 commit into
Conversation
Reject targetModels entries with empty modelServerName and nil target entries during webhook validation, preventing invalid objects from reaching the router datastore. Fixes volcano-sh#1060 Signed-off-by: Ayush Gupta <ayushguptadev1@gmail.com> Signed-off-by: madmecodes <ayushguptadev1@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the ModelRoute validation logic in the kthena-router webhook by ensuring that target models are not nil and that the ModelServerName field is populated. Additionally, it includes comprehensive unit tests to verify these new validation constraints across various scenarios, including nil entries and empty strings. I have no feedback to provide.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds webhook validation for targetModels entries to reject nil entries and entries with an empty modelServerName.
Changes:
- Add per-target validation loop in
validateModelRoute. - Add three new test cases for empty
modelServerName, nil target, and mixed invalid entries.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/kthena-router/webhook/validator.go | Iterates targetModels to flag nil entries and empty modelServerName. |
| pkg/kthena-router/webhook/validator_test.go | Adds test cases covering the new validation paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
duplicate with #1061 |
|
Closing as duplicate of #1061, which was submitted first. Thanks for pointing it out! |
Summary
Fixes #1060
Test plan