✨ Deprecate Inject interface

[varshaprasad96]

Deprecate inject interface

cc: @estroz

[k8s-ci-robot]

Hi @varshaprasad96. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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/test-infra repository.

[coderanger]

Might want to explain a bit about why it is being deprecated?

[varshaprasad96]

@vincepri @alvaroaleman : Have only added a comment as a deprecation warning above the interface. Is there logging required anywhere else?

[vincepri]

@coderanger The code injection code is going to be deprecated and removed in a future code, it's hard to maintain this codebase and it's highly error prone for users. In addition, when making breaking changes users might not see any compile time errors and some code might start to fail silently, which isn't great.

@varshaprasad96 In terms of deprecation, we'd want to deprecate all of the injectors, not just the client one. We should also add a log line when any of the injection paths is used to warn users that this functionality is going to be removed in v0.10.x

[vincepri]

/ok-to-test

[vincepri]

/retest

[coderanger]

I would strongly recommend pulling the log into a helper function and making an env var that can be set on CI to turn these into fatal errors. Normal go test runners don't show output unless the test fails.

[estroz]

@coderanger defining an env var in the manner you suggest seems like a rather heavy-handed approach to a simple deprecation, which I believe has been done in the past just fine.

[varshaprasad96]

/retest

[coderanger]

@estroz Why not make it better though? Makes it much easier to check if you are clear on deprecation warnings if you can DEPRECATIONS_AS_ERRORS=1 make test and call it done :)

[estroz]

@coderanger that's what linters are for, which will fail if deprecated functions/packages are used.

[coderanger]

@estroz That works too, but is a lot more work to write a custom kubebuilder linter. Compared to make a logDeprecation() helper function somewhere.

[estroz]

golangci-lint will do this for you with staticcheck. Regardless the feature you suggest should be done in a separate PR. This PR should only contain log messages and a deprecation comment IMO.

[coderanger]

I'm not sure that would work since the calls for these functions are not in user code, but I'll try and make a repro and see.

[alvaroaleman]

@varshaprasad96 can you rename the pull to clarify that this is about deprecating all the Inject functionality and not just the client?

[DirectXMan12]

For next time we do this: there should be a corresponding issue or something, and as coderanger noted, there needs to be a more in-depth explanation in the PR & commit messages -- "deprecate XYZ" is insufficient. People need to be able to look back on PRs and issues to figure out what is going on.

[hiddeco]

For next time we do this: there should be a corresponding issue or something, and as coderanger noted, there needs to be a more in-depth explanation in the PR & commit messages -- "deprecate XYZ" is insufficient.

Unsolicited consumer advice / perspective:

Tracing this back was actually pretty easy for us, but we do keep a close eye on reading the changelog to understand what's changed. The fact that we were unable to do anything about it, other than commenting here that we ran into deprecation notices we could not do anything about was however not pleasant to our end-users.

So IMHO the issue is/was not with (the ability of) looking back, but looking forward to the impact of changes, so that changes are made in stages (or all at once) without having an impact on consumers.

Lastly, thanks all involved for rolling out the patch release, we have updated most of our controllers today. 🌻

[DirectXMan12]

@hiddeco yeah, totally agree on the impact of this change (log messages passed on to end users w/o any fix). This was not our best moment, unfortunately, but we'll certainly try to do better in the future.

My comment re: explanation is mainly for folks looking back in the future as to why something was changed. We certainly also need forward-looking impact too though :-).

[varshaprasad96]

I'm sorry for the inconvenience caused here. I agree, this PR was premature, and doesn’t explain the reason for deprecating the functionality either. I am starting a discussion on this in the issue, so that we can plan the next steps on removing the inject interface usage internally first and then remove it later for the users.