fix(webhook): add separator between PodGroup validation errors#5487
Conversation
volcano-sh-bot
added
the
do-not-merge/invalid-commit-message
|
Welcome @aeron-gh! It looks like this is your first PR to volcano-sh/volcano 🎉 |
volcano-sh-bot
added
the
size/M
![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 refactors the validatePodGroup function to collect multiple validation errors into a slice and join them with a semicolon separator, rather than concatenating them directly. It also adds a test case to verify this behavior. The review feedback highlights two important improvements: trimming whitespace from individual error messages to prevent formatting issues when they are joined, and adding a nil-pointer guard in the test runner to prevent potential panics when asserting on the response message.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Pull request overview
Improves readability of PodGroup admission webhook rejection messages when multiple validations fail by separating individual error fragments, and adds a regression test to cover the multi-error case described in #5443.
Changes:
- Collects non-empty validation error messages and joins them with
"; "invalidatePodGroup. - Adds a table-driven test case ensuring both queue and
networkTopologyvalidation errors appear with the separator. - Extends the test table with
msgContainsassertions for message content.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/webhooks/admission/podgroups/validate/validate_podgroup.go | Joins multiple validation error strings with a "; " separator instead of concatenating directly. |
| pkg/webhooks/admission/podgroups/validate/validate_podgroup_test.go | Adds a regression test case and message substring assertions for the separated multi-error output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
adf0ef9 to
ab90269
Compare
volcano-sh-bot
removed
the
do-not-merge/invalid-commit-message
|
Gentle ping on this one CI is green and it's a pretty small change. Could someone take a look when you have a moment? Happy to rebase or tweak anything if needed. Thanks! |
validatePodGroup joined each validator's result with += and no separator, so a PodGroup failing more than one check (for example a non-existent queue plus an invalid networkTopology) produced a run-on message that was hard to read. Collect the non-empty messages into a slice and join them with "; ". Also add a test that fails both checks and verifies the separator. Signed-off-by: aeron-gh <agab0323@gmail.com>
ab90269 to
1d90b88
Compare
|
/kind bug Rebased this onto the latest master since it had fallen a bit behind, so CI is green again. It's a small, self-contained webhook fix, would really appreciate a review whenever someone gets a spare moment. Happy to tweak anything if needed. Thanks! |
|
@hajnalmt could you take a quick look when you have a moment? It's a small, self-contained webhook fix and CI is green. Thanks! |
Trim each validation message before joining so the combined output has no stray space before the "; " separator when a helper returns a message with trailing whitespace. Also guard the test's message assertion against a nil Result to avoid a possible nil-pointer panic on allowed responses. Signed-off-by: aeron-gh <agab0323@gmail.com>
|
/retest the failure in |
|
@aeron-gh: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
thanks @hajnalmt both applied trimmed the messages before joining, and added the nil result guard in the test. CI is gren now |
hajnalmt
left a comment
hajnalmt
left a comment
There was a problem hiding this comment.
/label tide/merge-method-squash
/lgtm
/approve
Thank you for the contribution!
volcano-sh-bot
added
tide/merge-method-squash
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hajnalmt The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
volcano-sh-bot
added
the
approved
…no-sh#5487) * fix(webhook): add separator between PodGroup validation errors validatePodGroup joined each validator's result with += and no separator, so a PodGroup failing more than one check (for example a non-existent queue plus an invalid networkTopology) produced a run-on message that was hard to read. Collect the non-empty messages into a slice and join them with "; ". Also add a test that fails both checks and verifies the separator. Signed-off-by: aeron-gh <agab0323@gmail.com> * fix(webhook): trim joined PodGroup validation errors and guard test Trim each validation message before joining so the combined output has no stray space before the "; " separator when a helper returns a message with trailing whitespace. Also guard the test's message assertion against a nil Result to avoid a possible nil-pointer panic on allowed responses. Signed-off-by: aeron-gh <agab0323@gmail.com> --------- Signed-off-by: aeron-gh <agab0323@gmail.com>
…no-sh#5487) * fix(webhook): add separator between PodGroup validation errors validatePodGroup joined each validator's result with += and no separator, so a PodGroup failing more than one check (for example a non-existent queue plus an invalid networkTopology) produced a run-on message that was hard to read. Collect the non-empty messages into a slice and join them with "; ". Also add a test that fails both checks and verifies the separator. Signed-off-by: aeron-gh <agab0323@gmail.com> * fix(webhook): trim joined PodGroup validation errors and guard test Trim each validation message before joining so the combined output has no stray space before the "; " separator when a helper returns a message with trailing whitespace. Also guard the test's message assertion against a nil Result to avoid a possible nil-pointer panic on allowed responses. Signed-off-by: aeron-gh <agab0323@gmail.com> --------- Signed-off-by: aeron-gh <agab0323@gmail.com> Signed-off-by: yash-pokhriyal <er.yash.pokhriyal@gmail.com>
Fixes #5443
What this fixes
validatePodGroupbuilt its rejection message by appending each validator's output with+=and no separator:So when a PodGroup failed more than one check at once (for example a non-existent queue and an invalid
networkTopology), the two messages ran together mid-sentence and the rejection was hard to read.This collects each non-empty message into a slice and joins them with
"; ", so the errors are reported as distinct, readable items. An empty slice still joins to"", so the valid (no-error) path is unchanged.Test
Added a case to
TestValidatePodGroupthat fails both the queue and thenetworkTopologychecks at the same time, and asserts the resulting message contains both fragments and the"; "separator.cc @DevMhrn — you mentioned on the issue that you might pick this up. I already had the fix and test ready so I went ahead and opened it, but happy to defer or close this if you'd rather take it. Either way, no problem at all.