Create client for checking CRD deletion #1305
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
Just a few nits but otherwise lgtm |
k8s-ci-robot
added
size/M
The default client uses cache, and therefore, it might fail to notice if a CR is deleted. We instead use our own client who doesn't use the cache. Tested: make test-e2e
There was a problem hiding this comment.
/lgtm
/approve
/hold
/assign @rjbez17
Hey Ryan, since we can't reliably reproduce this problem, we don't have a test proving that it's fixed, but using an uncached client seems like the right thing to do in this case. Does this sound reasonable to you?
k8s-ci-robot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adrianludwin, GinnyJI 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
| // Create creates all reconcilers. | ||
| // | ||
| // This function is called both from main.go as well as from the integ tests. | ||
| func Create(mgr ctrl.Manager, f *forest.Forest, maxReconciles int) error { | ||
| func Create(mgr ctrl.Manager, f *forest.Forest, maxReconciles int, useFakeClient bool) error { |
There was a problem hiding this comment.
Since the client is created 100% by variables/info available outside this function, wouldn't it make more sense to just pass the client into this function instead of using a special bool to create a fake client or not?
There was a problem hiding this comment.
It's a good point, although in this context I'm not sure it's worth the added complexity. The interface and global variable would still have to be declared in this file, while the clients that implement it would now be in cmd/manager/main.go and internal/reconcilers/suite_test.go, so the logic would be fairly scattered. The way @GinnyJI has written it, pretty much everything you need to know is in the same place, and I think that simplicity is worthwhile.
If we start adding a bunch more options here, then I'd be open to refactoring it. wdyt?
|
/lgtm |
k8s-ci-robot
removed
the
do-not-merge/hold
The default client uses cache, and therefore, it might fail to notice if
a CR is deleted. We instead use our own client who doesn't use the
cache.
Tested: make test-e2e
Fix #1285