feat: enable configuring a handler to listen to informers stopping#1493
feat: enable configuring a handler to listen to informers stopping#1493
Conversation
csviri
left a comment
csviri
left a comment
There was a problem hiding this comment.
LGTM,
would it be possible to add an integration test? (not sure how though)
I'm not sure either apart from what we already test in this PR. Note also that the informer part is already tested in the fabric8 client, we just need to test our integration, which I believe we do somewhat in this PR. |
Yep, no doubt this is good enough. Just meatn in a way if that would be easy to do, would not harm, |
|
cc @andreaTP |
| ConfigurationServiceProvider.instance().getInformerStoppedHandler() | ||
| .ifPresent(ish -> { | ||
| final var stopped = informer.stopped(); | ||
| if (stopped != null) { |
There was a problem hiding this comment.
in which condition stopped is going to be null ?
With the current implementation in the client it doesn't seem to be possible.
I would leave this check out to fail early if this erroneous situation appears.
There was a problem hiding this comment.
Good point. This happens during tests, though, where it would require quite a bit of setup just to mock the future…
There was a problem hiding this comment.
I would rather prefer more setup in the tests as opposed to the risk of this to happen and be swallowed in a real operator.
The risk here is to end up with a non-working callback without noticing.
If you are really, really against doing it, I should, at least, ask for a loud warning when this situation occurs .
There was a problem hiding this comment.
It's now explicit so there's no way to silently fail anymore.
vmuzikar
left a comment
vmuzikar
left a comment
There was a problem hiding this comment.
I played around with it with Keycloak Operator and can confirm it works well for our use-case. Thanks!
csviri
left a comment
csviri
left a comment
There was a problem hiding this comment.
just one tiny naming issue
|
|
||
| import io.fabric8.kubernetes.client.informers.SharedIndexInformer; | ||
|
|
||
| public interface InformerStoppedHandler { |
There was a problem hiding this comment.
Would rather rename it to InformerStopHandler ?
There was a problem hiding this comment.
I think the current name is better because it reflects the fact that the handler is called after the informer is stopped.
|
Kudos, SonarCloud Quality Gate passed! |
4 participants
Requires fabric8io/kubernetes-client#4412