✨ revert injection deprecation logging until internal injection code use stops

[estroz]

#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

[estroz]

/cc @varshaprasad96 @vincepri @alvaroaleman

[k8s-ci-robot]

@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:

/cc @varshaprasad96 @vincepri @alvaroaleman

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.

[coderanger]

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.

[estroz]

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:

1. Users directly vendoring the injection library
2. Code internal to controller-runtime

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.

[coderanger]

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.

[alvaroaleman]

/lgtm
/assign vincepri

I think we should keep the code for two releases after we removed all internal usages of this and re-added the logging though.

[vincepri]

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

[alvaroaleman]

I would vote for disabling it in both and only enable after c-r doesn't use this anymore

[vincepri]

/approve

[k8s-ci-robot]

[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:

• OWNERS [vincepri]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[estroz]

I was unable to get linter warnings for this situation to work

@coderanger for some reason neither can I, and I'm 100% sure golangci-lint used to do this. I'm not sure if its golangci-lint version, but the staticcheck linter, which warns of deprecations, isn't running at all.

[alvaroaleman]

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

[coderanger]

Yep, that's my worry :) Because this is basically a "reverse function call", it will silently change behavior when the injector code is removed.