controllers/portforward: portforwards are run by the reconciler, reading off the engine state [ch11908] #4523
Conversation
| existing, isActive := r.activeForwards[name] | ||
| if isActive { | ||
| // We're already running this PortForward -- do we need to do anything further? | ||
| if equality.Semantic.DeepEqual(existing.Spec, desired.Spec) && equality.Semantic.DeepEqual(existing.ObjectMeta, desired.ObjectMeta) { |
There was a problem hiding this comment.
comparing the ObjectMeta like this isn't right...it's going to compare things you don't expect, like ResourceVersion
There was a problem hiding this comment.
Hmm fair. so the one thing i need to make sure stays the same is the ManifestName annotation -- would it be too weird to compare that directly? or should i duplicate that data on the state? or?
| return | ||
| } | ||
|
|
||
| if summary.IsLogOnly() { |
There was a problem hiding this comment.
I think your summary already has the list of PortForward names that have changed? can narrow this to len(summary.PortForwards) == 0
There was a problem hiding this comment.
yep will do that after I'm changing state via events (right now i'm just setting the PortForwards directly on the state for tests)
|
|
||
| for _, entry := range toStart { | ||
| // Treat port-forwarding errors as part of the pod log | ||
| ctx := logger.CtxWithLogHandler(entry.ctx, PodLogActionWriter{ |
There was a problem hiding this comment.
should probably use ObjectLogHandler instead of a custom one
There was a problem hiding this comment.
lol I just lifted this straight from the original code. will do.
I've moved the "actually run the port forwards" logic into the PortForwardReconciler (reading desired PortForwards off the EngineState). This shouldn't be touched in prod yet -- will make a separate PR to remove the "actually run the port forwards" logic from the subscriber.