-
Notifications
You must be signed in to change notification settings - Fork 1.2k
🐛 Fix informer cache creating pointers to pointers #667
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
🐛 Fix informer cache creating pointers to pointers #667
Conversation
When the list type is already a pointer this code results in a cache type of **Type which can't be cast to runtime.Object. This adds a check to the function to only grab a pointer to a type if it is not already a ptr type.
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. I understand the commands that are listed here. |
Welcome @DewaldV! |
Hi @DewaldV. 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. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: DewaldV The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hmm, my org has a signed CLA so I expected that check to pass. I'll try and see what's up with that before proceeding here. |
/check-cla |
Ok, that should be good now. Apologies for the lapse, I had missed a step when linking my GH account to our org's. |
/assign @DirectXMan12 |
@DewaldV thanks for your PR. Could you add a test for this so we don't regress? And more of a comment, but pretty much all |
@alvaroaleman Happy to, I'll get another commit with a test done in a bit. |
Apologies for the delay on this, I've got a basic spike with the tests local but I've been delayed due to some work pressures. I'll try and get some time this week to get the test cleaned up and pushed. |
Updating double quotes in help text
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
hey, are you still interested in working on this |
Apologies for the delay on this. The testing turned out to be rather complicated as it required creating some CRDs with Lists that use pointers so I stalled in Nov and then it dropped off my radar. I'm going to put some time aside to get the tests finished up over the coming week so we can get this merged. We do still need it. |
@DewaldV I ended up needing as well, do you mind if I take your commit from here, add another one with a test on top of it and create a new PR so this gets done in a timely manner? |
@alvaroaleman I'm happy with that. Appreciate you jumping in on this and apologies for not getting this merged before. 😄 |
Closing this in favor of 796 /close |
@vincepri: Closed this PR. 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. |
Fixes #656
It appears the code in controller-runtime/pkg/cache/informer_cache.go tries to determine whether the items can be cast to runtime.Object and in the process assumes that the slice's elements are not pointers.
This means that if you have a List whose Items type is
[]*Item
the informer cache will create acacheTypeValue
of**Item
which then cannot be case toruntime.Object
resulting in an error of: