-
Notifications
You must be signed in to change notification settings - Fork 2
fix(vm): prohibit duplicating networks in the VirtualMachine .spec
#1545
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
Reviewer's GuideThis PR refactors the VirtualMachine network validation by splitting the generic Validate function into dedicated ValidateCreate and ValidateUpdate methods, and adds logic to prohibit duplicate network names in both creation and update scenarios, with accompanying unit test enhancements. Class diagram for updated NetworksValidator methods and logicclassDiagram
class NetworksValidator {
+ValidateCreate(ctx, vm)
+ValidateUpdate(ctx, oldVM, newVM)
-featureGate
}
class VirtualMachine {
Spec: VirtualMachineSpec
}
class VirtualMachineSpec {
Networks: []Network
}
class Network {
Type
Name
}
NetworksValidator --> VirtualMachine
VirtualMachine --> VirtualMachineSpec
VirtualMachineSpec --> Network
Flow diagram for duplicate network name validation in VirtualMachine specflowchart TD
A["Start validation"] --> B["Iterate over Networks in spec"]
B --> C{Is network name empty?}
C -- Yes --> D["Error: network must have non-empty name"]
C -- No --> E{Is network name already in namesSet?}
E -- Yes --> F["Error: network name is duplicated"]
E -- No --> G["Add name to namesSet"]
G --> H["Continue iteration"]
H --> B
F --> I["End validation"]
D --> I
B --> J["All networks validated"]
J --> K["Validation successful"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes - here's some feedback:
- Consider extracting the common network validation steps (main-type placement, name presence, duplicate checks) into a shared helper to avoid duplicating logic in ValidateCreate and ValidateUpdate.
- ValidateCreate currently skips checking the SDN feature gate while ValidateUpdate blocks configurations when SDN is disabled—add a feature-gate check to the create path for consistency.
- The isUnchanged branch in ValidateUpdate bypasses duplicate name checks when specs match, which may let invalid existing configurations persist—consider always validating duplicates on the new spec.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extracting the common network validation steps (main-type placement, name presence, duplicate checks) into a shared helper to avoid duplicating logic in ValidateCreate and ValidateUpdate.
- ValidateCreate currently skips checking the SDN feature gate while ValidateUpdate blocks configurations when SDN is disabled—add a feature-gate check to the create path for consistency.
- The isUnchanged branch in ValidateUpdate bypasses duplicate name checks when specs match, which may let invalid existing configurations persist—consider always validating duplicates on the new spec.
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
865ae20
to
355af94
Compare
Workflow has started. The target step completed with status: success. |
88d4d39
to
589c253
Compare
Signed-off-by: Dmitry Lopatin <[email protected]>
Signed-off-by: Dmitry Lopatin <[email protected]>
589c253
to
3b193ce
Compare
Description
This update enhances the validation logic for Virtual Machines' network configurations by prohibiting duplicate network names within the specification.
Checklist
Changelog entries