Reject shrinking disk during YAML validation #3596
Conversation
Signed-off-by: Songpon Srisawai <songpon.ssw@gmail.com>
Signed-off-by: Songpon Srisawai <songpon.ssw@gmail.com>
| @@ -125,6 +125,11 @@ func editAction(cmd *cobra.Command, args []string) error { | |||
| // TODO: may need to support editing the rejected YAML | |||
| return fmt.Errorf("the YAML is invalid, saved the buffer as %q: %w", rejectedYAML, err) | |||
| } | |||
There was a problem hiding this comment.
This is same as saveRejectedYAML and should be deduplicated
| @@ -595,3 +595,27 @@ func warnExperimental(y *LimaYAML) { | |||
| logrus.Warn("`mountInotify` is experimental") | |||
| } | |||
| } | |||
|
|
|||
| // ValidateYAMLAgainstLatest validates the values between the latest YAML and the updated(New) YAML. | |||
| func ValidateYAMLAgainstLatest(yNew, yLatest []byte) error { | |||
There was a problem hiding this comment.
What does "latest" mean here?
There was a problem hiding this comment.
This refers to the current (or existing) YAML file for the instance.
Basically, I want to compare the new YAML (edited YAML) to the old YAML (current/existing YAML).
I'm not great at naming things 😅 — any better suggestions for this?
There was a problem hiding this comment.
Pull Request Overview
This PR implements early YAML validation to reject configurations that attempt to shrink the disk size rather than waiting until instance startup.
- Added a ValidateYAMLAgainstLatest function that compares disk sizes from the new and latest YAML configurations.
- Integrated the new validation into the limactl edit flow to save and report rejected configurations.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/limayaml/validate.go | Added a YAML validation function to prevent disk size shrinking. |
| cmd/limactl/edit.go | Integrated validation into the edit command and added a function to handle rejected YAML. |
| // Skip validation if both fields are unset. | ||
| if n.Disk == nil && l.Disk == nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The current nil check only skips validation when both disk fields are nil, which may lead to a nil pointer dereference if only one disk value is set. Consider adding explicit checks to handle cases where one of the disk fields is nil.
| } | |
| } | |
| // Handle cases where one of the fields is nil. | |
| if n.Disk == nil { | |
| return fmt.Errorf("field `disk`: new disk value is unset while the latest disk value is set to %v", *l.Disk) | |
| } | |
| if l.Disk == nil { | |
| return fmt.Errorf("field `disk`: latest disk value is unset while the new disk value is set to %v", *n.Disk) | |
| } | |
| // Both fields are non-nil; proceed with validation. |
Copilot uses AI. Check for mistakes.
| func saveRejectedYAML(y []byte, origErr error) error { | ||
| rejectedYAML := "lima.REJECTED.yaml" | ||
| if writeErr := os.WriteFile(rejectedYAML, y, 0o644); writeErr != nil { | ||
| return fmt.Errorf("the YAML is invalid, attempted to save the buffer as %q but failed: %w: %w", rejectedYAML, writeErr, origErr) |
There was a problem hiding this comment.
The error message wraps two errors using '%w: %w', which might be confusing. Consider simplifying the error wrapping to clearly associate the cause of failure with a single wrapped error.
| return fmt.Errorf("the YAML is invalid, attempted to save the buffer as %q but failed: %w: %w", rejectedYAML, writeErr, origErr) | |
| return fmt.Errorf("the YAML is invalid, attempted to save the buffer as %q but failed: %w", rejectedYAML, writeErr) |
Copilot uses AI. Check for mistakes.
As @jandubois suggested in #3533.
Currently, rejecting shrinking disk size happens during instance startup.
This PR adds validation earlier during YAML validation.
The function compares values between the new config and the latest config YAML files to reject disk size shrinking during validation.