-
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?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: divysinghvi The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @divysinghvi. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Can one of the admins verify this patch? |
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.
I like the idea!
func warnDriverDeprecated(driverName string) { | ||
if driverName == "" { | ||
return | ||
} |
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
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.
func hyperkitDeprecationWarning(driverName string) { | ||
if !driver.IsHyperKit(driverName) { | ||
return | ||
for _, d := range registry.List() { |
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.
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:
d := registry.Driver(driverName)
if d.Name == "" {
// fail, no such driver
}
If d.Priority == registry.Deprecated {
// show warning
}
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.
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 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 comment
The 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.
|
||
// 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 comment
The 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 comment
The 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.
You can use alternative drivers: qemu2, vfkit, krunkit, parallels, docker, podman, ssh, hyperkit, virtualbox. this is a real result i did added hyperkit and virtualbox to it for testing on my local system rest they won't be visible since they will be deprecated
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.
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.
The new warning does not include links to the alternative drivers docs. Why did you remove the links? This is not required change for adding automated deprecated warnings. Please do not mix unrelated changes. If you want to remove the links please open an issue and we can discuss this there. |
@nirs thanks for the comment will keep in mind not to mix unrelated things , but i don't understand like https://minikube.sigs.k8s.io/docs/drivers/vfkit/ |
What do you mean by "I don't understand like .."?
The warning is generated - it is not static, so it should never include drivers which are not available on the current OS, and all the links to the docs will also be generated, so they will always be relevant to the current os. A DriverDef should include Name and DocURL. In this warning we should get list of alternative drivers, which should be drivers which are not deprecated or discouraged, and available on the current OS and architecture. For example when running on darwin/arm64 we should not recommend drivers which available only on amd64. So we should not have such a long list of alternative as you show in your example. We can generate the links form the Driver.DockURLs - in the same order of the alternative drivers. |
Thanks for explaining now i understand it way better i was a bit confused , and also the long list is also useless also since have the Driver.DockURLs it would be way easier to create docs link did not knew about that |
We were previously were creating warnings for each driver deprecated driver which was causing too much repetative code
warnDriverDeprecated() function automatically warns if the current driver is marked deprecated
Before
we had to create new function for each deprecated driver
After
automated warning for deprecated drivers
adds to #21631