controller: add TriggeredBy custom watches to engine #535
Conversation
sttts
changed the title
|
@sttts This LGTM, but can you update the PR description with some context as to why we want this. Any context on how it's been tested would also help. |
|
Added context. |
pkg/controller/engine.go
Outdated
| // TriggeredBy returns a custom watch for secondary resources triggering the | ||
| // controller. Events will be handled by the supplied EventHandler, and may be | ||
| // filtered by the supplied predicates. | ||
| func TriggeredBy(cache cache.Cache, kind client.Object, h handler.EventHandler, p ...predicate.Predicate) Watch { |
There was a problem hiding this comment.
I assume it was tested e2e by https://github.com/crossplane/crossplane/pull/4582/files#diff-52e214a3e1aa3741fcbc3dd3faca4e6ae54e98b177690300591a4d147294a70dR416 ?
Do we want to extend unit tests at https://github.com/crossplane/crossplane-runtime/blob/master/pkg/controller/engine_test.go ?
There was a problem hiding this comment.
The engine is basically untested today. It only checks some errors.
There was a problem hiding this comment.
ok, so maybe we should separately track extending unit test coverage? I'll leave it up to you and not hold this one :)
There was a problem hiding this comment.
I tried now for 20min. You mock yourself to death with cache, informer, controller. At the end, you are not sure anymore what you are testing. There is a reason this is only lightly tested 🤷
There was a problem hiding this comment.
@sttts got it, thanks a lot for checking! 👍
There was a problem hiding this comment.
Yeah, this whole package is kinda just syntactic sugar. 😬
|
Thanks @sttts! The use case makes sense to me now. |
|
What is missing to merge this? |
sttts
force-pushed
the
sttts-engine-triggered-by
branch
from
fb1419c
to
d084614
Compare
Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>
sttts
force-pushed
the
sttts-engine-triggered-by
branch
from
d084614
to
eb74932
Compare
| // controller. source.Kind can be used to create a source for a secondary cache. | ||
| // Events will be handled by the supplied EventHandler, and may be filtered by | ||
| // the supplied predicates. | ||
| func TriggeredBy(source source.Source, h handler.EventHandler, p ...predicate.Predicate) Watch { |
There was a problem hiding this comment.
@negz see that I slightly changed the signature. It's more flexible now.
Before: TriggeredBy(someCache, kind, h, p)
Now: TriggeredBy(source.Kind(someCache, kind), h, p)
Description of your changes
Added
controller.TriggeredByto create controller engine watches based on a different informer cache (the one from outside).This is helpful e.g. to trigger the compositor reconciling XRs through changes on
CompositionRevision. The compositor has its own informer cache because it is dynamically started for each XR kind, done using the controller engine. This kind of controllers though today can only watch objects in the same cache. This PR adds a way to inject watches from another cache, e.g. the top-level cache watching theCompositionRevisionkind.I have:
make reviewable testto ensure this PR is ready for review.How has this code been tested
This has been tested as part of crossplane/crossplane#4582, manually for the demo in that PR, plus I am working on tests for it.