install watch reaction function to fake client#550
install watch reaction function to fake client#550AshishNaware wants to merge 1 commit intoreconcilerio:mainfrom
Conversation
AshishNaware
force-pushed
the
support_watch_reactions
branch
from
c189e97 to
c533767
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #550 +/- ##
==========================================
- Coverage 58.93% 58.85% -0.09%
==========================================
Files 33 33
Lines 2981 2987 +6
==========================================
+ Hits 1757 1758 +1
- Misses 1130 1134 +4
- Partials 94 95 +1 ☔ View full report in Codecov by Sentry. |
|
Hi @AshishNaware. Thanks for the contribution. This change is heading in the right direction, but is also incomplete as it has no runtime effect. Let me know if you need any pointers into the code base. |
Hey. Thanks for the feedback. Let me dig more into the code-base. I will create a thread here if i have any questions :) |
|
@scothis - I just took a quick glance at the code and I see a Watch method already implemented for duckAwareClientWrapper that returns a watch.Interface. I assume that we need a similar implementation for the clientWrapper as well. thoughts? |
Yep. This is also where the watch reaction funcs should be applied. |
AshishNaware
force-pushed
the
support_watch_reactions
branch
from
c533767 to
16e49a3
Compare
Signed-off-by: Ashish Naware <ashishnaware3@gmail.com>
AshishNaware
force-pushed
the
support_watch_reactions
branch
from
a28e461 to
9f79887
Compare
|
Hi @scothis - Sorry for the delay, I was busy with some other high priority tasks. Could you please review this again? I haven't written any tests for above changes. do you have any suggestions on that? |
scothis
left a comment
scothis
left a comment
There was a problem hiding this comment.
It's fine to skip watches for the sub resource clients. Writing tests will be important here since this code won't be exercised in any other way. A good test will show how you expect a Watch/WatchReactor to actually be used.
| err = w.reactWatcherFunc(clientgotesting.NewGetAction(gvr, namespace, name)) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| err = w.reactWatcherFunc(clientgotesting.NewCreateAction(gvr, namespace, list)) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| err = w.reactWatcherFunc(clientgotesting.NewDeleteAction(gvr, namespace, name)) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| err = w.reactWatcherFunc(clientgotesting.NewUpdateAction(gvr, namespace, list)) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Why are we trying to react to get/create/delete/update here? I would expect it to only react with watch actions. I'm not familiar enough with the mechanics of watch to know exactly what should be here.
| // Request identifies the object to be reconciled | ||
| Request reconcilers.Request | ||
| // WithReactors installs each ReactionFunc into each fake clientset. ReactionFuncs intercept | ||
| // WithReactors and WithWatchReactors installs each ReactionFunc into each fake clientset. ReactionFuncs intercept |
There was a problem hiding this comment.
document each field separately, otherwise the info won't show up in tooling for WithWatchReactors.
x3
| WithReactors []ReactionFunc | ||
| // WithWatchReactors installs WatchReactionFunc into the fake clientset. This provides ability | ||
| // to simulate events in the watcher. | ||
| WithWatchReactors []WatchReactionFunc |
There was a problem hiding this comment.
May also need to define WithWatchReactors for (Sub)ReconcilerTestCase/AdmissionWebhookTestCase and pass it through when creating the ExpectConfig. Then again, I'm not sure watch makes sense in those particular contexts since reconcilers and webhooks are intended to execute at a moment in time while watches are long lived observations of change. What do you think?
2 participants
This PR adds ability to install watch reaction function to the fake client. This will help in writing tests where its required to simulate behavior of the watcher.