-
Notifications
You must be signed in to change notification settings - Fork 493
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 optional resource deletion for collector CR #3494
Fix optional resource deletion for collector CR #3494
Conversation
gvk, err := apiutil.GVKForObject(l, cl.Scheme()) | ||
if err != nil { | ||
return nil, err | ||
} | ||
list.SetGroupVersionKind(gvk) | ||
err = cl.List(ctx, list, options...) | ||
gvk.Kind = fmt.Sprintf("%sList", gvk.Kind) |
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.
This feels really stupid, but I couldn't find an idiomatic way to get a xxxList
instance given an xxx
instance.
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.
FYI, if I go back to the previous version using Unstructured
, then the cache doesn't work even if I explicitly enable unstructured caching in the client. I think I'd need to change the indexer as well.
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.
argh that's incredibly frustrating, should we ask the kubernetes folks if there's a better way to do this? This just feels very flimsy to me... like what if an object doesn't support List
for some reason?
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.
For what it's worth, controller-runtime itself does these kinds of things as well: https://github.com/kubernetes-sigs/controller-runtime/blob/f529320b4c0fa5205067dec4eaeabc3c6cf62d25/pkg/client/client.go#L230.
And if the object doesn't support List
, well, the test won't pass. We could even add a test with a fake client to verify.
I would like a solution without these kinds of hacks, but I'm losing hope that it's actually possible.
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.
yeah it's a huge bummer.
96d5c1b
to
8bc20d1
Compare
8bc20d1
to
7b43d08
Compare
7b43d08
to
53fb524
Compare
gvk, err := apiutil.GVKForObject(l, cl.Scheme()) | ||
if err != nil { | ||
return nil, err | ||
} | ||
list.SetGroupVersionKind(gvk) | ||
err = cl.List(ctx, list, options...) | ||
gvk.Kind = fmt.Sprintf("%sList", gvk.Kind) |
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.
argh that's incredibly frustrating, should we ask the kubernetes folks if there's a better way to do this? This just feels very flimsy to me... like what if an object doesn't support List
for some reason?
1fcfe5e
to
8cc6d12
Compare
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.
This LGTM, but I would like some other approvers / maintainers review on this too
8cc6d12
to
e78208a
Compare
Description:
I've refactored and cleaned up our owned resource tracking:
Link to tracking Issue(s):
Testing: