Bake backend config into ServicePort #285
Conversation
k8s-ci-robot
added
size/M
| @@ -39,6 +40,7 @@ type ServicePort struct { | |||
| Protocol annotations.AppProtocol | |||
| SvcTargetPort string | |||
| NEGEnabled bool | |||
| BackendConfig *backendconfigv1beta1.BackendConfig | |||
There was a problem hiding this comment.
I don't think it's necessary to make this a pointer. We never will need to modify it.
There was a problem hiding this comment.
One idea to make this a pointer is that we can distinguish if a ServicePort has any backendConfig associated with it.
There was a problem hiding this comment.
Okay, we can leave it for now, but we should evaluate whether we really need it. I'd like to avoid having a pointer unless we actually have to modify the object.
There was a problem hiding this comment.
That sounds good, thanks for the quick review!
| podLister cache.Indexer | ||
| endpointLister cache.Indexer | ||
| backendConfigLister cache.Indexer | ||
| negEnabled bool |
There was a problem hiding this comment.
Instead of passing in all these Indexer's, we should just pass in the controller context and create the listers when we need them. So translator should be boiled down to just:
type Translator struct {
namer utils.Namer
ctx *context.ControllerContext
}
There was a problem hiding this comment.
Sounds good, passed in ctx instead.
MrHohn
force-pushed
the
empty-backendconfig-workflow
branch
from
ec1011c
to
3195a54
Compare
k8s-ci-robot
added
size/L
| negEnabled bool | ||
| namer *utils.Namer | ||
| ctx *context.ControllerContext | ||
| negEnabled bool |
There was a problem hiding this comment.
You can do this in a separate PR, but we can also put a flag that indicates whether NEG is enabled in the ContollerContext.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/assign @nicksardo |
Thinking it might be great to separate this out of the actual backend service feature implementation. @rramkumar1 Feel free to close this if you have code changes ready, I remembered you were doing something similar...
@bowei