-
Notifications
You must be signed in to change notification settings - Fork 152
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor functions with pod runner #2308
Conversation
Rename to `MaybeWriteProfileCredentials`
It could be taken from `pod`
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Could you please create a GitHub issue with this suggestion? It could be good for first-time contributors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty straight forward. Approved!
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Could you please create a GitHub issue with this suggestion? It could be good for first-time contributors
Co-authored-by: Pavan Navarathna <6504783+pavannd1@users.noreply.github.com>
c4c35e7
to
db19f7f
Compare
Change Overview
This PR contains refactoring of all invokers of
PodRunner.Run
as it was proposed in issue #2070Now all invokers are using
PodRunner.RunEx
which invokes functor withPodController
instance.Pull request type
Please check the type of change your PR introduces:
Issues
Test Plan