⚠ Cache's ByObject is now a map Object -> options #2166
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
approved
vincepri
force-pushed
the
rework-cache-sanity
branch
from
acc97a2
to
00d618b
Compare
|
/assign @alvaroaleman @sbueringer |
| // ByObject offers more fine-grained control over the cache's ListWatch by object. | ||
| type ByObject struct { | ||
| // Label represents a label selector for the object. | ||
| Label labels.Selector |
There was a problem hiding this comment.
Why separate Label and Field selectors here, but use one ObjectSelector type for the DefaultSelector field above?
There was a problem hiding this comment.
This was a suggestion by @alvaroaleman, I'm happy to do either way, although to reduce verbosity of the ByObject struct, might make sense to keep it this way
There was a problem hiding this comment.
yeah IMHO this is easier to use when keeping them separate. I guess we could also do the same for the DefaultSelector
There was a problem hiding this comment.
As in, separate DefaultLabelSelector and DefaultFieldSelector?
| // [2] https://pkg.go.dev/k8s.io/apimachinery/pkg/fields#Set | ||
| Selectors SelectorsByObject | ||
| // ByObject offers more fine-grained control over the cache's ListWatch by object. | ||
| type ByObject struct { |
There was a problem hiding this comment.
This name is a little weird when it is used as cache.ByObject, should it be OptionsByObject?
There was a problem hiding this comment.
Can rename if we want to, the Struct is only used within the options, and was trying to reduce overall verbosity
There was a problem hiding this comment.
Maybe ObjectOptions? But no strong opining either way
There was a problem hiding this comment.
cache.Options{
ByObject: map[client.Object]cache.ByObject{
&corev1.Pod{}: {}
}
}IMO, this reads well and looks nice. @FillZpp if you feel that we should change it, happy to do it though
vincepri
force-pushed
the
rework-cache-sanity
branch
from
00d618b
to
66e07d5
Compare
vincepri
force-pushed
the
rework-cache-sanity
branch
6 times, most recently
from
0cb8f51
to
6e7c24c
Compare
This is a breaking change and revisits some of the previous changes we've made. It brings the options ByObject to a top level map, which now contains all the options the internal informer can be configured with. In addition, it removes the ObjectAll object, which was super confusing and only used to disable the deep copy for every object; instead it's now possible to disable deep copy through a top level option. Signed-off-by: Vince Prignano <vincepri@redhat.com>
vincepri
force-pushed
the
rework-cache-sanity
branch
from
6e7c24c
to
7e2dbaf
Compare
|
lgtm from my side |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, vincepri 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 |
This is a breaking change and revisits some of the previous changes we've made. It brings the options ByObject to a top level map, which now contains all the options the internal informer can be configured with. In addition, it removes the ObjectAll object, which was super confusing and only used to disable the deep copy for every object; instead it's now possible to disable deep copy through a top level option.
Signed-off-by: Vince Prignano vincepri@redhat.com