Skip to content

Reject shrinking disk during YAML validation #3596

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.

Sign up for GitHub

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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Reject shrinking disk during YAML validation #3596

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5974ec1
reject shrinking disk during YAML validation
songponssw May 30, 2025
8cb23be
skip validation against lastest yaml if both are nil
songponssw May 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions cmd/limactl/edit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is same as saveRejectedYAML and should be deduplicated


if err := limayaml.ValidateYAMLAgainstLatest(yBytes, yContent); err != nil {
return saveRejectedYAML(yBytes, err)
}

if err := os.WriteFile(filePath, yBytes, 0o644); err != nil {
return err
}
Expand Down Expand Up @@ -171,3 +176,12 @@ func askWhetherToStart() (bool, error) {
func editBashComplete(cmd *cobra.Command, _ []string, _ string) ([]string, cobra.ShellCompDirective) {
return bashCompleteInstanceNames(cmd)
}

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)
Copy link
Preview

Copilot AI May 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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.

}
// TODO: may need to support editing the rejected YAML
return fmt.Errorf("the YAML is invalid, saved the buffer as %q: %w", rejectedYAML, origErr)
}
24 changes: 24 additions & 0 deletions pkg/limayaml/validate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "latest" mean here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

var l, n LimaYAML
var err error
if err = Unmarshal(yLatest, &l, "Unmarshal latest YAML bytes"); err != nil {
return err
}
if err = Unmarshal(yNew, &n, "Unmarshal new YAML byte"); err != nil {
return err
}

// Skip validation if both fields are unset.
if n.Disk == nil && l.Disk == nil {
return nil
}
Copy link
Preview

Copilot AI May 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
}
}
// 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.

nDisk, _ := units.RAMInBytes(*n.Disk)
lDisk, _ := units.RAMInBytes(*l.Disk)
if nDisk < lDisk {
return fmt.Errorf("field `disk`: shrinking the disk (%v --> %v) is not supported", *l.Disk, *n.Disk)
}

return nil
}
Loading