-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Avoid usage of newly deprecated webhook.Validator and webhook.Defaulter interfaces #3721
Comments
cc: @alvaroaleman |
Kubebuilder is a separate project owned by a different set of people. /transfer kubernetes-sigs/kubebuilder |
@alvaroaleman: Something went wrong or the destination repo kubernetes-sigs/kubernetes-sigs/kubebuilder does not exist. In response to this:
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/test-infra repository. |
/transfer kubebuilder |
@alvaroaleman thank you for moving this! I thought I made it in kubebuilder but I must have mixed my tabs up. |
Thank you for raising this one. It would be amazing if we could track issues to fix those breaking changes and desecrations in controller-runtime always. |
What is required to do here: 1 - Replace The change must be done for the docs/examples and default scaffold for the go/v4 plugin only. TIP Please, change first the default scaffold under pkg/plugins/golang/v4/ (i.e.
make generate then, see that the code used in the tutorial as the samples under testdata will be automatically updated with the latest changes. Then, after that ensure that the documentation and steps are either valid after the changes and supplement it as fit.
|
Hi, would love to provide a PR for this, if it's not yet taken. Looking at your comment @camilamacedo86 and the code base, it seems like a pretty straight forward change. However, there haven't been any changes to the generated files since last year, meaning all generated files will be touched (2023 -> 2024). Would be happy to provide a separate PR just updating the timestamps if that better suits the workflow here. Let me know what you prefer. |
Hi @jonas-jonas,
That is a good point. Please feel free just to run make, generate and push a PR if that is not done yet and ping me out. I am more than happy to approve this one. Also, feel free to push the PR for remove the deprecation and ping me out as well. |
Sorry I didn't specify in the original post. We need to implement the interface on some type outside of the API packages and update the docs to warn about the dangers of implementing this in the API packages. The purpose of this change is to reduce coupling between the APIs package and controller-runtime code. See kubernetes-sigs/controller-runtime#2596 for context. |
@jonas-jonas, please check the comment above. I will add some comments to the PR as well. |
@camilamacedo86 we probably need somebody from the kubebuilder team to let @jonas-jonas know what is the appropriate place to implement this interface so that it is not part of the API packages which are commonly imported by other projects. |
This comment was marked as outdated.
This comment was marked as outdated.
Hi @nathanperkins,
I understand that we have two issues/tasks here.
Unless I misunderstand something, we do not need to change the layout to no longer use deprecated. c/c @varshaprasad96 ^ |
Totally reasonable, in that case the PR is fine from my perspective! |
The PR for controller-runtime to remove the deprecated interfaces is kubernetes-sigs/controller-runtime#2877. Linking to this issue if more discussion is necessary. |
What do you want to happen?
In this controller-runtime issue, it was decided to deprecate the webhook.Validator and webhook.Defaulter interfaces, due to issues they cause with project upgrades by requiring controller-runtime import in API packages. Those functions are planned to be removed later.
We should update the Kubebuilder generated code and the docs to use admission.CustomValidator and admission.CustomDefaulter instead.
Jan 13th edit:
We also need to implement this interface outside of the API packages so that the API packages are not coupled with controller-runtime versions (see the issue above for context). The docs should be updated to warn the reader about the dangers of implementing this on the API types.
Extra Labels
No response
The text was updated successfully, but these errors were encountered: