Skip to content
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

Call limadriver.Validate as part of limayaml.Validate #2512

Open
jandubois opened this issue Jul 25, 2024 · 2 comments
Open

Call limadriver.Validate as part of limayaml.Validate #2512

jandubois opened this issue Jul 25, 2024 · 2 comments

Comments

@jandubois
Copy link
Member

          > They can return errors in validation, instead of warnings?

@afbjorklund Yes, they can. The warning comes from func (l *LimaVzDriver) Validate() error { and just write to the log, but don't return an error:

switch audioDevice := *l.Yaml.Audio.Device; audioDevice {
case "":
case "vz", "default", "none":
default:
logrus.Warnf("field `audio.device` must be \"vz\", \"default\", or \"none\" for VZ driver, got %q", audioDevice)
}

I think all the warning should return an error instead (but in a separate PR).

I also think limaDriver.Validate() should be called as part of limayaml.Validate() or through some wrapper that will call both. Calling it as part of instance.Prepare() is too late.

Originally posted by @jandubois in #1951 (comment)

@jandubois
Copy link
Member Author

An alternative would be to move most or all of the driver-based validation into limayaml.Validate.

The logic is already leaking into that function e.g. in #1951 in the code that selects the default vmType based on all the other settings. So the knowledge about which settings are compatible with which driver already lives there. Moving the rest of the driver validation into the same function just consolidates all of it into a single place.

I understand that conceptually having a separate per-driver validation function is desirable if/when we ever support external (non-builtin) drivers, but we don't have them currently.

@jandubois
Copy link
Member Author

I just noticed that some driver-specific settings are not validated at all. E.g.

$ limactl start --vm-type qemu --set .rosetta.enabled=true
? Creating an instance "default" Proceed with the current configuration
INFO[0001] Starting the instance "default" with VM driver "qemu"
…

$ yq .rosetta ~/.lima/default/lima.yaml
# Enable Rosetta for Linux (EXPERIMENTAL).
# Hint: try `softwareupdate --install-rosetta` if Lima gets stuck at `Installing rosetta...`
# 🟢 Builtin default: false
enabled: true

will not even show a warning; the Rosetta setting is just silently ignored by the QEMU driver.

I think this is just another argument in favour of merging all validation into limayaml.Validate().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant