konvoy-dp: start Envoy with minimal bootstrap config #85
Conversation
yskopets
changed the title
|
|
||
| var ( | ||
| // overridable by unit tests | ||
| newConfigFile = GenerateBootstrapFile |
There was a problem hiding this comment.
Isn't it more intuitive to pass it to the New method and just pass different implementation in tests? This way you don't need such comment in production code. Additionaly, I can't see it overriden in tests anywhere, am I wrong?
There was a problem hiding this comment.
Again, this is idiomatic Go.
Yes, it's not used at the moment, but at least designed to be unit testable.
There was a problem hiding this comment.
Can you point out some examples? I'd like to learn about it.
bonus points: outside k8s codebase.
| return err | ||
| } | ||
|
|
||
| ctx, cancel := context.WithCancel(context.Background()) |
There was a problem hiding this comment.
Interesting approach. I can see that cancel() fires SIGKILL signal on the Envoy process. I think we should do it more gracefully with SIGTERM and wait couple of seconds until all requests are processed. We don't have to do it now, we can create a task and do it later.
components/konvoy-control-plane/app/konvoy-dp/pkg/dataplane/envoy/envoy_test.go
Outdated
Show resolved
Hide resolved
components/konvoy-control-plane/app/konvoy-dp/pkg/dataplane/envoy/envoy_test.go
Outdated
Show resolved
Hide resolved
yskopets
force-pushed
the
feature/add-konvoy-dp
branch
from
b7d6f80
to
c7b13bf
Compare
yskopets
force-pushed
the
feature/add-konvoy-dp
branch
from
c7b13bf
to
167ca7a
Compare
changes:
konvoy-dptoolkonvoy-dp runcommand to start Envoy with minimal bootstrap configurationkonvoy-dpconfiguration