Refactor functions with pod runner #2308
Conversation
Rename to `MaybeWriteProfileCredentials`
It could be taken from `pod`
e-sumin
requested review from
pavannd1,
PrasadG193,
viveksinghggits,
r4rajat and
chaitalisg
|
Thanks for submitting this pull request 🎉. The team will review it soon and get back to you. If you haven't already, please take a moment to review our project contributing guideline and Code of Conduct document. |
|
I didn't split this PR to the train, because almost all changes are pretty similar. Also, you can review it commit by commit as they logically separated, this will simplify your work. |
| format.LogWithCtx(ctx, pod.Name, pod.Spec.Containers[0].Name, logs) | ||
| out, err := parseLogAndCreateOutput(logs) |
There was a problem hiding this comment.
I have strong feeling that this is almost equal to
out, err := output.LogAndParse(ctx, r)
where r is an io.ReaderCloser instance.
but decided not to change it in this PR to not bring irrelevant changes.
| var stdout, stderr bytes.Buffer | ||
| err = commandExecutor.Exec(ctx, cmd, nil, &stdout, &stderr) | ||
| format.LogWithCtx(ctx, pod.Name, pod.Spec.Containers[0].Name, stdout.String()) | ||
| format.LogWithCtx(ctx, pod.Name, pod.Spec.Containers[0].Name, stderr.String()) |
There was a problem hiding this comment.
this is very common pattern in functions - execute, log and then analyze output.
we can extract this functionality to utils and then reuse everywhere to reduce amount of code.
There was a problem hiding this comment.
also not sure whether we need to wait until end of execution and log at the end, probably we can log on the fly, since we passing io.Writer to Exec function.
There was a problem hiding this comment.
Agreed. Could you please create a GitHub issue with this suggestion? It could be good for first-time contributors
| var stdout, stderr bytes.Buffer | ||
| err = commandExecutor.Exec(ctx, cmd, nil, &stdout, &stderr) | ||
| format.LogWithCtx(ctx, pod.Name, pod.Spec.Containers[0].Name, stdout.String()) | ||
| format.LogWithCtx(ctx, pod.Name, pod.Spec.Containers[0].Name, stderr.String()) |
There was a problem hiding this comment.
Agreed. Could you please create a GitHub issue with this suggestion? It could be good for first-time contributors
Co-authored-by: Pavan Navarathna <[email protected]>
e-sumin
force-pushed
the
refactor_functions_with_pod_runner
branch
from
c4c35e7 to
db19f7f
Compare
Change Overview
This PR contains refactoring of all invokers of
PodRunner.Runas it was proposed in issue #2070Now all invokers are using
PodRunner.RunExwhich invokes functor withPodControllerinstance.Pull request type
Please check the type of change your PR introduces:
Issues
Test Plan