fix(webhook): separate CronJob validation errors with "; " - #5610
Conversation
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 CronJob validation logic by introducing a helper function validateCronJob that aggregates validation errors and joins them with a semicolon separator, along with adding corresponding unit tests. The review feedback suggests trimming leading and trailing whitespace from the individual validation messages before joining them to ensure clean, consistent error formatting, and updating the unit tests accordingly.
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
This PR fixes CronJob admission webhook validation output so that when multiple validators fail, their error messages are separated by "; " instead of being concatenated into a run-on string. This improves readability of rejection messages while keeping single-error messages unchanged.
Changes:
- Introduced a shared
validateCronJobhelper that collects non-empty validator messages and joins them with"; ". - Updated both create and update validation paths to use the shared helper.
- Added regression tests for multi-error separation and single-error unchanged behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/webhooks/admission/cronjobs/validate/admit_cronjob.go | Adds validateCronJob helper and routes create/update validation through it, joining multiple error messages with "; ". |
| pkg/webhooks/admission/cronjobs/validate/admit_cronjob_test.go | Adds tests asserting multi-error messages include "; " and single-error messages are unchanged. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
When a CronJob failed more than one check, the messages were joined with no separator and ran together in one line. Trim each validator message and join the non-empty ones with "; " so they read as separate items. Signed-off-by: aeron-gh <agab0323@gmail.com>
1d8c2a2 to
6d60d92
Compare
|
e2e-shard / E2E Agent Scheduler Soft looks like an unrelated flake The rest is green. Re-running to confirm. |
hajnalmt
left a comment
hajnalmt
left a comment
There was a problem hiding this comment.
/area webhooks
/approve
/lgtm
Thank you for the contribution!
| func TestValidateCronJobSeparatesMultipleErrors(t *testing.T) { | ||
| cronjob := &v1alpha1.CronJob{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: strings.Repeat("a", 53), |
There was a problem hiding this comment.
I really like these funny touches in our tests 😄
There was a problem hiding this comment.
thanks @hajnalmt! Glad the tests landed well 😄 Appreciate the quick review.
volcano-sh-bot
added
area/webhooks
lgtm
|
[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
/kind bug
When a CronJob fails more than one check at once (say a bad schedule and a name over the length limit), the webhook joined the messages with no separator, so the rejection came out as one run-on line. This trims each validator message and joins the non-empty ones with "; " so each error reads on its own.
It's the CronJob version of the PodGroup fix in #5487 (same collect-and-join, with the leading/trailing whitespace trimmed for a clean result). Going by @hajnalmt's grep on that PR, this was the last spot left with the pattern. It's one
validateCronJobhelper shared by the create and update paths, plus a couple of tests.Scoped to the CronJob webhook. The
admit_job.gocleanup the issue mentions uses a different convention (its own leading-space / trailing-;style), so it's better kept as a separate change.Thanks @anxkhn for starting this off in #5533. I opened this since it's issue (#5489) and I already had the same pattern from #5487.
Fixes #5489