⚠ Refactor cache.Options, deprecate MultiNamespacedCacheBuilder #2157
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
k8s-ci-robot
added
approved
|
/assign @alvaroaleman @sbueringer |
| func selectNamespace(def, override string) string { | ||
| if override != "" { | ||
| func selectNamespaces(def, override []string) []string { | ||
| if len(override) > 0 { |
There was a problem hiding this comment.
Do we have to check for nil here instead? Otherwise you can't overwrite with an empty slice. Or is that not a relevant use case?
There was a problem hiding this comment.
Wouldn't both nil and 0 have the same effect?
There was a problem hiding this comment.
The last time I checked this it was a difference. Let me double-check
There was a problem hiding this comment.
There was a problem hiding this comment.
I think that works as I would expect, the default is a nil slice, which should never overwrite anything. Only slices with at least one member should, so it works ~like before
There was a problem hiding this comment.
Yup that's why I asked. Do we ever want to overwrite an existing value with an empty slice (which would then lead to us using all namespaces).
But I guess in that case folks can also just overwrite it with a slice with the All value
vincepri
force-pushed
the
refactor-cache-opts
branch
from
ed924c7 to
1ea2fc6
Compare
This changeset refactors the entire cache.Options struct to provide a legible and clear way to configure parameters when a cache is created. In addition, it handles under the hood the automatic multi-namespace cache support we currently offer through the MultiNamespacedCacheBuilder, which is now deprecated. Signed-off-by: Vince Prignano <vincepri@redhat.com>
vincepri
force-pushed
the
refactor-cache-opts
branch
from
1ea2fc6 to
915b85d
Compare
|
/lgtm We can follow-up if someone has a different opinion on #2157 (comment) |
k8s-ci-robot
added
the
lgtm
|
/retest |
There was a problem hiding this comment.
While its nice to see some work getting done in here I would have much preferred if we had started off with some kind of design doc that includes what options we want to end up supporting with the cache and how we configure them. Right now we are going back to changing in to whatever seems best at the moment without any clear vision on how this should look in the end.
I also feel we are getting a bit too careless with doing breaking changes, they have a cost for many of our users and we should avoid doing them unless they clearly and obviously improve things.
| DefaultTransform toolscache.TransformFunc | ||
|
|
||
| // ByObject restricts the cache's ListWatch to the desired fields per GVK at the specified object. | ||
| ByObject ViewByObject |
There was a problem hiding this comment.
I don't really understand how it improves anything to move this into a substruct, IMHO this just makes it harder to find what options exist.
What might have improved things would be if this was the object -> object-level options map rather than a struct that embedds a bunch of settings as maps.
| // list objects per GVK at the specified object. | ||
| // Be very careful with this, when enabled you must DeepCopy any object before mutating it, | ||
| // otherwise you will mutate the object in the cache. | ||
| UnsafeDisableDeepCopy DisableDeepCopyByObject |
There was a problem hiding this comment.
This doesn't really have anything to do with viewing objects
There was a problem hiding this comment.
I'll follow-up and move this one out
| Namespace string | ||
| // View restricts the cache's ListWatch to the desired fields per GVK | ||
| // Default watches all fields and all namespaces. | ||
| View ViewOptions |
There was a problem hiding this comment.
I am really, really not a fan of moving things into substructs. It is much easier to find what options exist if they are on the top level.
There was a problem hiding this comment.
Do we have a better name? Having all of the selectors and transformers at the top level IMO it's super confusing for new users. It's also not clear at all that all Get() and List() requests would be limited to those for example
This changeset refactors the entire cache.Options struct to provide a legible and clear way to configure parameters when a cache is created. In addition, it handles under the hood the automatic multi-namespace cache support we currently offer through the
MultiNamespacedCacheBuilder, which is now deprecated.
Signed-off-by: Vince Prignano vincepri@redhat.com