Refactoring log reader. #749
Conversation
tekton-robot
added
the
size/L
| int(taskNum), lr.Follow, lr.AllSteps) | ||
| pipeLogs(logC, errC, tlr) | ||
| // clone the object to keep task number and name separately | ||
| c := r.clone() |
There was a problem hiding this comment.
By this the reference from pkg/helper/taskrun to pkg/cmd/taskrun package can be deleted.
pkg/helper/log/task_reader.go
Outdated
| // Check if taskrun failed on start up | ||
| if err := hasTaskRunFailed(tr.Status.Conditions, r.task); err != nil { | ||
| if r.stream != nil { | ||
| fmt.Fprintf(r.stream.Err, "%s\n", err.Error()) | ||
| } | ||
| return nil, nil, err |
There was a problem hiding this comment.
Isn't failed pod log shown again without -f flag 🤔 ?
(because return at this point before readStepsLogs)
following code is correct 🤔 ?
if r.stream != nil {
fmt.Fprintf(r.stream.Err, "%s\n", err.Error())
} else {
return nil, nil, err
}
There was a problem hiding this comment.
Yes, that was something that was overlooked on my part in #721. I think adding the else block makes sense.
Overall, we need to clean up error messaging in logs/have better testing around this. Maybe even an e2e scenario.
There was a problem hiding this comment.
Yes, that was something that was overlooked on my part in #721. I think adding the else block makes sense.
Oh, Thanks !!
I'll add else block in this PR for now 👍
Overall, we need to clean up error messaging in logs/have better testing around this. Maybe even an e2e scenario.
+1
related issue is tektoncd#748 taskrun's logreader and pipelinerun's logreader has a few same logic. It is useful and readable moving these code into same package.
16yuki0702
force-pushed
the
refactoring_log_reader
branch
from
e95f331
to
8fc2482
Compare
|
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
tekton-robot
added
the
approved
| } | ||
|
|
||
| //Sort taskruns, to display the taskrun logs as per pipeline tasks order | ||
| // Sort taskruns, to display the taskrun logs as per pipeline tasks order |
There was a problem hiding this comment.
We need to do sorting for readLivePipelineLogs too? This looks like a bug that hasn't been reported.
There was a problem hiding this comment.
Because readLivePipelineLogs get logs asynchronous in concurrent goroutine.
Logs are displayed in order from the started pod and this timing is uncontrollable.
if we sort order in readLivePipelineLogs we need to wait until all task pods will start.
(But it maybe lack of real time... tradeoff...🤔)
There was a problem hiding this comment.
Ahh... sorry, please let me considering it.
there is possibility I misunderstanding it, sorry 😅
There was a problem hiding this comment.
I follow what you're saying, and that was my thought here too as far as having to wait for all TaskRuns. I figured I would ask to make sure but no worries if this is something out of the scope of this pr. We can always iterate on it if there isn't a clear solution here.
There was a problem hiding this comment.
I follow what you're saying, and that was my thought here too as far as having to wait for all TaskRuns. I figured I would ask to make sure but no worries if this is something out of the scope of this pr. We can always iterate on it if there isn't a clear solution here.
Thank you very much !!
yes, it seems be better addressing this on another PR 🙏
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danielhelfand, vdemeester The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Related issue is #748
taskrun's logreader and pipelinerun's logreader has a few same logic.
moving these code into same package.
and remove reference from
pkg/helper/taskrun/taskrun.gotocmdpackage.Changes
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
make checkmake generatedSee the contribution guide
for more details.
Release Notes