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
✨ Deprecate Inject interface #1322
✨ Deprecate Inject interface #1322
Conversation
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 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/test-infra repository. |
Might want to explain a bit about why it is being deprecated? |
@vincepri @alvaroaleman : Have only added a comment as a deprecation warning above the interface. Is there logging required anywhere else? |
4cd3d25
to
1a7dc3a
Compare
@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 |
1a7dc3a
to
ae59b64
Compare
ae59b64
to
92e5147
Compare
/ok-to-test |
/retest |
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. |
92e5147
to
64eb710
Compare
@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. |
/retest |
@estroz Why not make it better though? Makes it much easier to check if you are clear on deprecation warnings if you can |
@coderanger that's what linters are for, which will fail if deprecated functions/packages are used. |
@estroz That works too, but is a lot more work to write a custom kubebuilder linter. Compared to make a |
|
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. |
@varshaprasad96 can you rename the pull to clarify that this is about deprecating all the Inject functionality and not just the client? |
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. |
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. 🌻 |
@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 :-). |
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. |
…code Signed-off-by: Eric Stroczynski <ericstroczynski@gmail.com>
Deprecate
inject
interfacecc: @estroz