Update kpod logs to use the new container state and runtime #62
Conversation
|
the --tail flag is still broken #16, but the tests pass (not sure why). Will get back to that later. |
|
LGTM, assuming happy tests. |
cmd/kpod/logs.go
Outdated
| @@ -4,8 +4,9 @@ import ( | |||
| "fmt" | |||
| "time" | |||
|
|
|||
| "github.com/projectatomic/libpod/libpod" | |||
|
|
|||
There was a problem hiding this comment.
Remove the extra spaces around the github lines.
libpod/runtime_ctr.go
Outdated
| "time" | ||
|
|
||
| "github.com/hpcloud/tail" | ||
|
|
libpod/runtime_ctr.go
Outdated
| return ErrRuntimeStopped | ||
| } | ||
|
|
||
| defer close(logChan) |
There was a problem hiding this comment.
Should this be before the check above? Does the user expect the channel to be closed when GetLogs returns?
There was a problem hiding this comment.
uh, not sure I just modified the code that was already written, which was like this.
libpod/runtime_ctr.go
Outdated
| return r.lookUpContainer(idOrName) | ||
| } | ||
|
|
||
| func (r *Runtime) lookUpContainer(idOrName string) (*Container, error) { |
There was a problem hiding this comment.
Why do we need this extra function?
There was a problem hiding this comment.
I was worried about the lock hanging again, but not needed anymore. fixed
libpod/runtime_ctr.go
Outdated
|
|
||
| // GetLogs returns the logs of a container from the log file | ||
| // log file is created when the container is started/ran | ||
| func (r *Runtime) GetLogs(container string, logChan chan string, opts LogOptions) error { |
There was a problem hiding this comment.
Can we move this out to cmd/kpod? I don't think we need anything in the runtime for this aside from the container, which you can just get with LookupContainer
|
I don't like having the log parsing inside the runtime. I think we should move it either into cmd/kpod or into container.go. Otherwise looks fine. |
umohnani8
force-pushed
the
kpod_logs
branch
2 times, most recently
from
0a5c238 to
0515ff9
Compare
Signed-off-by: Urvashi Mohnani <[email protected]>
|
LGTM. @rhatdan PTAL |
|
📌 Commit 2a51d53 has been approved by |
|
⚡ Test exempted: pull fully rebased and already tested. |
bridge: various fixes
github-actions
bot
added
the
locked - please file new issue/PR
Signed-off-by: Urvashi Mohnani [email protected]