-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Automated generic deprecation warning #21689
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -309,7 +309,7 @@ func provisionWithDriver(cmd *cobra.Command, ds registry.DriverState, existing * | |
} | ||
|
||
virtualBoxMacOS13PlusWarning(driverName) | ||
hyperkitDeprecationWarning(driverName) | ||
warnDriverDeprecated(driverName) | ||
validateFlags(cmd, driverName) | ||
validateUser(driverName) | ||
if driverName == oci.Docker { | ||
|
@@ -414,17 +414,31 @@ func virtualBoxMacOS13PlusWarning(driverName string) { | |
`) | ||
} | ||
|
||
// hyperkitDeprecationWarning prints a deprecation warning for the hyperkit driver | ||
func hyperkitDeprecationWarning(driverName string) { | ||
if !driver.IsHyperKit(driverName) { | ||
// warnDriverDeprecated prints a small deprecation warning if the selected driver is deprecated | ||
func warnDriverDeprecated(driverName string) { | ||
if driverName == "" { | ||
return | ||
} | ||
out.WarningT(`The 'hyperkit' driver is deprecated and will be removed in a future release. | ||
You can use alternative drivers such as 'vfkit', 'qemu', or 'docker'. | ||
https://minikube.sigs.k8s.io/docs/drivers/vfkit/ | ||
https://minikube.sigs.k8s.io/docs/drivers/qemu/ | ||
https://minikube.sigs.k8s.io/docs/drivers/docker/ | ||
`) | ||
|
||
for _, d := range registry.List() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looping over all drivers is not needed, since we have the driver name. I think we better get the driver from the registry and check if the driver is deprecated. Can we done using:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that with driverName "" we will get a DriverDef with empty name, and this is not an error, so we should not fail, just return. The check for valid driver should be done before we call this function, for example if a user makes a typo in the driver name, or the driver is not available on this OS. |
||
if d.Priority != registry.Deprecated { | ||
continue | ||
} | ||
|
||
// match driver name | ||
if d.Name == driverName { | ||
out.WarningT("The '{{.name}}' driver is deprecated and will be removed in a future release.\n You can use alternative drivers: {{.drivers}}.", out.V{"name": d.Name, "drivers": strings.Join(driver.SupportedDrivers(), ", ")}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SupportedDrivers() return results based on current OS? Is it sorted by priority? Previously we used: You can use alternative drivers such as 'vfkit', 'qemu', or 'docker' This is the wanted order, using driver priority (vfkit is the preferred driver) and preferring vm drivers since on macOS all drivers need a vm. We want to keep this order. If SupportedDrivers() does not return the drivers in the right order for the current OS, we need to improve it before using it here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is it is priority sorted The 'hyperkit' driver is deprecated and will be removed in a future release. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The order is wrong - vfkit is the preferred driver and should be first, not qemu. This is the native driver for macOS. qemu on macOS is poorly maintained and should not be the preferred driver. Also the hyperkit driver appears in the list of alternatives which is wrong - the hyperkit driver is not an alternative to itself. We need to create AlternativeDrivers() function - SupportedDrivers() is not the right way. |
||
return | ||
} | ||
|
||
// match any alias | ||
for _, a := range d.Alias { | ||
if a == driverName { | ||
out.WarningT("The '{{.name}}' driver is deprecated and will be removed in a future release.\n You can use alternative drivers: {{.drivers}}.", out.V{"name": d.Name, "drivers": strings.Join(driver.SupportedDrivers(), ", ")}) | ||
return | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does not make sense to iterate over all drivers and check both driver name and all aliases. The registry keeps a map of drivers by name and alias, so getting a driver should be a single map lookup using the name and if not found a second map lookup using the alias. |
||
} | ||
} | ||
} | ||
|
||
func validateBuiltImageVersion(r command.Runner, driverName string) { | ||
|
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.
empty driver means the driver was not selected by the user. Do we want to warn only about driver selected by the user?
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.
My bad i don't think warning for empty driver should be part of this function
Uh oh!
There was an error while loading. Please reload this page.
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.
sure, we should not warn about empty driverName, and the check achieve this. But can get this for free by getting the driver and return on missing driver.