ExecWithOption - get rid of stdout/stderr buffers #2506
Conversation
There was a problem hiding this comment.
The change makes sense.
@pavannd1 @viveksinghggits but I wonder if we have any process (e.g. versioning) to notify the user of public API changes?
Output could be obtained by writers passed via `ExecOptions`
e-sumin
force-pushed
the
exec-get-rid-of-buffer
branch
from
ed20ccd to
8630a46
Compare
| func ExecWithOptions(kubeCli kubernetes.Interface, options ExecOptions) (string, string, error) { | ||
| // ExecWithOptions executes a command in the specified container, returning an error. | ||
| // `options` allowed for additional parameters to be passed. | ||
| func ExecWithOptions(kubeCli kubernetes.Interface, options ExecOptions) error { |
There was a problem hiding this comment.
After looking at the desc, I thought, are there use cases where some consumers would need Exec and others would need ExecWithOptions, can we not just expose one. So that it's easier to use these APIs?
There was a problem hiding this comment.
In the ideal case, of course, it would be better to use ExecWithOptions only. But, unfortunately, both are widely used.
Looking on the code, I figured out that ExecOutput is probably even more redundant.
I can do bigger refactoring and reduce number of execs, but it will be bigger change, so maybe we can do that by two or three steps ? This one, drops buffering in ExecWithOptions, Second one will remove ExecOutput and, if we will agree, that we'd like to do that, the third (and the biggest one) can get rid of pure Exec ?
There was a problem hiding this comment.
Now, that I think about it again, I don't think we should block the PR in this discussion. I think names are sufficient enough to not cause any confusion.
If we want to exec with some options use ExecWithOptions otherwise use Exec.
Change Overview
Invoker of
kube.ExecWithOptionshas the possibility to get the stdout/stderr streams by passing appropriateio.Writers viaExecOptions. Since all its invokers do this, it's redundant to collect the same streams into internal buffers and return them as a string.At the same time, we have
kube.Execwhich is simplified version ofkube.ExecWithOptions. Thiskube.Execis the only case when we need to collect stdout/stderr to a buffer and return them after execution is done.Therefore, let's move this buffering to
kube.Execand simplifykube.ExecWithOptionsPull request type
Please check the type of change your PR introduces:
Test Plan