-
Notifications
You must be signed in to change notification settings - Fork 35
Validate update plan consistency #603
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds validation to the update plan configuration in Ytsaurus resources to ensure update plan consistency. The validation enforces two main rules: (1) component types specified in update plans must be valid local component types, and (2) update plans with Nothing or Everything class selectors should not contain other selectors.
Key changes:
- Added component type validation against the
LocalComponentTypesconstant - Extended the Nothing/Everything exclusivity validation to cover both Nothing and Everything classes
- Refactored field path construction to reduce code duplication
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
api/v1/ytsaurus_webhook.go
Outdated
| if (hasNothing || hasEverything) && i > 0 { | ||
| allErrors = append(allErrors, field.Invalid(selectorPath, updateSelector, "Update plan with class Nothing or Everything should not contain anything more")) | ||
| } |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validation logic for Nothing/Everything class exclusivity is incorrect. The current condition (hasNothing || hasEverything) && i > 0 has two problems:
-
It only triggers when processing items after the first one, so if Nothing/Everything is at index 0 followed by other items, the error is reported at the wrong location (on the second item rather than explaining that Nothing/Everything must be alone).
-
If Nothing/Everything appears at index > 0, the error message incorrectly suggests it "should not contain anything more" when the real issue is that Nothing/Everything must be at index 0 and must be the only element.
The correct validation should check: if the current selector has Nothing or Everything class, verify that it's at index 0 AND the plan length is 1. Consider replacing with:
if updateSelector.Class == consts.ComponentClassNothing || updateSelector.Class == consts.ComponentClassEverything {
if i != 0 || len(newYtsaurus.Spec.UpdatePlan) != 1 {
allErrors = append(allErrors, field.Invalid(selectorPath, updateSelector, "Update plan with class Nothing or Everything must be the only element in the plan"))
}
}1861b86 to
a04bcaa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
a04bcaa to
a325f56
Compare
- component types and classes should be valid - plan with class Nothing or Everything should not contain anything else Signed-off-by: Konstantin Khlebnikov <[email protected]>
a325f56 to
c147011
Compare
qurname2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rebased from main, LGTM.
Signed-off-by: Konstantin Khlebnikov [email protected]