-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
✨ Create a supported way of setting a cache on source.Kind #794
Conversation
- sourceWithFixedCache does not implement InjectCache - it does not rely on the current implicit implementation that a cache is not overwriting in case it was injected before - see #650 Co-authored-by: Ralf Pannemans <ralf.pannemans@sap.com>
Co-authored-by: Ulrich Kramer <u.kramer@sap.com>
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 @jkbschmid! |
Hi @jkbschmid. 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. |
/check-cla |
/check-cla |
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.
Generally looks good, what becomes a bit weird now though is that some sources get initialized by allocation a var and others by using a constructuctor, IMHO consistenly using one of the two would be better.
/ok-to-test
pkg/source/source.go
Outdated
// NewKindWithFixedCache creates a Source without InjectCache, so that it is assured that the given cache is used | ||
// and not overwritten. It can be used to watch objects in a different cluster by passing the cache | ||
// from that other cluster | ||
func NewKindWithFixedCache(object runtime.Object, cache cache.Cache) Source { |
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.
func NewKindWithFixedCache(object runtime.Object, cache cache.Cache) Source { | |
func NewKindWithCache(object runtime.Object, cache cache.Cache) Source { |
pkg/source/source.go
Outdated
@@ -55,6 +55,22 @@ type Source interface { | |||
Start(handler.EventHandler, workqueue.RateLimitingInterface, ...predicate.Predicate) error | |||
} | |||
|
|||
// NewKindWithFixedCache creates a Source without InjectCache, so that it is assured that the given cache is used |
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.
// NewKindWithFixedCache creates a Source without InjectCache, so that it is assured that the given cache is used | |
// NewKindWithCache creates a Source without relying on InjectCache, so that it is assured that the given cache is used |
Minor comments, otherwise lgtm |
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.
agree with vincepri comments otherwise lgtm
agreed with @alvaroaleman - if all a constructor doing here is instantiating a struct, I don't think we should use one. Why can't this be a public struct, or why can't we just change Cache to be exported as well? controller-runtime/pkg/source/source.go Line 80 in f515d1e
Furthermore, if that's the direction we're going, we should probably embed the struct into kindWithCache rather than creating an entirely new struct. The only difference between the two is the extra bytes for the reference. Simplifying this enables us to create a DefaultKind that in effect preserves the existing functionality, rather than new structs and new constructors. |
/hold |
I just read the linked thread and while I think we should look for a cleaner long term solution, I understand the problem and the reason we have this solution. /hold cancel Still a little concerned since it's going to be harder to remove this API once people start depending on it. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gerred, jkbschmid 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 |
But then we would end up with a struct that cannot be used until the cache is set. You would have to document this. With the constructor, it is safe to use and you cannot use it in a wrong way. |
I agree with that. Maybe we can add constructors for all sources and mark the direct usage of the structs as deprecated, so this is consistent. |
@@ -55,6 +55,22 @@ type Source interface { | |||
Start(handler.EventHandler, workqueue.RateLimitingInterface, ...predicate.Predicate) error | |||
} | |||
|
|||
// NewKindWithCache creates a Source without InjectCache, so that it is assured that the given cache is used |
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.
Nit: the first sentence of this godoc is basically for ppl who know how its implemented and not explaining the purpose which is far more important. It would be nice to just say sth like NewKindWithCache is used to create a Wach for a different cluster
, then go on with the implementation details like "this is achieved by intentionally not implementing InjectCache" etc. Would be great if someone could create a followup for that.
/lgtm
/retest We should probably throw more CPU at this job and hope that stabilizes it. |
Create a wrapper for Kind without
InjectCache
to create a supported way of setting a cache on source.Kind. This can used, e.g., for watching resources on another cluster.fixes #650