-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
✨ revert injection deprecation logging until internal injection code use stops #1382
Conversation
…code Signed-off-by: Eric Stroczynski <ericstroczynski@gmail.com>
@estroz: GitHub didn't allow me to request PR reviews from the following users: varshaprasad96. Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs. 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. |
Maybe offer an env var to turn them on? I'm very worried this deprecation is going to catch a lot of people unawares and I think we need to handle it better. |
The way I see it, as long as injection semantics are still around then no one will be the wiser or they'll have ample time to migrate to some other API. There are 2 categories of use for injection code:
Category 1 will get static linter warnings when using deprecated code, so they should be notified already of the deprecation. Category 2 will not and only see runtime warnings via the logs this PR is removing; however I don't think users in category 2 will be affected as long as controller-runtime can somehow inject fields into a manager, controller, or cluster post-construction, or be refactored to not need injection. I'm not saying I know what either of these things look like, but I doubt that injection semantics will go away completely given the complexity of the dependency chain of a manager and its constituents. I could be convinced of a need for an internal-only env variable for controller-runtime testing so this kind of issue doesn't crop up again (discussed in #1322), but this can only be added alongside a new API or refactor. |
I was unable to get linter warnings for this situation to work. The user is not calling anything, they are just declaring some methods. And yes, I didn't realize there was still usage inside controller-runtime, that should be removed ASAP. |
/lgtm I think we should keep the code for two releases after we removed all internal usages of this and re-added the logging though. |
Should we target v0.8 for this change, rather than v0.9 (main branch)? We should probably remove the internal usage within 0.9, if we still want to remove it by 0.10 |
I would vote for disabling it in both and only enable after c-r doesn't use this anymore |
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.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: estroz, vincepri The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@coderanger for some reason neither can I, and I'm 100% sure |
I doubt you can get linter warnings for implementing a deprecated interface, no one is directly calling the injector methods, that is the whole PITA about it |
Yep, that's my worry :) Because this is basically a "reverse function call", it will silently change behavior when the injector code is removed. |
#1322 adds a bunch of logs to injection functions, which are used heavily by the manager and controller builder and therefore unavoidably print for users who aren't using injection code themselves. Until internal usage is stopped, these logs should be removed and static deprecation notices solely used.
Signed-off-by: Eric Stroczynski ericstroczynski@gmail.com