reconcile api objects instead of state objects [ch12124] #4613
Conversation
| var _ store.TearDowner = &Reconciler{} | ||
| var _ reconcile.Reconciler = &Reconciler{} | ||
|
|
||
| func NewReconciler(store store.RStore, kClient k8s.Client, ctrlClient ctrlclient.Client) *Reconciler { |
There was a problem hiding this comment.
You won't be able to take ctrlclient.Client as a constructor param, you have to let it be "injected" via SetClient(). (Your Reconcile() method won't be called before that though, so you don't have to worry about it being nil.)
There's a weird chicken-and-egg party going on between the reconcilers + the controller manager (manager creates client but not until it's actually initialized/running which is after the clients get constructed), see #4250 for how we ended up here.
| toShutdown = r.addToShutdown(toShutdown, existing) | ||
| if active, ok := r.activeForwards[name]; ok { | ||
| if equality.Semantic.DeepEqual(active.Spec, pf.Spec) && | ||
| equality.Semantic.DeepEqual(active.ObjectMeta.Annotations, pf.ObjectMeta.Annotations) { |
There was a problem hiding this comment.
FWIW I think comparing spec + just the v1alpha1.AnnotationManifest annotation (rather than all) from the old code is a bit less error-prone in the event that some{one,thing} starts manipulating annotations across resources.
| // a. the PFs are started as expected (r.activeForwards etc.) and | ||
| // b. the status is reflected in the API | ||
| time.Sleep(time.Second) | ||
| f.requireState(pfFooName, func(pf *PortForward) bool { |
There was a problem hiding this comment.
I think the time.Sleep() can go away?
Also, this needs a real assertion 😬
There was a problem hiding this comment.
lol that was supposed to go away 😅
| } | ||
| } | ||
|
|
||
| func (f *pfrFixture) onChange() { | ||
| f.r.OnChange(f.ctx, f.st, store.LegacyChangeSummary()) | ||
| func (f *pfrFixture) wait() { |
There was a problem hiding this comment.
TBH I think the tests are likely to suffer from timing failures from this. I think it might be better to replace this method with something like requirePfStarted(name string) which uses requireState() to check that !pf.Status.StartedAt.IsZero(); i.e. use it as control flow/synchronization and then do normal assertions after.
There was a problem hiding this comment.
hm yeah given that i haven't yet implemented status yet, i'm just doing "you can get the port forward back from the API server" which is maybe not THE most robust but better than the existing, wdyt?
|
@milas this is ready for you when you get a chance |
Hello @milas,
Please review the following commits I made in branch maiamcc/reconcile-pfs:
ec728c3 (2021-06-07 11:30:24 -0400)
reconcile in response to API changes and update tests
66be159 (2021-06-04 19:34:47 -0400)
preserve old behavior
c133e96 (2021-06-04 19:24:28 -0400)
wip
Code review reminders, by giving a LGTM you attest that: